netdev - Re: [PATCH] net: skb content must be visible for lockless skb

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]

Date:   Mon, 01 Aug 2022 11:59:54 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Kirill Tkhai <tkhai@...ru>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: skb content must be visible for lockless
 skb_peek() and its variations

On Mon, 2022-08-01 at 10:00 +0300, Kirill Tkhai wrote:
> On 01.08.2022 09:52, Paolo Abeni wrote:
> > On Sun, 2022-07-31 at 23:39 +0300, Kirill Tkhai wrote:
> > > From: Kirill Tkhai <tkhai@...ru>
> > > 
> > > Currently, there are no barriers, and skb->xxx update may become invisible on cpu2.
> > > In the below example var2 may point to intial_val0 instead of expected var1:
> > > 
> > > [cpu1]					[cpu2]
> > > skb->xxx = initial_val0;
> > > ...
> > > skb->xxx = var1;			skb = READ_ONCE(prev_skb->next);
> > > <no barrier>				<no barrier>
> > > WRITE_ONCE(prev_skb->next, skb);	var2 = skb->xxx;
> > > 
> > > This patch adds barriers and fixes the problem. Note, that __skb_peek() is not patched,
> > > since it's a lowlevel function, and a caller has to understand the things it does (and
> > > also __skb_peek() is used under queue lock in some places).
> > > 
> > > Signed-off-by: Kirill Tkhai <tkhai@...ru>
> > > ---
> > > Hi, David, Eric and other developers,
> > > 
> > > picking unix sockets code I found this problem, 
> > 
> > Could you please report exactly how/where the problem maifests (e.g.
> > the involved call paths/time sequence)?
> 
> I didn't get why call paths in the patch description are not enough for you. Please, explain
> what you want.

You mentioned the unix socket, so I expect to see something alike (I'm
totally making up the symbols lists just to give an example): 

CPU0					CPU1
unix_stream_read_generic()		unix_stream_sendmsg()
skb_peek()				skb_queue_tail(other->sk_receive_queue)

plus some wording on how the critical race is reached, if not
completely obvious.

>  
> > > and for me it looks like it exists. If there
> > > are arguments that everything is OK and it's expected, please, explain.
> > 
> > I don't see why such barriers are needed for the locked peek/tail
> > variants, as the spin_lock pair implies a full memory barrier.
> 
> This is for lockless skb_peek() calls and the patch is called in that way :). For locked skb_peek()
> this is not needed. 

But you are also unconditioanlly adding barriers to the locked
append/enqueue functions - which would possibly make sense only when
the latters are paired with lockless read access.

As Eric said, if any new barrier is needed, we want to apply it only
where needed, and not to every skb_queue*()/skb_peek() user, so very
likely a new helper (o a new pair of helpers) will be needed.

Thanks!

Paolo