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: <CAKgT0UfzNA8qk+QFTN6ihXTxZkcE=vfrjBtyHKL6_9Yyzxt=eQ@mail.gmail.com>
Date:   Tue, 22 Dec 2020 08:12:28 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Antoine Tenart <atenart@...nel.org>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        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, Dec 22, 2020 at 1:21 AM Antoine Tenart <atenart@...nel.org> wrote:
>
> Hello Alexander, Jakub,
>
> 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.

> 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.

I agree the code factoring would be better targeted to net-next.

The rtnl_lock approach from v1 would work for net and for backports.

> Jakub, should I send a v3 then?
>
> Thanks!
> Antoine