<div dir="ltr">Hi Ivan, <div>Thanks for the detailed explanation. </div><div> </div><div>While primary termination is a known limitation, I believe the application or PMD should handle edge cases proactively to ensure stability. </div></div> <div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Tue, Jul 22, 2025 at 9:26 PM Ivan Malov <<a href="mailto:ivan.malov@arknetworks.am">ivan.malov@arknetworks.am</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Dariusz, Khadem, On Mon, 21 Jul 2025, Dariusz Sosnowski wrote: > On Mon, Jul 21, 2025 at 01:59:59PM +0200, Thomas Monjalon wrote: >> 21/07/2025 13:46, Ivan Malov: >>> On Mon, 21 Jul 2025, Dariusz Sosnowski wrote: >>> >>>> + mlx5 maintainers >>>> >>>> Thank you for the patch. >>>> >>>> Could you please include other PMD maintainers (or other maintainers, depending on changed code) >>>> in the future patches? >>>> There is a script which automatically adds maintainers while sending a patch. >>>> It is described in: <a href="https://doc.dpdk.org/guides/contributing/patches.html#sending-patches" rel="noreferrer" target="_blank">https://doc.dpdk.org/guides/contributing/patches.html#sending-patches</a> >>>> >>>> On Mon, Jul 21, 2025 at 03:38:51AM -0400, Khadem Ullah wrote: >>>>> When the primary process exits, the shared mlx5 state becomes >>>>> unavailable to secondary processes. If a secondary process attempts >>>>> to query device information (e.g., via testpmd), a NULL dereference >>>>> may occur due to missing shared data. >>>>> >>>>> This patch adds a check for shared context availability and fails >>>>> gracefully while preventing a crash. >>>>> >>>>> Fixes: e60fbd5b24fc ("mlx5: add device configure/start/stop") >>>>> Cc: <a href="mailto:stable@dpdk.org" target="_blank">stable@dpdk.org</a> >>>>> >>>>> Signed-off-by: Khadem Ullah <<a href="mailto:14pwcse1224@uetpeshawar.edu.pk" target="_blank">14pwcse1224@uetpeshawar.edu.pk</a>> >>>>> --- >>>>> drivers/net/mlx5/mlx5_ethdev.c | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c >>>>> index 68d1c1bfa7..1848f6536a 100644 >>>>> --- a/drivers/net/mlx5/mlx5_ethdev.c >>>>> +++ b/drivers/net/mlx5/mlx5_ethdev.c >>>>> @@ -368,6 +368,12 @@ mlx5_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *info) >>>>> * Since we need one CQ per QP, the limit is the minimum number >>>>> * between the two values. >>>>> */ >>>>> + if (priv == NULL || priv->sh == NULL) { >>>>> + DRV_LOG(ERR, >>>>> + "mlx5 shared data unavailable (primary process likely exited)"); >>>>> + rte_errno = ENODEV; >>>>> + return -rte_errno; >>>>> + } >>>> >>>> I don't think it's an issue on PMD level, but rather on >>>> ethdev/multi-process handling level. >>> >>> I may be very wrong here, but can't the PMD use its internal 'shared' state to >>> somehow memorise the fact that a secondary process has attached, in order to not >>> let the primary process close in the first place (before the secondary process >>> detaches)? Or is this going to violate some established convention? >> >> It looks to be a good idea. > > I agree with idea of adding these checks, but not entirely agree with > it being at driver level, since all drivers would have to duplicate this logic. > > Drivers already have to go through ethdev library when port is probed > on secondary process - rte_eth_dev_attach_secondary() must be > called to retrieve port data local to the process and device data shared > between processes. > Memorizing whether a secondary process is using given port can be added > to rte_eth_dev_attach_secondary(), and relevant check for primary > process can then be added to rte_eth_dev_close(), so that all drivers > benefit. > > What do you think? Yes, in general, the idea would be to have a "secondary reference counter" field in 'rte_eth_dev_data' and have it incremented/decremented/checked in some generic places. The issue is that, in addition to 'rte_eth_dev_close', a device can be "closed" via its respective bus 'remove' method. Yes, there are drivers that invoke 'rte_eth_dev_close' inside the 'remove' implementation, but not all of them, unfortunately. For example, take a look at 'af_packet' and similar. On the other hands, there are drivers that use 'rte_eth_dev_create' and its counterpart 'rte_eth_dev_destroy' in the implementations of bus 'probe/remove', but that is also not consistent across the 'drivers/net' tree. Given all these issues and the fact that ethdev is not the only possible device type, may be 'rte_device' can house the reference counter, with 'rte_dev_probe' and 'rte_dev_remove' augmented to do the inrement/decrement/validate thing? And, since 'rte_eth_dev_close' invokes direct ethdev 'close' method, also add a check to over there (and may be to 'rte_eth_dev_reset', too)? May be I'm wrong. However, all of that might still make no sense, given the fact that the primary process can just die. So, may be I am indeed very wrong and, as Stephen has suggested in [1], this issue it to be addressed by clarifying the documentation. [1] <a href="https://mails.dpdk.org/archives/dev/2025-July/321865.html" rel="noreferrer" target="_blank">https://mails.dpdk.org/archives/dev/2025-July/321865.html</a> Thank you. > >> >>>> When primary process closes the port, ethdev library zeroes and frees >>>> device data shared between processes. >>>> ethdev port data (rte_eth_dev) on secondary is not updated so it now points to >>>> invalid data. rte_eth_dev_info_get() is not the only API call affected. >>>> >>>> If the primary process closes the port before exiting >>>> (like testpmd does) and it exits before the secondary, >>>> the any driver call seems invalid because of that use-after-free behavior. >>>> >>>> @Thomas, @Andrew - Do you happen to know if doing anything on ethdev ports >>>> in secondary process after primary has gracefully exited is supported? >> >> No there is no statement about whether it is supported or not. >> I think we should at least return an error instead of crashing. >> >> > </blockquote></div><div> </div> -- <div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">Engr. Khadem Ullah, </div><div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">Software Engineer, </div>Dreambig Semiconductor Inc </div><div dir="ltr"><a href="https://dreambigsemi.com/" target="_blank">https://dreambigsemi.com/</a> </div></div></div></div>