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

["Jiayuan Chen" <jiayuan.chen@linux.dev>]

May 29, 2025 at 07:46, "John Fastabend" <john.fastabend@gmail.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().