Re: [PATCH stable 5.15/6.1/6.6] af_unix: Clear oob_skb in scan_inflight().

[Lee Jones <lee@kernel.org>]

On Mon, 03 Mar 2025, Kuniyuki Iwashima wrote:

> Embryo socket is not queued in gc_candidates, so we can't drop
> a reference held by its oob_skb.
> 
> Let's say we create listener and embryo sockets, send the
> listener's fd to the embryo as OOB data, and close() them
> without recv()ing the OOB data.
> 
> There is a self-reference cycle like
> 
>   listener -> embryo.oob_skb -> listener
> 
> , so this must be cleaned up by GC.  Otherwise, the listener's
> refcnt is not released and sockets are leaked:
> 
>   # unshare -n
>   # cat /proc/net/protocols | grep UNIX-STREAM
>   UNIX-STREAM 1024      0      -1   NI       0   yes  kernel ...
> 
>   # python3
>   >>> from array import array
>   >>> from socket import *
>   >>>
>   >>> s = socket(AF_UNIX, SOCK_STREAM)
>   >>> s.bind('\0test\0')
>   >>> s.listen()
>   >>>
>   >>> c = socket(AF_UNIX, SOCK_STREAM)
>   >>> c.connect(s.getsockname())
>   >>> c.sendmsg([b'x'], [(SOL_SOCKET, SCM_RIGHTS, array('i', [s.fileno()]))], MSG_OOB)
>   1
>   >>> quit()
> 
>   # cat /proc/net/protocols | grep UNIX-STREAM
>   UNIX-STREAM 1024      3      -1   NI       0   yes  kernel ...
>                         ^^^
>                         3 sockets still in use after FDs are close()d
> 
> Let's drop the embryo socket's oob_skb ref in scan_inflight().
> 
> This also fixes a racy access to oob_skb that commit 9841991a446c
> ("af_unix: Update unix_sk(sk)->oob_skb under sk_receive_queue
> lock.") fixed for the new Tarjan's algo-based GC.
> 
> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
> Reported-by: Lei Lu <llfamsec@gmail.com>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
> This has no upstream commit because I replaced the entire GC in
> 6.10 and the new GC does not have this bug, and this fix is only
> applicable to the old GC (<= 6.9), thus for 5.15/6.1/6.6.
> ---
> ---
>  net/unix/garbage.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

Tested-by: Lee Jones <lee@kernel.org>

> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> index 2a758531e102..b3fbdf129944 100644
> --- a/net/unix/garbage.c
> +++ b/net/unix/garbage.c
> @@ -102,13 +102,14 @@ static void scan_inflight(struct sock *x, void (*func)(struct unix_sock *),
>  			/* Process the descriptors of this socket */
>  			int nfd = UNIXCB(skb).fp->count;
>  			struct file **fp = UNIXCB(skb).fp->fp;
> +			struct unix_sock *u;
>  
>  			while (nfd--) {
>  				/* Get the socket the fd matches if it indeed does so */
>  				struct sock *sk = unix_get_socket(*fp++);
>  
>  				if (sk) {
> -					struct unix_sock *u = unix_sk(sk);
> +					u = unix_sk(sk);
>  
>  					/* Ignore non-candidates, they could
>  					 * have been added to the queues after
> @@ -122,6 +123,13 @@ static void scan_inflight(struct sock *x, void (*func)(struct unix_sock *),
>  				}
>  			}
>  			if (hit && hitlist != NULL) {
> +#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
> +				u = unix_sk(x);
> +				if (u->oob_skb) {
> +					WARN_ON_ONCE(skb_unref(u->oob_skb));
> +					u->oob_skb = NULL;
> +				}
> +#endif
>  				__skb_unlink(skb, &x->sk_receive_queue);
>  				__skb_queue_tail(hitlist, skb);
>  			}
> @@ -299,17 +307,9 @@ void unix_gc(void)
>  	 * which are creating the cycle(s).
>  	 */
>  	skb_queue_head_init(&hitlist);
> -	list_for_each_entry(u, &gc_candidates, link) {
> +	list_for_each_entry(u, &gc_candidates, link)
>  		scan_children(&u->sk, inc_inflight, &hitlist);
>  
> -#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
> -		if (u->oob_skb) {
> -			kfree_skb(u->oob_skb);
> -			u->oob_skb = NULL;
> -		}
> -#endif
> -	}
> -
>  	/* not_cycle_list contains those sockets which do not make up a
>  	 * cycle.  Restore these to the inflight list.
>  	 */
> -- 
> 2.39.5 (Apple Git-154)
> 

-- 
Lee Jones [李琼斯]