netdev - Re: [PATCH net-next 1/3] net: provide macros for commonly copied lockless queue stop/wake code

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: <ZCvGDxW+HkcHYaU/@gondor.apana.org.au>
Date:   Tue, 4 Apr 2023 14:39:11 +0800
From:   Herbert Xu <herbert@...dor.apana.org.au>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        Heiner Kallweit <hkallweit1@...il.com>, davem@...emloft.net,
        netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com
Subject: Re: [PATCH net-next 1/3] net: provide macros for commonly copied
 lockless queue stop/wake code

On Mon, Apr 03, 2023 at 08:18:04AM -0700, Alexander Duyck wrote:
>
> > I *think* that the right ordering would be:
> >
> > WRITE cons
> > mb()  # A
> > READ stopped
> > rmb() # C
> > READ prod, cons
> 
> What would the extra rmb() get you? The mb() will have already flushed
> out any writes and if stopped is set the tail should have already been
> written before setting it.
> 
> One other thing to keep in mind is that the wake gives itself a pretty
> good runway. We are talking about enough to transmit at least 2
> frames. So if another consumer is stopping it we aren't waking it
> unless there is enough space for yet another frame after the current
> consumer.
> 
> > And on the producer side (existing):
> >
> > WRITE prod
> > READ prod, cons
> > mb()  # B
> > WRITE stopped
> > READ prod, cons
> >
> > But I'm slightly afraid to change it, it's been working for over
> > a decade :D
> 
> I wouldn't change it. The code has predated BQL in the e1000 driver
> and has been that way since the inception of it I believe in 2.6.19.

Thanks for adding me to this thread as otherwise I would've surely
missed it.

I see where the confusion is coming from.  The key is that we weren't
trying to stop every single race, because not all of them are fatal.

In particular, we tolerate the race where a wake is done when it
shouldn't be because the network stack copes with that by requeueing
the skb onto the qdisc.

So it's a trade-off.  We could make our code water-tight, but then
we would be incurring a penalty for every skb.  With our current
approach, the penalty is only incurred in the unlikely event of a
race which results in the unlucky skb being requeued.

The race that we do want to stop is a queue being stuck in a stopped
state when it shouldn't because that indeed is fatal.

Going back to the proposed helpers, we only need one mb because
that's all we need to fix the stuck/stopped queue race.

Thanks,
-- 
Email: Herbert Xu <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt