netdev - Re: [PATCH bpf-next v1] bpf, sockmap: Fix psock incorrectly pointing to sk

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] [day] [month] [year] [list]

Message-ID: <9e1e9066f16378f810304ad60b972afe7e4d421a@linux.dev>
Date: Thu, 29 May 2025 05:22:00 +0000
From: "Jiayuan Chen" <jiayuan.chen@...ux.dev>
To: "John Fastabend" <john.fastabend@...il.com>
Cc: bpf@...r.kernel.org, "Jakub Sitnicki" <jakub@...udflare.com>, "David S.
 Miller" <davem@...emloft.net>, "Eric Dumazet" <edumazet@...gle.com>,
 "Jakub Kicinski" <kuba@...nel.org>, "Paolo Abeni" <pabeni@...hat.com>,
 "Simon Horman" <horms@...nel.org>, "Alexei Starovoitov" <ast@...nel.org>,
 "Daniel Borkmann" <daniel@...earbox.net>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v1] bpf, sockmap: Fix psock incorrectly pointing
 to sk

May 29, 2025 at 07:46, "John Fastabend" <john.fastabend@...il.com> wrote:

> 
> On 2025-05-24 00:22:19, Jiayuan Chen wrote:
> 
> > 
> > We observed an issue from the latest selftest: sockmap_redir where
> > 
> >  sk_psock(psock->sk) != psock in the backlog. The root cause is the special
> > 
> >  behavior in sockmap_redir - it frequently performs map_update() and
> > 
> >  map_delete() on the same socket. During map_update(), we create a new
> > 
> >  psock and during map_delete(), we eventually free the psock via rcu_work
> > 
> >  in sk_psock_drop(). However, pending workqueues might still exist and not
> > 
> >  be processed yet. If users immediately perform another map_update(), a new
> > 
> >  psock will be allocated for the same sk, resulting in two psocks pointing
> > 
> >  to the same sk.
> > 
> >  
> > 
> >  When the pending workqueue is later triggered, it uses the old psock to
> > 
> >  access sk for I/O operations, which is incorrect.
> > 
> >  
> > 
> >  Timing Diagram:
> > 
> >  
> > 
> >  cpu0 cpu1
> > 
> >  
> > 
> >  map_update(sk):
> > 
> >  sk->psock = psock1
> > 
> >  psock1->sk = sk
> > 
> >  map_delete(sk):
> > 
> >  rcu_work_free(psock1)
> > 
> >  
> > 
> >  map_update(sk):
> > 
> >  sk->psock = psock2
> > 
> >  psock2->sk = sk
> > 
> >  workqueue:
> > 
> >  wakeup with psock1, but the sk of psock1
> > 
> >  doesn't belong to psock1
> > 
> >  rcu_handler:
> > 
> >  clean psock1
> > 
> >  free(psock1)
> > 
> >  
> > 
> >  Previously, we used reference counting to address the concurrency issue
> > 
> >  between backlog and sock_map_close(). This logic remains necessary as it
> > 
> >  prevents the sk from being freed while processing the backlog. But this
> > 
> >  patch prevents pending backlogs from using a psock after it has been
> > 
> >  freed.
> > 
> 
> Nit, its not that psock would be freed because we do have the
> 
> cancel_delayed_work_sync() before the kfree(psock). But this
> 
> is not a good state with two psocks referenceing the same sk.
> 

BTW, did we miss ingress_lock while processing ingress_skb in backlog?
will we have the concurrency issue when skb was appended into ingress_skb
in sk_psock_skb_redirect().