linux-kernel - 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: <20250605142448.3llri3w7wbclfxwc@gmail.com>
Date: Thu, 5 Jun 2025 07:24:48 -0700
From: John Fastabend <john.fastabend@...il.com>
To: Jiayuan Chen <jiayuan.chen@...ux.dev>
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

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

[...]

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