Netdev List
 help / color / mirror / Atom feed
* [PATCH net v2] netdevsim: avoid NULL dereference after failed probe
@ 2026-08-19 14:21 Jiacheng Xu
  2026-08-20  6:26 ` Hangbin Liu
  0 siblings, 1 reply; 3+ messages in thread
From: Jiacheng Xu @ 2026-08-19 14:21 UTC (permalink / raw)
  To: hangbin.liu; +Cc: kuba, andrew+netdev, netdev, davem, edumazet, pabeni

device_register() reports whether device registration succeeded, not
whether the matching driver's probe succeeded. If nsim_drv_probe()
fails, the driver core leaves the nsim_bus_dev registered while the probe
error path clears its driver data.

new_device_store() subsequently marks the nsim_bus_dev initialized. A
write to its new_port or del_port attribute therefore passes the init
check and calls nsim_drv_port_add() or nsim_drv_port_del() with no valid
nsim_dev. Both helpers pass the NULL driver data to priv_to_devlink(),
leading to a NULL pointer dereference.

This can be reproduced by creating a netdevsim device for which
nsim_drv_probe() fails and then writing to the new_port or del_port
attribute of the registered device.

Serialize the driver data check and the port operation with the device
lock, and reject the operation with -ENODEV when no driver data is
present. This also prevents driver unbind from freeing nsim_dev between
the check and its use, and follows the locking used by sriov_numvfs.

Fixes: 794b2c05ca1c ("netdevsim: extend device attrs to support port addition and deletion")
Cc: stable@vger.kernel.org
Signed-off-by: Jiacheng Xu <stitch@zju.edu.cn>
---
Changes in v2:
- Target the patch to the net tree.
- Remove the reproducer attachment and describe the trigger in the
  commit message.

drivers/net/netdevsim/bus.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
index 41483e371f05..0ab51306ad0a 100644
--- a/drivers/net/netdevsim/bus.c
+++ b/drivers/net/netdevsim/bus.c
@@ -93,8 +93,13 @@ new_port_store(struct device *dev, struct device_attribute *attr,
                return -EINVAL;
        }

-       ret = nsim_drv_port_add(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF, port_index,
-                               addr_set ? eth_addr : NULL);
+       device_lock(dev);
+       if (!dev_get_drvdata(dev))
+               ret = -ENODEV;
+       else
+               ret = nsim_drv_port_add(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF,
+                                       port_index, addr_set ? eth_addr : NULL);
+       device_unlock(dev);
        return ret ? ret : count;
 }

@@ -115,7 +120,13 @@ del_port_store(struct device *dev, struct device_attribute *attr,
        if (ret)
                return ret;

-       ret = nsim_drv_port_del(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF, port_index);
+       device_lock(dev);
+       if (!dev_get_drvdata(dev))
+               ret = -ENODEV;
+       else
+               ret = nsim_drv_port_del(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF,
+                                       port_index);
+       device_unlock(dev);
        return ret ? ret : count;
 }

--
2.25.1

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH net v2] netdevsim: avoid NULL dereference after failed probe
  2026-08-19 14:21 [PATCH net v2] netdevsim: avoid NULL dereference after failed probe Jiacheng Xu
@ 2026-08-20  6:26 ` Hangbin Liu
  2026-08-20  9:25   ` Jiacheng Xu
  0 siblings, 1 reply; 3+ messages in thread
From: Hangbin Liu @ 2026-08-20  6:26 UTC (permalink / raw)
  To: Jiacheng Xu; +Cc: kuba, andrew+netdev, netdev, davem, edumazet, pabeni

Hi Jiacheng,
On Wed, Aug 19, 2026 at 10:21:30PM +0800, Jiacheng Xu wrote:
> device_register() reports whether device registration succeeded, not
> whether the matching driver's probe succeeded. If nsim_drv_probe()
> fails, the driver core leaves the nsim_bus_dev registered while the probe
> error path clears its driver data.

I'm not very familiar with this part. Can't we just unregister the
device if probe failed?

BTW, your code is not rebased on latest net. Please do rebase if you
need to post a new version.

Thanks
Hangbin
> 
> new_device_store() subsequently marks the nsim_bus_dev initialized. A
> write to its new_port or del_port attribute therefore passes the init
> check and calls nsim_drv_port_add() or nsim_drv_port_del() with no valid
> nsim_dev. Both helpers pass the NULL driver data to priv_to_devlink(),
> leading to a NULL pointer dereference.
> 
> This can be reproduced by creating a netdevsim device for which
> nsim_drv_probe() fails and then writing to the new_port or del_port
> attribute of the registered device.
> 
> Serialize the driver data check and the port operation with the device
> lock, and reject the operation with -ENODEV when no driver data is
> present. This also prevents driver unbind from freeing nsim_dev between
> the check and its use, and follows the locking used by sriov_numvfs.
> 
> Fixes: 794b2c05ca1c ("netdevsim: extend device attrs to support port addition and deletion")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiacheng Xu <stitch@zju.edu.cn>
> ---
> Changes in v2:
> - Target the patch to the net tree.
> - Remove the reproducer attachment and describe the trigger in the
>   commit message.
> 
> drivers/net/netdevsim/bus.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
> index 41483e371f05..0ab51306ad0a 100644
> --- a/drivers/net/netdevsim/bus.c
> +++ b/drivers/net/netdevsim/bus.c
> @@ -93,8 +93,13 @@ new_port_store(struct device *dev, struct device_attribute *attr,
>                 return -EINVAL;
>         }
> 
> -       ret = nsim_drv_port_add(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF, port_index,
> -                               addr_set ? eth_addr : NULL);
> +       device_lock(dev);
> +       if (!dev_get_drvdata(dev))
> +               ret = -ENODEV;
> +       else
> +               ret = nsim_drv_port_add(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF,
> +                                       port_index, addr_set ? eth_addr : NULL);
> +       device_unlock(dev);
>         return ret ? ret : count;
>  }
> 
> @@ -115,7 +120,13 @@ del_port_store(struct device *dev, struct device_attribute *attr,
>         if (ret)
>                 return ret;
> 
> -       ret = nsim_drv_port_del(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF, port_index);
> +       device_lock(dev);
> +       if (!dev_get_drvdata(dev))
> +               ret = -ENODEV;
> +       else
> +               ret = nsim_drv_port_del(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF,
> +                                       port_index);
> +       device_unlock(dev);
>         return ret ? ret : count;
>  }
> 
> --
> 2.25.1

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: Re: [PATCH net v2] netdevsim: avoid NULL dereference after failed probe
  2026-08-20  6:26 ` Hangbin Liu
@ 2026-08-20  9:25   ` Jiacheng Xu
  0 siblings, 0 replies; 3+ messages in thread
From: Jiacheng Xu @ 2026-08-20  9:25 UTC (permalink / raw)
  To: Hangbin Liu; +Cc: kuba, andrew+netdev, netdev, davem, edumazet, pabeni

Hi Hangbin,

I am not very familiar with this part either. My current understanding 
is that the default Linux device-model behavior is to keep a device 
registered when its probe callback fails, leaving it unbound.

However, for netdevsim, since the device is explicitly created through
the new_device sysfs interface, unregistering it after a failed probe
also seems reasonable. I would appreciate confirmation from you and the 
other developers that this is the preferred behavior for netdevsim.

If so, I will rework the patch to unregister the device when probe fails
and send an updated version. I will also rebase it on the latest net
branch before reposting.

Thanks,
Jiacheng

> -----原始邮件-----
> 发件人: "Hangbin Liu" <hangbin.liu@linux.dev>
> 发送时间:2026-08-20 14:26:48 (星期四)
> 收件人: "Jiacheng Xu" <stitch@zju.edu.cn>
> 抄送: kuba@kernel.org, andrew+netdev@lunn.ch, netdev@vger.kernel.org, davem@davemloft.net, edumazet@google.com, pabeni@redhat.com
> 主题: Re: [PATCH net v2] netdevsim: avoid NULL dereference after failed probe
> 
> Hi Jiacheng,
> On Wed, Aug 19, 2026 at 10:21:30PM +0800, Jiacheng Xu wrote:
> > device_register() reports whether device registration succeeded, not
> > whether the matching driver's probe succeeded. If nsim_drv_probe()
> > fails, the driver core leaves the nsim_bus_dev registered while the probe
> > error path clears its driver data.
> 
> I'm not very familiar with this part. Can't we just unregister the
> device if probe failed?
> 
> BTW, your code is not rebased on latest net. Please do rebase if you
> need to post a new version.
> 
> Thanks
> Hangbin
> > 
> > new_device_store() subsequently marks the nsim_bus_dev initialized. A
> > write to its new_port or del_port attribute therefore passes the init
> > check and calls nsim_drv_port_add() or nsim_drv_port_del() with no valid
> > nsim_dev. Both helpers pass the NULL driver data to priv_to_devlink(),
> > leading to a NULL pointer dereference.
> > 
> > This can be reproduced by creating a netdevsim device for which
> > nsim_drv_probe() fails and then writing to the new_port or del_port
> > attribute of the registered device.
> > 
> > Serialize the driver data check and the port operation with the device
> > lock, and reject the operation with -ENODEV when no driver data is
> > present. This also prevents driver unbind from freeing nsim_dev between
> > the check and its use, and follows the locking used by sriov_numvfs.
> > 
> > Fixes: 794b2c05ca1c ("netdevsim: extend device attrs to support port addition and deletion")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Jiacheng Xu <stitch@zju.edu.cn>
> > ---
> > Changes in v2:
> > - Target the patch to the net tree.
> > - Remove the reproducer attachment and describe the trigger in the
> >   commit message.
> > 
> > drivers/net/netdevsim/bus.c | 17 ++++++++++++++---
> > 1 file changed, 14 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
> > index 41483e371f05..0ab51306ad0a 100644
> > --- a/drivers/net/netdevsim/bus.c
> > +++ b/drivers/net/netdevsim/bus.c
> > @@ -93,8 +93,13 @@ new_port_store(struct device *dev, struct device_attribute *attr,
> >                 return -EINVAL;
> >         }
> > 
> > -       ret = nsim_drv_port_add(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF, port_index,
> > -                               addr_set ? eth_addr : NULL);
> > +       device_lock(dev);
> > +       if (!dev_get_drvdata(dev))
> > +               ret = -ENODEV;
> > +       else
> > +               ret = nsim_drv_port_add(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF,
> > +                                       port_index, addr_set ? eth_addr : NULL);
> > +       device_unlock(dev);
> >         return ret ? ret : count;
> >  }
> > 
> > @@ -115,7 +120,13 @@ del_port_store(struct device *dev, struct device_attribute *attr,
> >         if (ret)
> >                 return ret;
> > 
> > -       ret = nsim_drv_port_del(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF, port_index);
> > +       device_lock(dev);
> > +       if (!dev_get_drvdata(dev))
> > +               ret = -ENODEV;
> > +       else
> > +               ret = nsim_drv_port_del(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF,
> > +                                       port_index);
> > +       device_unlock(dev);
> >         return ret ? ret : count;
> >  }
> > 
> > --
> > 2.25.1

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-08-20  9:25 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-19 14:21 [PATCH net v2] netdevsim: avoid NULL dereference after failed probe Jiacheng Xu
2026-08-20  6:26 ` Hangbin Liu
2026-08-20  9:25   ` Jiacheng Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox