[PATCH 3/3] net/vdev: fix insert vdev core dump
Burakov, Anatoly
anatoly.burakov at intel.com
Fri Jul 12 10:38:44 CEST 2024
On 7/12/2024 4:18 AM, Ye, MingjinX wrote:
>
>
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov at intel.com>
>> Sent: Friday, July 12, 2024 12:10 AM
>> To: Ye, MingjinX <mingjinx.ye at intel.com>; dev at dpdk.org
>> Cc: stable at dpdk.org
>> Subject: Re: [PATCH 3/3] net/vdev: fix insert vdev core dump
>>
>> On 3/14/2024 10:36 AM, Mingjin Ye wrote:
>>> Inserting a vdev device when the device arguments are already stored
>>> in devargs_list, the rte_devargs_insert function replaces the supplied
>>> new devargs with the found devargs and frees the new devargs. As a
>>> result, the use of free devargs results in a core dump.
>>>
>>> This patch fixes the issue by using valid devargs.
>>>
>>> Fixes: f3a1188cee4a ("devargs: make device representation generic")
>>> Cc: stable at dpdk.org
>>>
>>> Signed-off-by: Mingjin Ye <mingjinx.ye at intel.com>
>>
>> I am not too familiar with how devargs works, so I have a trivial question.
>>
>> I understand the point of this patch, and it is roughly as follows:
>>
>> 1) we enter insert_vdev and allocated new devargs structure (and copy the
>> `name` into devargs)
>> 2) we set dev->device.name to devargs->name
>> 3) we insert devargs into the list using rte_devargs_insert
>> 4) if devargs list already had devargs with the same name, our devargs is
>> destroyed and replaced with devargs that is in the list
>> 5) because of this, dev->device.name becomes invalid as that specific
>> devargs has been freed - it points to name from the old devargs
>>
>> We do need to store devargs->name in dev->device.name, and we need to
>> do so after calling rte_devargs_insert to avoid keeping reference to memory
>> that was freed. So, provisionally,
>>
>> Reviewed-by: Anatoly Burakov <anatoly.burakov at intel.com>
>>
>> My question is, under what circumstances would there be duplicate entries
>> in devargs list?
>
> In a multi-process circumstances, the primary and secondary processes both have "vdev" startup
> Parameters. And their parameters have the same name. This causes multiple devargs objects with the same name
> to be created in the secondary process, the 1st one constructed from the startup parameter and
> the 2nd one constructed due to the VDEV_SCAN_ONE message received.
>
> Path 1:
> eal_parse_args->eal_parse_common_option: OPT_VDEV_NUM
> eal_option_device_parse->rte_devargs_add
>
> Path 2:
> vdev_action: vdev_scan_one ->insert_vdev(alloc_devargs)
So, technically, this is only a problem because we're specifying --vdev
at secondary process startup even though we could've just relied on vdev
multiprocess hotplug to provide us with a device? Or is there more to it?
In any case, I think this patch is OK. I think the Fixes: tag is
pointing to an incorrect commit. I've walked through the commit history,
and it seems that this wasn't a bug initially, but became a bug when
vdev multiprocess hotplug was added, so I think the correct Fixes: tag
should be as follows:
Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
So, on account of the above, I suggest rewording the commit message to
something like the following:
In secondary processes, insert_vdev() may be called multiple times on
the same device due to a combination of multiprocess hotplug for vdev
bus, and EAL argument adding the same vdev. In this circumstance, when
rte_devargs_insert() is called, the devargs->name reference becomes
invalid because rte_devargs_insert() will destroy the just-allocated
devargs and will replace the pointer with one from the devargs list. As
a result, the reference to devargs->name stored in dev->device.name
becomes invalid. Fix the issue by not setting device name until after
rte_devargs_insert() was called.
Fixes: cdb068f031c6 ("bus/vdev: scan by multi-process channel")
Cc: stable at dpdk.org
Signed-off-by: Mingjin Ye <mingjinx.ye at intel.com>
Reviewed-by: Anatoly Burakov <anatoly.burakov at intel.com>
--
Thanks,
Anatoly
More information about the dev
mailing list