netdev - Re: [PATCH net 1/2] net: move mp dev config validation to __net_mp_open

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: <CAHS8izPwADTAf1NxRXECDhYQNRsNRq-A2Cwzq7Hy_b=Gjc_pJg@mail.gmail.com>
Date: Wed, 2 Apr 2025 11:46:02 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Pavel Begunkov <asml.silence@...il.com>, davem@...emloft.net, netdev@...r.kernel.org, 
	edumazet@...gle.com, pabeni@...hat.com, andrew+netdev@...n.ch, 
	horms@...nel.org, ap420073@...il.com, dw@...idwei.uk, sdf@...ichev.me
Subject: Re: [PATCH net 1/2] net: move mp dev config validation to __net_mp_open_rxq()

On Tue, Apr 1, 2025 at 8:00 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue, 1 Apr 2025 12:37:34 +0100 Pavel Begunkov wrote:
> > > -   err = xa_alloc(&binding->bound_rxqs, &xa_idx, rxq, xa_limit_32b,
> > > -                  GFP_KERNEL);
> > > +   err = __net_mp_open_rxq(dev, rxq_idx, &mp_params, extack);
> > >     if (err)
> > >             return err;
> >
> > Was reversing the order b/w open and xa_alloc intentional?
> > It didn't need __net_mp_close_rxq() before, which is a good thing
> > considering the error handling in __net_mp_close_rxq is a bit
> > flaky (i.e. the WARN_ON at the end).
>
> Should have mentioned.. yes, intentional, otherwise we'd either have to
> insert a potentially invalid rxq pointer into the xarray or duplicate
> the rxq bounds check. Inserting invalid pointer and deleting it immediately
> should be okay, since readers take the instance lock, but felt a little
> dirty. In practice xa_alloc() failures should be extremely rare here so
> I went with the reorder.


FWIW yes I imagine accessors of binding->bound_rxqs should all have
binding->dev locked at that point (and rtnl_locked before the locking
changes), and I preferred the other order just because restarting the
queue seemed like a heavy operation with all the ndos it calls, even
if it doesn't hit the WARN_ON.

But the error path should be rare anyway, and I think that works too.
I could not find other issues in the patch.

Reviewed-by: Mina Almasry <almasrymina@...gle.com>

-- 
Thanks,
Mina