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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 09 Feb 2021 10:12:11 +0100
From:   Antoine Tenart <atenart@...nel.org>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next v2 09/12] net-sysfs: remove the rtnl lock when accessing the xps maps

Quoting Alexander Duyck (2021-02-08 23:20:58)
> On Mon, Feb 8, 2021 at 9:19 AM Antoine Tenart <atenart@...nel.org> wrote:
> > @@ -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.

Yet another possible race :-) Good catch, I thought about the one we
fixed already but not this one.

As the sb_dev pointer is protected by the rtnl lock, we'll have to keep
the lock. I'll move that patch at the end of the series (it'll be easier
to add the get_device/put_device logic with the xps_queue_show
function). I'll also move netdev_txq_to_tc out of xps_queue_show, to
call it under the rtnl lock taken.

Thanks,
Antoine