Re: [PATCH net 2/4] af_unix: Fix scm_stat.nr_fds leaks for OOB skb.

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message for "af_unix: Fix scm_stat.nr_fds leaks for
  OOB skb." gives the wrong reason for the leak.

Pre-existing issues:
- [Medium] unix_dgram_disconnected() (net/unix/af_unix.c:616) purges a
  datagram socket's receive queue with skb_queue_purge_reason() and…

--

af_unix: Fix scm_stat.nr_fds leaks for OOB skb.

This commit adds scm_stat_del() calls on the paths that free or skip an
OOB skb. Those paths are in unix_stream_recv_urg(), manage_oob() and
unix_stream_read_skb(). Without the calls, u->scm_stat.nr_fds stays
elevated after an OOB skb carrying SCM_RIGHTS is dropped.

> Because unix_destruct_scm() has no reference to the receiving socket when
> passed descriptors are not AF_UNIX sockets, u->scm_stat.nr_fds (reported
> as scm_fds in /proc/<pid>/fdinfo/<fd>) remains permanently elevated.

[Severity: Low]
Is this explanation accurate? It reads as if the leak only happens when
the passed descriptors are not AF_UNIX sockets.

The destructor path never decrements nr_fds for any fd type:

unix_destruct_scm()->unix_detach_fds()->unix_destroy_fpl()->unix_del_edges()

net/unix/garbage.c:unix_del_edges() {
    ...
	if (!fpl->dead) {
		receiver = fpl->edges[0].successor;
		receiver->scm_stat.nr_unix_fds -= fpl->count_unix;
	}
out:
	WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight - fpl->count);
    ...
}

The destructor can reach the receiver through the AF_UNIX edges, but it
only updates nr_unix_fds and unix_inflight.

The only decrement of nr_fds is the atomic_sub() in scm_stat_del(). So
these OOB free paths leaked nr_fds for AF_UNIX descriptors too.

The oob_fdinfo selftest in this series sends AF_UNIX socket fds
(self->fd[inflight * 2]) over MSG_OOB. It expects scm_fds to go back to 0
only with this fix applied.

Could the commit message describe the cause as scm_stat_del() being the
only place nr_fds is decremented, whatever the fd type?

> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index bfaab7c6d7203..44c0a81dd423a 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c

[ ... ]

> @@ -2906,6 +2915,7 @@ static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
>  		spin_unlock(&queue->lock);
>  		mutex_unlock(&u->iolock);
>  
> +		scm_stat_del(sk, skb);
>  		kfree_skb_reason(skb, SKB_DROP_REASON_UNIX_SKIP_OOB);
>  		return -EAGAIN;
>  	}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. Is
there a similar nr_fds leak in unix_dgram_disconnected()?

net/unix/af_unix.c:unix_dgram_disconnected() {
	if (!skb_queue_empty(&sk->sk_receive_queue)) {
		skb_queue_purge_reason(&sk->sk_receive_queue,
				       SKB_DROP_REASON_UNIX_DISCONNECT);
    ...
}

Each skb on that queue was counted by scm_stat_add() at send time. The
queue is purged without scm_stat_del(), and the socket stays in use. This
happens when unix_dgram_connect() gives it a new peer (or AF_UNSPEC), and
also from unix_dgram_sendmsg().

Sending SCM_RIGHTS to a connected SOCK_DGRAM socket and then calling
connect() on it before reading would leave scm_fds in fdinfo too high.

This is fixed later in the same series by "af_unix: Fix scm_stat.nr_fds
leak in unix_dgram_disconnected()". That patch dequeues each skb and calls
scm_stat_del() before kfree_skb_reason(), and the dgram_disconnect_fdinfo
selftest covers it.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106051905.218638-1-sidchintamaneni%40gmail.com