netdev - Re: [PATCH net v2 1/3] net: fix race conditions in xps by locking the maps and dev->tc

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20201223102729.6463a5c2@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 23 Dec 2020 10:27:29 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Antoine Tenart <atenart@...nel.org>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net v2 1/3] net: fix race conditions in xps by locking
 the maps and dev->tc_num

On Tue, 22 Dec 2020 08:12:28 -0800 Alexander Duyck wrote:
> On Tue, Dec 22, 2020 at 1:21 AM Antoine Tenart <atenart@...nel.org> wrote:
> > Quoting Alexander Duyck (2020-12-22 00:21:57)  
> > >
> > > Looking over this patch it seems kind of obvious that extending the
> > > xps_map_mutex is making things far more complex then they need to be.
> > >
> > > Applying the rtnl_mutex would probably be much simpler. Although as I
> > > think you have already discovered we need to apply it to the store,
> > > and show for this interface. In addition we probably need to perform
> > > similar locking around traffic_class_show in order to prevent it from
> > > generating a similar error.  
> >
> > I don't think we have the same kind of issues with traffic_class_show:
> > dev->num_tc is used, but not for navigating through the map. Protecting
> > only a single read wouldn't change much. We can still think about what
> > could go wrong here without the lock, but that is not related to this
> > series of fixes.  
> 
> The problem is we are actually reading the netdev, tx queue, and
> tc_to_txq mapping. Basically we have several different items that we
> are accessing at the same time. If any one is updated while we are
> doing it then it will throw things off.
> 
> > If I understood correctly, as things are a bit too complex now, you
> > would prefer that we go for the solution proposed in v1?  
> 
> Yeah, that is what I am thinking. Basically we just need to make sure
> the num_tc cannot be updated while we are reading the other values.

Yeah, okay, as much as I dislike this approach 300 lines may be a little
too large for stable.

> > I can still do the code factoring for the 2 sysfs show operations, but
> > that would then target net-next and would be in a different series. So I
> > believe we'll use the patches of v1, unmodified.  

Are you saying just patch 3 for net-next? We need to do something about
the fact that with sysfs taking rtnl_lock xps_map_mutex is now entirely
pointless. I guess its value eroded over the years since Tom's initial
design so we can just get rid of it.