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

Jiayuan Chen
2025-05-29T05:22:14Z

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().