Re: [PATCH net] netdevsim: reject port addition and clean up ports after a failed reload

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch is meant to block port creation with -EIO after a
  failed reload, but the check only lives in nsim_drv_port_add(), so VF…

Pre-existing issues:
- [High] Pre-existing, not introduced by this patch: new_port_store()
  (drivers/net/netdevsim/bus.c) calls nsim_drv_port_add() without…

--

netdevsim: reject port addition and clean up ports after a failed reload

After a failed devlink reload, nsim_drv_port_add() now returns -EIO.
nsim_dev_reload_destroy() now deletes any leftover ports on its early
return path when reload has failed. This fixes a syzbot report of a
WARN_ON in bpf_offload_dev_destroy() followed by a use-after-free in
nsim_drv_remove().

> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index feb88ccbbcedb..61f0824ce4d3b 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -1697,8 +1697,10 @@ static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev)
>  {
>  	struct devlink *devlink = priv_to_devlink(nsim_dev);
>  
> -	if (devlink_is_reload_failed(devlink))
> +	if (devlink_is_reload_failed(devlink)) {
> +		nsim_dev_port_del_all(nsim_dev);
>  		return;
> +	}
>  	debugfs_remove(nsim_dev->take_snapshot);
>  
>  	if (nsim_dev_get_vfs(nsim_dev)) {

[ ... ]

> @@ -1758,7 +1760,9 @@ int nsim_drv_port_add(struct nsim_bus_dev *nsim_bus_dev, enum nsim_dev_port_type
>  	int err;
>  
>  	devl_lock(priv_to_devlink(nsim_dev));

[Severity: High]
This is a pre-existing issue, but can nsim_drv_port_add() be reached here
with a NULL or freed nsim_dev?

In drivers/net/netdevsim/bus.c, new_port_store() calls nsim_drv_port_add()
without taking device_lock() and without checking drvdata. The sibling
handler nsim_bus_dev_numvfs_store() does both:

	device_lock(dev);
	ret = -ENOENT;
	if (dev_get_drvdata(dev))
		ret = nsim_drv_configure_vfs(nsim_bus_dev, num_vfs);
	device_unlock(dev);

The netdevsim driver does not set suppress_bind_attrs. Unbinding it runs
nsim_bus_remove()->nsim_drv_remove(), which ends with:

	devlink_free(devlink);
	dev_set_drvdata(&nsim_bus_dev->dev, NULL);

nsim_bus_dev->init stays true and the new_port attribute is not removed,
so the following sequence seems possible:

  echo netdevsimN > /sys/bus/netdevsim/drivers/netdevsim/unbind
  echo 0 > /sys/bus/netdevsim/devices/netdevsimN/new_port

new_port_store() passes the init check, then dev_get_drvdata() in
nsim_drv_port_add() returns NULL. After that,
devl_lock(priv_to_devlink(NULL)) dereferences an invalid pointer.

Suppose instead the unbind races with a new_port write and lands between
dev_get_drvdata() and devl_lock(). Would that become a use-after-free of
the devlink allocation?

The new reload_failed check runs after devl_lock(), so it does not help
here. del_port_store()->nsim_drv_port_del() appears to have the same
pattern.

> -	if (__nsim_dev_port_lookup(nsim_dev, type, port_index))
> +	if (devlink_is_reload_failed(priv_to_devlink(nsim_dev)))
> +		err = -EIO;

[Severity: Medium]
The commit message says "Block port creation with -EIO when reload has
failed". Does this check cover every path that creates ports?

nsim_esw_switchdev_enable() calls __nsim_dev_port_add() directly and does
not check devlink_is_reload_failed():

drivers/net/netdevsim/dev.c:nsim_esw_switchdev_enable() {
	for (i = 0; i < nsim_dev_get_vfs(nsim_dev); i++) {
		err = __nsim_dev_port_add(nsim_dev, NSIM_DEV_PORT_TYPE_VF, i, NULL);
	...
}

After a failed reload, it can still be reached in two ways:

nsim_bus_dev_numvfs_store()
  nsim_drv_configure_vfs()
    nsim_esw_switchdev_enable()
      __nsim_dev_port_add()

devlink_nl_eswitch_set_doit()
  nsim_devlink_eswitch_mode_set()
    nsim_esw_switchdev_enable()
      __nsim_dev_port_add()

The devlink core does not block driver ops when reload_failed is set.
esw_mode can also stay at DEVLINK_ESWITCH_MODE_SWITCHDEV across a failed
reload, for two reasons:

- nsim_dev_reload_destroy() only calls nsim_esw_legacy_enable() when
  num_vfs is non-zero.
- nsim_dev_reload_create() does not reset esw_mode.

For example:

  devlink dev eswitch set ... mode switchdev   (with 0 VFs)
  set fail_reload in debugfs
  devlink dev reload ...                       (reload_failed is set)
  echo N > sriov_numvfs

This seems to create N VF netdevs and devlink ports on a device whose
traps, fib, health, psample and hwstats are already torn down. It never
returns -EIO.

At remove time, the new nsim_dev_port_del_all() call in the reload_failed
branch deletes these ports, so the bpf_offload_dev_destroy() warning does
not come back this way. However, that branch skips the VF and eswitch
reset that the normal path does:

	if (nsim_dev_get_vfs(nsim_dev)) {
		nsim_bus_dev_set_vfs(nsim_dev->nsim_bus_dev, 0);
		if (nsim_esw_mode_is_switchdev(nsim_dev))
			nsim_esw_legacy_enable(nsim_dev, NULL);
	}

After a successful retry reload, wouldn't the device be left in switchdev
mode with num_vfs == N but no VF representor ports?

A second eswitch set to switchdev returns 0 early because
mode == esw_mode. Writing the same num_vfs value is also a no-op. The
representors would only come back after num_vfs is cleared or the mode is
toggled.

Could the reload_failed check go into __nsim_dev_port_add() or
nsim_esw_switchdev_enable() instead? Putting it in nsim_drv_configure_vfs()
and nsim_devlink_eswitch_mode_set() would also work. Either way, all
port-creation paths would be covered.

> +	else if (__nsim_dev_port_lookup(nsim_dev, type, port_index))
>  		err = -EEXIST;
>  	else
>  		err = __nsim_dev_port_add(nsim_dev, type, port_index, perm_addr);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029145844.231018-1-omermetekaya0%40gmail.com