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

John Fastabend
2025-06-05T14:25:07Z

On 2025-05-29 02:42:14, Jiayuan Chen wrote:
> 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.

[...]

> >
> > Can we add this to sk_psock_stop where we have the TX_ENABLED bit
> >
> > cleared.
>
>
>
> Thanks, I just add SK_PSOCK_TX_ENABLED checking at the start of sk_psock_backlog().
> Every works fine, and truly no more flag needed !
>
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 34c51eb1a14f..83c78379932e 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -656,6 +656,13 @@ static void sk_psock_backlog(struct work_struct *work)
> bool ingress;
> int ret;
>
> + /* If sk is quickly removed from the map and then added back, the old
> + * psock should not be scheduled, because there are now two psocks
> + * pointing to the same sk.
> + */
> + if (!sk_psock_test_state(psock, SK_PSOCK_TX_ENABLED))
> + return;
> +
> /* Increment the psock refcnt to synchronize with close(fd) path in
> * sock_map_close(), ensuring we wait for backlog thread completion
> * before sk_socket freed. If refcnt increment fails, it indicates
>

Thanks. Please submit an official patch so we can get it merged.