Re: [PATCH net] netdevsim: delete ports added after a failed reload

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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 fix is incomplete, and the claim it rests on is wrong.

--

netdevsim: delete ports added after a failed reload

This patch makes nsim_dev_reload_destroy() call nsim_dev_port_del_all() on
the early-return path that is taken after a failed devlink reload. Before
this, ports added through new_port after the failed reload were never
deleted, which led to WARNs in devlink_free() and a use-after-free on the
leftover netdev.

Closes: https://syzkaller.appspot.com/bug?extid=88435e204734072bf39a

> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index feb88ccbbcedb..a8717908cfb83 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -1697,8 +1697,13 @@ 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)) {
> +		/* The failed reload already tore everything down, but ports
> +		 * can still be added through new_port after that.
> +		 */
> +		nsim_dev_port_del_all(nsim_dev);
>  		return;
> +	}

[Severity: Medium]
Is deleting the ports enough on this path? The commit message says:

  "The ports are the only thing that can be created after the failed
  reload, so delete them on the early-return path too."

After a failed reload, user space can apparently still set sriov_numvfs,
switch the eswitch to switchdev mode, and create devlink rate nodes. None
of these paths check devlink_is_reload_failed():

  nsim_bus_dev_numvfs_store()
    nsim_drv_configure_vfs()
      nsim_bus_dev_set_vfs(nsim_bus_dev, num_vfs);
      nsim_esw_switchdev_enable()

  nsim_devlink_eswitch_mode_set()
    nsim_esw_switchdev_enable()
      __nsim_dev_port_add(nsim_dev, NSIM_DEV_PORT_TYPE_VF, i, NULL);
      nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_SWITCHDEV;

  devlink_nl_rate_new_doit()
    nsim_rate_node_new()
      if (!nsim_esw_mode_is_switchdev(nsim_dev)) {
          ...
          return -EOPNOTSUPP;
      }

Before deleting ports, the normal path further down in
nsim_dev_reload_destroy() resets the VFs and tears down the eswitch state:

	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);
	}

nsim_esw_legacy_enable() is the only caller of devl_rate_nodes_destroy() in
netdevsim. The new early-return branch never reaches it.

Take this sequence: fail_reload=1, reload fails, set sriov_numvfs, switch to
switchdev, run "devlink port function rate add <dev>/node1", then
del_device. The early branch removes the ports but leaves the rate node on
devlink->rate_list. Would devlink_free() then hit this check:

net/devlink/core.c:devlink_free() {
    ...
	WARN_ON(devlink_rates_check(devlink, NULL, NULL));
    ...
}

and leak the devlink_rate and the nsim_rate_node?

The commit message also says a successful reload after the failed one is
fixed. In that case the early branch deletes the VF ports but leaves
num_vfs set and esw_mode as switchdev. Any remaining rate node would trip
WARN_ON(devlink_rates_check(...)) in devlink_reload_reinit_sanity_check().

After that, nsim_dev_reload_create() re-adds only the PF ports. The device
then ends up in switchdev mode with VFs configured but no VF representor
ports. A normal reload never leaves the device in that state.

Should the failed branch do the same VF reset and nsim_esw_legacy_enable()
teardown as the normal path before calling nsim_dev_port_del_all()? If not,
the "only thing that can be created" sentence in the commit message should
probably be corrected.

>  	debugfs_remove(nsim_dev->take_snapshot);
>  
>  	if (nsim_dev_get_vfs(nsim_dev)) {

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