Re: [PATCH net-next v2 09/12] net-sysfs: remove the rtnl lock when accessing the xps maps

[Alexander Duyck <alexander.duyck@gmail.com>]

On Mon, Feb 8, 2021 at 9:19 AM Antoine Tenart <atenart@kernel.org> wrote:
>
> Now that nr_ids and num_tc are stored in the xps dev_maps, which are RCU
> protected, we do not have the need to protect the xps_cpus_show and
> xps_rxqs_show functions with the rtnl lock.
>
> Signed-off-by: Antoine Tenart <atenart@kernel.org>
> ---
>  net/core/net-sysfs.c | 26 ++++----------------------
>  1 file changed, 4 insertions(+), 22 deletions(-)
>
> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
> index c2276b589cfb..6ce5772e799e 100644
> --- a/net/core/net-sysfs.c
> +++ b/net/core/net-sysfs.c
> @@ -1328,17 +1328,12 @@ static ssize_t xps_cpus_show(struct netdev_queue *queue,
>
>         index = get_netdev_queue_index(queue);
>
> -       if (!rtnl_trylock())
> -               return restart_syscall();
> -
>         /* If queue belongs to subordinate dev use its map */
>         dev = netdev_get_tx_queue(dev, index)->sb_dev ? : dev;
>
>         tc = netdev_txq_to_tc(dev, index);
> -       if (tc < 0) {
> -               ret = -EINVAL;
> -               goto err_rtnl_unlock;
> -       }
> +       if (tc < 0)
> +               return -EINVAL;
>
>         rcu_read_lock();
>         dev_maps = rcu_dereference(dev->xps_maps[XPS_CPUS]);

So I think we hit a snag here. The sb_dev pointer is protected by the
rtnl_lock. So I don't think we can release the rtnl_lock until after
we are done with the dev pointer.

Also I am not sure it is safe to use netdev_txq_to_tc without holding
the lock. I don't know if we ever went through and guaranteed that it
will always work if the lock isn't held since in theory the device
could reprogram all the map values out from under us.

Odds are we should probably fix traffic_class_show as I suspect it
probably also needs to be holding the rtnl_lock to prevent any
possible races. I'll submit a patch for that.

> @@ -1371,16 +1366,12 @@ static ssize_t xps_cpus_show(struct netdev_queue *queue,
>  out_no_maps:
>         rcu_read_unlock();
>
> -       rtnl_unlock();
> -
>         len = bitmap_print_to_pagebuf(false, buf, mask, nr_ids);
>         bitmap_free(mask);
>         return len < PAGE_SIZE ? len : -EINVAL;
>
>  err_rcu_unlock:
>         rcu_read_unlock();
> -err_rtnl_unlock:
> -       rtnl_unlock();
>         return ret;
>  }
>
> @@ -1435,14 +1426,9 @@ static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf)
>
>         index = get_netdev_queue_index(queue);
>
> -       if (!rtnl_trylock())
> -               return restart_syscall();
> -
>         tc = netdev_txq_to_tc(dev, index);
> -       if (tc < 0) {
> -               ret = -EINVAL;
> -               goto err_rtnl_unlock;
> -       }
> +       if (tc < 0)
> +               return -EINVAL;
>
>         rcu_read_lock();
>         dev_maps = rcu_dereference(dev->xps_maps[XPS_RXQS]);
> @@ -1475,8 +1461,6 @@ static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf)
>  out_no_maps:
>         rcu_read_unlock();
>
> -       rtnl_unlock();
> -
>         len = bitmap_print_to_pagebuf(false, buf, mask, nr_ids);
>         bitmap_free(mask);
>
> @@ -1484,8 +1468,6 @@ static ssize_t xps_rxqs_show(struct netdev_queue *queue, char *buf)
>
>  err_rcu_unlock:
>         rcu_read_unlock();
> -err_rtnl_unlock:
> -       rtnl_unlock();
>         return ret;
>  }
>
> --
> 2.29.2
>