[PATCH net 0/4] af_unix: Fix inq_len and scm_stat.nr_fds accounting leaks

[PATCH net 0/4] af_unix: Fix inq_len and scm_stat.nr_fds accounting leaks

[Siddharth Chintamaneni]

This series fixes receive-queue accounting leaks of u->inq_len and
u->scm_stat.nr_fds in AF_UNIX sockets, along with selftests:

- Patch 1 fixes a u->inq_len leak in manage_oob() when an unconsumed
  out-of-band (MSG_OOB) skb is skipped and dropped during a normal read.
- Patch 2 fixes u->scm_stat.nr_fds leaks when an OOB skb is skipped or
  freed in manage_oob(), unix_stream_recv_urg(), and
  unix_stream_read_skb().
- Patch 3 fixes a u->scm_stat.nr_fds leak in unix_dgram_disconnected()
  when purging queued skbs from sk->sk_receive_queue on disconnect.
- Patch 4 adds SIOCINQ and scm_fds (/proc/self/fdinfo/<fd>) checks to
  the msg_oob and scm_rights selftests.

Siddharth Chintamaneni (4):
  af_unix: Fix inq_len leak when skipping unread OOB skb.
  af_unix: Fix scm_stat.nr_fds leaks for OOB skb.
  af_unix: Fix scm_stat.nr_fds leak in unix_dgram_disconnected().
  selftest: af_unix: Test SIOCINQ and scm_fds with OOB and dgram
    disconnect.

 net/unix/af_unix.c                            |  69 ++++++-----
 tools/testing/selftests/net/af_unix/msg_oob.c |  12 +-
 .../selftests/net/af_unix/scm_rights.c        | 108 ++++++++++++++++++
 3 files changed, 161 insertions(+), 28 deletions(-)

-- 
2.43.0

[PATCH net 1/4] af_unix: Fix inq_len leak when skipping unread OOB skb.

[Siddharth Chintamaneni]

When queue_oob() enqueues an out-of-band skb onto a stream socket's
receive queue, it increments u->inq_len by 1 for the OOB byte.

If !sock_flag(sk, SOCK_URGINLINE) and the OOB byte has not been consumed
via MSG_OOB, a subsequent normal read in manage_oob() unlinks the
unconsumed u->oob_skb from sk_receive_queue and drops it via
kfree_skb_reason(unread_skb, SKB_DROP_REASON_UNIX_SKIP_OOB), but fails to
decrement u->inq_len.

As a result, unix_inq_len() (reported via ioctl(SIOCINQ), SCM_INQ, and
sock_diag) remains permanently elevated by 1 for each skipped OOB skb.

Decrement u->inq_len under sk_receive_queue.lock when unlinking
unread_skb in manage_oob().

Fixes: f4e1fb04c123 ("af_unix: Use cached value for SOCK_STREAM in unix_inq_len().")
Signed-off-by: Siddharth Chintamaneni <sidchintamaneni@gmail.com>
---
 net/unix/af_unix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 98fcfd7301f6..bfaab7c6d720 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2853,6 +2853,7 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sk_buff **last,
 		WRITE_ONCE(u->oob_skb, NULL);
 
 		if (!sock_flag(sk, SOCK_URGINLINE)) {
+			WRITE_ONCE(u->inq_len, u->inq_len - 1);
 			__skb_unlink(skb, &sk->sk_receive_queue);
 			unread_skb = skb;
 			skb = skb_peek(&sk->sk_receive_queue);
-- 
2.43.0

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

[Siddharth Chintamaneni]

When queue_oob() enqueues an out-of-band skb carrying SCM_RIGHTS file
descriptors, it accounts the inflight descriptors in u->scm_stat.nr_fds
via scm_stat_add().

However, the paths that later free or skip an OOB skb do not call
scm_stat_del():

1. In manage_oob(), when !sock_flag(sk, SOCK_URGINLINE) and an unconsumed
   u->oob_skb is unlinked as unread_skb and dropped via
   kfree_skb_reason().
2. In manage_oob(), when a consumed OOB skb (read_skb) is unlinked and
   freed via consume_skb().
3. In unix_stream_recv_urg(), when a consecutive MSG_OOB read unlinks and
   frees a previously consumed OOB skb (read_skb).
4. In unix_stream_read_skb() (SOCKMAP), when skb == u->oob_skb, the skb
   is unlinked and dropped via kfree_skb_reason() before reaching
   unix_orphan_scm().

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.

Call scm_stat_del() before freeing read_skb or unread_skb in
unix_stream_recv_urg(), manage_oob(), and unix_stream_read_skb().

Fixes: 2aab4b969002 ("af_unix: fix struct pid leaks in OOB support")
Fixes: 32ca245464e1 ("af_unix: Don't leave consecutive consumed OOB skbs.")
Fixes: 965dc93481d1 ("af_unix: Drop all SCM attributes for SOCKMAP.")
Signed-off-by: Siddharth Chintamaneni <sidchintamaneni@gmail.com>
---
 net/unix/af_unix.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index bfaab7c6d720..44c0a81dd423 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2807,7 +2807,10 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state)
 
 	mutex_unlock(&u->iolock);
 
-	consume_skb(read_skb);
+	if (read_skb) {
+		scm_stat_del(sk, read_skb);
+		consume_skb(read_skb);
+	}
 
 	if (chunk < 0)
 		return -EFAULT;
@@ -2867,8 +2870,14 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sk_buff **last,
 unlock:
 	spin_unlock(&sk->sk_receive_queue.lock);
 
-	consume_skb(read_skb);
-	kfree_skb_reason(unread_skb, SKB_DROP_REASON_UNIX_SKIP_OOB);
+	if (read_skb) {
+		scm_stat_del(sk, read_skb);
+		consume_skb(read_skb);
+	}
+	if (unread_skb) {
+		scm_stat_del(sk, unread_skb);
+		kfree_skb_reason(unread_skb, SKB_DROP_REASON_UNIX_SKIP_OOB);
+	}
 
 	return skb;
 }
@@ -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;
 	}
-- 
2.43.0

[PATCH net 3/4] af_unix: Fix scm_stat.nr_fds leak in unix_dgram_disconnected().

[Siddharth Chintamaneni]

When a connected SOCK_DGRAM socket disconnects (e.g. via connect() with
AF_UNSPEC), reconnects to a different peer, or observes a dead peer in
unix_dgram_sendmsg(), unix_dgram_disconnected() purges all unread
datagrams from sk->sk_receive_queue via skb_queue_purge_reason().

Because skb_queue_purge_reason() frees each skb without calling
scm_stat_del(sk, skb), any SCM_RIGHTS file descriptors attached to the
purged skbs remain permanently accounted in u->scm_stat.nr_fds (reported
as scm_fds in /proc/<pid>/fdinfo/<fd>) for the remaining lifetime of the
socket.

Move scm_stat_add() and scm_stat_del() above unix_dgram_disconnected()
and dequeue each skb in unix_dgram_disconnected() so scm_stat_del() is
called before kfree_skb_reason().

Fixes: 3c32da19a858 ("unix: Show number of pending scm files of receive queue in fdinfo")
Signed-off-by: Siddharth Chintamaneni <sidchintamaneni@gmail.com>
---
 net/unix/af_unix.c | 52 +++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 24 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 44c0a81dd423..47120d81aca2 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -609,6 +609,28 @@ static void unix_write_space(struct sock *sk)
 	rcu_read_unlock();
 }
 
+static void scm_stat_add(struct sock *sk, struct sk_buff *skb)
+{
+	struct scm_fp_list *fp = UNIXCB(skb).fp;
+	struct unix_sock *u = unix_sk(sk);
+
+	if (unlikely(fp && fp->count)) {
+		atomic_add(fp->count, &u->scm_stat.nr_fds);
+		unix_add_edges(fp, u);
+	}
+}
+
+static void scm_stat_del(struct sock *sk, struct sk_buff *skb)
+{
+	struct scm_fp_list *fp = UNIXCB(skb).fp;
+	struct unix_sock *u = unix_sk(sk);
+
+	if (unlikely(fp && fp->count)) {
+		atomic_sub(fp->count, &u->scm_stat.nr_fds);
+		unix_del_edges(fp);
+	}
+}
+
 /* When dgram socket disconnects (or changes its peer), we clear its receive
  * queue of packets arrived from previous peer. First, it allows to do
  * flow control based only on wmem_alloc; second, sk connected to peer
@@ -616,8 +638,12 @@ static void unix_write_space(struct sock *sk)
 static void unix_dgram_disconnected(struct sock *sk, struct sock *other)
 {
 	if (!skb_queue_empty(&sk->sk_receive_queue)) {
-		skb_queue_purge_reason(&sk->sk_receive_queue,
-				       SKB_DROP_REASON_UNIX_DISCONNECT);
+		struct sk_buff *skb;
+
+		while ((skb = skb_dequeue(&sk->sk_receive_queue)) != NULL) {
+			scm_stat_del(sk, skb);
+			kfree_skb_reason(skb, SKB_DROP_REASON_UNIX_DISCONNECT);
+		}
 
 		wake_up_interruptible_all(&unix_sk(sk)->peer_wait);
 
@@ -2058,28 +2084,6 @@ static bool unix_skb_scm_eq(const struct sock *sk, struct sk_buff *skb,
 	       unix_secdata_eq(scm, skb);
 }
 
-static void scm_stat_add(struct sock *sk, struct sk_buff *skb)
-{
-	struct scm_fp_list *fp = UNIXCB(skb).fp;
-	struct unix_sock *u = unix_sk(sk);
-
-	if (unlikely(fp && fp->count)) {
-		atomic_add(fp->count, &u->scm_stat.nr_fds);
-		unix_add_edges(fp, u);
-	}
-}
-
-static void scm_stat_del(struct sock *sk, struct sk_buff *skb)
-{
-	struct scm_fp_list *fp = UNIXCB(skb).fp;
-	struct unix_sock *u = unix_sk(sk);
-
-	if (unlikely(fp && fp->count)) {
-		atomic_sub(fp->count, &u->scm_stat.nr_fds);
-		unix_del_edges(fp);
-	}
-}
-
 static void unix_orphan_scm(struct sock *sk, struct sk_buff *skb)
 {
 	scm_stat_del(sk, skb);
-- 
2.43.0

[PATCH net 4/4] selftest: af_unix: Test SIOCINQ and scm_fds with OOB and dgram disconnect.

[Siddharth Chintamaneni]

Verify in msg_oob's __resetpair() that ioctl(SIOCINQ) reports 0 unread
bytes after draining the stream receive queue, and add oob_fdinfo and
dgram_disconnect_fdinfo tests in scm_rights.c verifying that
/proc/self/fdinfo/<fd> (scm_fds) returns to 0 when OOB skbs carrying
SCM_RIGHTS are skipped or freed and when a dgram socket disconnects with
queued SCM_RIGHTS skbs.

Signed-off-by: Siddharth Chintamaneni <sidchintamaneni@gmail.com>
---
 tools/testing/selftests/net/af_unix/msg_oob.c |  12 +-
 .../selftests/net/af_unix/scm_rights.c        | 108 ++++++++++++++++++
 2 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/af_unix/msg_oob.c b/tools/testing/selftests/net/af_unix/msg_oob.c
index f051d79f7a8e..7d5a25485048 100644
--- a/tools/testing/selftests/net/af_unix/msg_oob.c
+++ b/tools/testing/selftests/net/af_unix/msg_oob.c
@@ -5,6 +5,7 @@
 #include <string.h>
 #include <unistd.h>
 
+#include <linux/sockios.h>
 #include <netinet/in.h>
 #include <sys/epoll.h>
 #include <sys/ioctl.h>
@@ -336,8 +337,17 @@ static void __resetpair(struct __test_metadata *_metadata,
 {
 	int i;
 
-	for (i = 0; i < 2; i++)
+	for (i = 0; i < 2; i++) {
+		if (!reset) {
+			int inq, ret;
+
+			ret = ioctl(self->fd[i * 2 + 1], SIOCINQ, &inq);
+			ASSERT_EQ(ret, 0);
+			ASSERT_EQ(inq, 0);
+		}
+
 		close(self->fd[i * 2 + 1]);
+	}
 
 	__recvpair(_metadata, self, "", reset ? -ECONNRESET : 0, 1,
 		   variant->peek ? MSG_PEEK : 0, true);
diff --git a/tools/testing/selftests/net/af_unix/scm_rights.c b/tools/testing/selftests/net/af_unix/scm_rights.c
index c165f250220a..38a125d9f6e4 100644
--- a/tools/testing/selftests/net/af_unix/scm_rights.c
+++ b/tools/testing/selftests/net/af_unix/scm_rights.c
@@ -378,6 +378,36 @@ TEST_F(scm_rights, backtrack_from_scc)
 	close_sockets(10);
 }
 
+static int count_scm_fds(struct __test_metadata *_metadata, int fd)
+{
+	char path[64], *line = NULL;
+	int scm_fds = -1, ret;
+	size_t unused;
+	FILE *f;
+
+	snprintf(path, sizeof(path), "/proc/self/fdinfo/%d", fd);
+
+	f = fopen(path, "r");
+	ASSERT_NE(NULL, f);
+
+	while (getline(&line, &unused, f) != -1) {
+		if (strncmp(line, "scm_fds:", 8))
+			continue;
+
+		ret = sscanf(line + 8, "%d", &scm_fds);
+		ASSERT_EQ(1, ret);
+
+		break;
+	}
+
+	free(line);
+
+	ret = fclose(f);
+	ASSERT_EQ(0, ret);
+
+	return scm_fds;
+}
+
 TEST_F(scm_rights, mixed_lowpoint)
 {
 	create_sockets(6);
@@ -395,4 +425,82 @@ TEST_F(scm_rights, mixed_lowpoint)
 	close_sockets(6);
 }
 
+TEST_F(scm_rights, oob_fdinfo)
+{
+	int recv_fd, ret;
+	char buf[2];
+
+	if (!(variant->flags & MSG_OOB) || variant->disabled)
+		return;
+
+	create_sockets(2);
+
+	if (variant->test_listener) {
+		recv_fd = accept(self->fd[0], NULL, NULL);
+		ASSERT_LE(0, recv_fd);
+	} else {
+		recv_fd = self->fd[0];
+	}
+
+	/* 1. Unconsumed OOB skb skipped by normal recv() in manage_oob() */
+	send_fd(0, 0);
+	ASSERT_EQ(2, count_scm_fds(_metadata, recv_fd));
+	ret = send(self->fd[1], "a", 1, 0);
+	ASSERT_EQ(1, ret);
+	ret = recv(recv_fd, buf, 1, 0);
+	ASSERT_EQ(1, ret);
+	ASSERT_EQ(0, count_scm_fds(_metadata, recv_fd));
+
+	/* 2. Consumed OOB skb freed by normal recv() in manage_oob() */
+	send_fd(0, 0);
+	ASSERT_EQ(2, count_scm_fds(_metadata, recv_fd));
+	ret = recv(recv_fd, buf, 1, MSG_OOB);
+	ASSERT_EQ(1, ret);
+	ret = send(self->fd[1], "a", 1, 0);
+	ASSERT_EQ(1, ret);
+	ret = recv(recv_fd, buf, 1, 0);
+	ASSERT_EQ(1, ret);
+	ASSERT_EQ(0, count_scm_fds(_metadata, recv_fd));
+
+	/* 3. Consumed OOB skb freed by consecutive recv(MSG_OOB) */
+	send_fd(0, 0);
+	ASSERT_EQ(2, count_scm_fds(_metadata, recv_fd));
+	ret = recv(recv_fd, buf, 1, MSG_OOB);
+	ASSERT_EQ(1, ret);
+	ret = send(self->fd[1], "y", 1, MSG_OOB);
+	ASSERT_EQ(1, ret);
+	ret = recv(recv_fd, buf, 1, MSG_OOB);
+	ASSERT_EQ(1, ret);
+	ASSERT_EQ(0, count_scm_fds(_metadata, recv_fd));
+
+	if (variant->test_listener) {
+		ret = close(recv_fd);
+		ASSERT_EQ(0, ret);
+	}
+
+	close_sockets(2);
+}
+
+TEST_F(scm_rights, dgram_disconnect_fdinfo)
+{
+	struct sockaddr addr = {
+		.sa_family = AF_UNSPEC,
+	};
+	int ret;
+
+	if (variant->type != SOCK_DGRAM || variant->disabled)
+		return;
+
+	create_sockets(2);
+
+	send_fd(0, 0);
+	ASSERT_EQ(2, count_scm_fds(_metadata, self->fd[0]));
+
+	ret = connect(self->fd[0], &addr, sizeof(addr.sa_family));
+	ASSERT_EQ(0, ret);
+	ASSERT_EQ(0, count_scm_fds(_metadata, self->fd[0]));
+
+	close_sockets(2);
+}
+
 TEST_HARNESS_MAIN
-- 
2.43.0

Re: [PATCH net 0/4] af_unix: Fix inq_len and scm_stat.nr_fds accounting leaks

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.