[PATCH v2 net-next 00/11] af_unix: Set skb drop reason in every kfree_skb() path.

[PATCH v2 net-next 00/11] af_unix: Set skb drop reason in every kfree_skb() path.

[Kuniyuki Iwashima]

There is a potential user for skb drop reason for AF_UNIX.

This series sets skb drop reason for every kfree_skb() path
in AF_UNIX code.

Link: https://lore.kernel.org/netdev/CAAf2ycmZHti95WaBR3s+L5Epm1q7sXmvZ-EqCK=-oZj=45tOwQ@mail.gmail.com/


Changes:
  v2:
    * Drop the old patch 6 to silence false-positive uninit warning

  v1: https://lore.kernel.org/netdev/20250110092641.85905-1-kuniyu@amazon.com/


Kuniyuki Iwashima (11):
  net: dropreason: Gather SOCKET_ drop reasons.
  af_unix: Set drop reason in unix_release_sock().
  af_unix: Set drop reason in unix_sock_destructor().
  af_unix: Set drop reason in __unix_gc().
  af_unix: Set drop reason in unix_stream_connect().
  af_unix: Set drop reason in unix_stream_sendmsg().
  af_unix: Set drop reason in queue_oob().
  af_unix: Set drop reason in manage_oob().
  af_unix: Set drop reason in unix_stream_read_skb().
  af_unix: Set drop reason in unix_dgram_disconnected().
  af_unix: Set drop reason in unix_dgram_sendmsg().

 include/net/dropreason-core.h |  46 ++++++++--
 net/unix/af_unix.c            | 153 +++++++++++++++++++++++++---------
 net/unix/garbage.c            |   2 +-
 3 files changed, 154 insertions(+), 47 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 01/11] net: dropreason: Gather SOCKET_ drop reasons.

[Kuniyuki Iwashima]

The following patches add new drop reasons starting with
the SOCKET_ prefix.

Let's gather the existing SOCKET_ reasons.

Note that the order is not part of uAPI.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/dropreason-core.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 3a6602f37978..efeae9f0f956 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -6,9 +6,10 @@
 #define DEFINE_DROP_REASON(FN, FNe)	\
 	FN(NOT_SPECIFIED)		\
 	FN(NO_SOCKET)			\
+	FN(SOCKET_FILTER)		\
+	FN(SOCKET_RCVBUFF)		\
 	FN(PKT_TOO_SMALL)		\
 	FN(TCP_CSUM)			\
-	FN(SOCKET_FILTER)		\
 	FN(UDP_CSUM)			\
 	FN(NETFILTER_DROP)		\
 	FN(OTHERHOST)			\
@@ -18,7 +19,6 @@
 	FN(UNICAST_IN_L2_MULTICAST)	\
 	FN(XFRM_POLICY)			\
 	FN(IP_NOPROTO)			\
-	FN(SOCKET_RCVBUFF)		\
 	FN(PROTO_MEM)			\
 	FN(TCP_AUTH_HDR)		\
 	FN(TCP_MD5NOTFOUND)		\
@@ -137,12 +137,14 @@ enum skb_drop_reason {
 	 * 3) no valid child socket during 3WHS process
 	 */
 	SKB_DROP_REASON_NO_SOCKET,
+	/** @SKB_DROP_REASON_SOCKET_FILTER: dropped by socket filter */
+	SKB_DROP_REASON_SOCKET_FILTER,
+	/** @SKB_DROP_REASON_SOCKET_RCVBUFF: socket receive buff is full */
+	SKB_DROP_REASON_SOCKET_RCVBUFF,
 	/** @SKB_DROP_REASON_PKT_TOO_SMALL: packet size is too small */
 	SKB_DROP_REASON_PKT_TOO_SMALL,
 	/** @SKB_DROP_REASON_TCP_CSUM: TCP checksum error */
 	SKB_DROP_REASON_TCP_CSUM,
-	/** @SKB_DROP_REASON_SOCKET_FILTER: dropped by socket filter */
-	SKB_DROP_REASON_SOCKET_FILTER,
 	/** @SKB_DROP_REASON_UDP_CSUM: UDP checksum error */
 	SKB_DROP_REASON_UDP_CSUM,
 	/** @SKB_DROP_REASON_NETFILTER_DROP: dropped by netfilter */
@@ -173,8 +175,6 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_XFRM_POLICY,
 	/** @SKB_DROP_REASON_IP_NOPROTO: no support for IP protocol */
 	SKB_DROP_REASON_IP_NOPROTO,
-	/** @SKB_DROP_REASON_SOCKET_RCVBUFF: socket receive buff is full */
-	SKB_DROP_REASON_SOCKET_RCVBUFF,
 	/**
 	 * @SKB_DROP_REASON_PROTO_MEM: proto memory limitation, such as
 	 * udp packet drop out of udp_memory_allocated.
-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 02/11] af_unix: Set drop reason in unix_release_sock().

[Kuniyuki Iwashima]

unix_release_sock() is called when the last refcnt of struct file
is released.

Let's define a new drop reason SKB_DROP_REASON_SOCKET_CLOSE and
set it for kfree_skb() in unix_release_sock().

  # echo 1 > /sys/kernel/tracing/events/skb/kfree_skb/enable

  # python3
  >>> from socket import *
  >>> s1, s2 = socketpair(AF_UNIX)
  >>> s1.send(b'hello world')
  >>> s2.close()

  # cat /sys/kernel/tracing/trace_pipe
  ...
     python3-280 ... kfree_skb: ... protocol=0 location=unix_release_sock+0x260/0x420 reason: SOCKET_CLOSE

To be precise, unix_release_sock() is also called for a new child
socket in unix_stream_connect() when something fails, but the new
sk does not have skb in the recv queue then and no event is logged.

Note that only tcp_inbound_ao_hash() uses a similar drop reason,
SKB_DROP_REASON_TCP_CLOSE, and this can be generalised later.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/dropreason-core.h | 3 +++
 net/unix/af_unix.c            | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index efeae9f0f956..8823de6539d1 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -6,6 +6,7 @@
 #define DEFINE_DROP_REASON(FN, FNe)	\
 	FN(NOT_SPECIFIED)		\
 	FN(NO_SOCKET)			\
+	FN(SOCKET_CLOSE)		\
 	FN(SOCKET_FILTER)		\
 	FN(SOCKET_RCVBUFF)		\
 	FN(PKT_TOO_SMALL)		\
@@ -137,6 +138,8 @@ enum skb_drop_reason {
 	 * 3) no valid child socket during 3WHS process
 	 */
 	SKB_DROP_REASON_NO_SOCKET,
+	/** @SKB_DROP_REASON_SOCKET_CLOSE: socket is close()d */
+	SKB_DROP_REASON_SOCKET_CLOSE,
 	/** @SKB_DROP_REASON_SOCKET_FILTER: dropped by socket filter */
 	SKB_DROP_REASON_SOCKET_FILTER,
 	/** @SKB_DROP_REASON_SOCKET_RCVBUFF: socket receive buff is full */
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 8f2b605ce5b3..a05d25cc5545 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -715,8 +715,8 @@ static void unix_release_sock(struct sock *sk, int embrion)
 		if (state == TCP_LISTEN)
 			unix_release_sock(skb->sk, 1);
 
-		/* passed fds are erased in the kfree_skb hook	      */
-		kfree_skb(skb);
+		/* passed fds are erased in the kfree_skb hook */
+		kfree_skb_reason(skb, SKB_DROP_REASON_SOCKET_CLOSE);
 	}
 
 	if (path.dentry)
-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 03/11] af_unix: Set drop reason in unix_sock_destructor().

[Kuniyuki Iwashima]

unix_sock_destructor() is called as sk->sk_destruct() just before
the socket is actually freed.

Let's use SKB_DROP_REASON_SOCKET_CLOSE for skb_queue_purge().

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index a05d25cc5545..41b99984008a 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -640,7 +640,7 @@ static void unix_sock_destructor(struct sock *sk)
 {
 	struct unix_sock *u = unix_sk(sk);
 
-	skb_queue_purge(&sk->sk_receive_queue);
+	skb_queue_purge_reason(&sk->sk_receive_queue, SKB_DROP_REASON_SOCKET_CLOSE);
 
 	DEBUG_NET_WARN_ON_ONCE(refcount_read(&sk->sk_wmem_alloc));
 	DEBUG_NET_WARN_ON_ONCE(!sk_unhashed(sk));
-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 04/11] af_unix: Set drop reason in __unix_gc().

[Kuniyuki Iwashima]

Inflight file descriptors by SCM_RIGHTS hold references to the
struct file.

AF_UNIX sockets could hold references to each other, forming
reference cycles.

Once such sockets are close()d without the fd recv()ed, they
will be unaccessible from userspace but remain in kernel.

__unix_gc() garbage-collects skb with the dead file descriptors
and frees them by __skb_queue_purge().

Let's set SKB_DROP_REASON_SOCKET_CLOSE there.

  # echo 1 > /sys/kernel/tracing/events/skb/kfree_skb/enable

  # python3
  >>> from socket import *
  >>> from array import array
  >>>
  >>> # Create a reference cycle
  >>> s1 = socket(AF_UNIX, SOCK_DGRAM)
  >>> s1.bind('')
  >>> s1.sendmsg([b"nop"], [(SOL_SOCKET, SCM_RIGHTS, array("i", [s1.fileno()]))], 0, s1.getsockname())
  >>> s1.close()
  >>>
  >>> # Trigger GC
  >>> s2 = socket(AF_UNIX)
  >>> s2.close()

  # cat /sys/kernel/tracing/trace_pipe
  ...
     kworker/u16:2-42 ... kfree_skb: ... location=__unix_gc+0x4ad/0x580 reason: SOCKET_CLOSE

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/garbage.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index 0068e758be4d..9848b7b78701 100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -573,7 +573,7 @@ static void __unix_gc(struct work_struct *work)
 			UNIXCB(skb).fp->dead = true;
 	}
 
-	__skb_queue_purge(&hitlist);
+	__skb_queue_purge_reason(&hitlist, SKB_DROP_REASON_SOCKET_CLOSE);
 skip_gc:
 	WRITE_ONCE(gc_in_progress, false);
 }
-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 05/11] af_unix: Set drop reason in unix_stream_connect().

[Kuniyuki Iwashima]

connect() to a SOCK_STREAM socket could fail for various reasons.

Let's set drop reasons respectively:

  * NO_SOCKET      : No listening socket found
  * RCV_SHUTDOWN   : The listening socket called shutdown(SHUT_RD)
  * SOCKET_RCVBUFF : The listening socket's accept queue is full
  * INVALID_STATE  : The client is in TCP_ESTABLISHED or TCP_LISTEN
  * SECURITY_HOOK  : LSM refused connect()

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/dropreason-core.h |  6 ++++++
 net/unix/af_unix.c            | 22 ++++++++++++++++++----
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 8823de6539d1..1b5e962f7f33 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -8,7 +8,9 @@
 	FN(NO_SOCKET)			\
 	FN(SOCKET_CLOSE)		\
 	FN(SOCKET_FILTER)		\
+	FN(SOCKET_INVALID_STATE)	\
 	FN(SOCKET_RCVBUFF)		\
+	FN(SOCKET_RCV_SHUTDOWN)		\
 	FN(PKT_TOO_SMALL)		\
 	FN(TCP_CSUM)			\
 	FN(UDP_CSUM)			\
@@ -142,8 +144,12 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_SOCKET_CLOSE,
 	/** @SKB_DROP_REASON_SOCKET_FILTER: dropped by socket filter */
 	SKB_DROP_REASON_SOCKET_FILTER,
+	/** @SKB_DROP_REASON_SOCKET_INVALID_STATE: sk->sk_state is invalid. */
+	SKB_DROP_REASON_SOCKET_INVALID_STATE,
 	/** @SKB_DROP_REASON_SOCKET_RCVBUFF: socket receive buff is full */
 	SKB_DROP_REASON_SOCKET_RCVBUFF,
+	/** @SKB_DROP_REASON_SOCKET_RCV_SHUTDOWN: socket is shutdown(SHUT_RD) */
+	SKB_DROP_REASON_SOCKET_RCV_SHUTDOWN,
 	/** @SKB_DROP_REASON_PKT_TOO_SMALL: packet size is too small */
 	SKB_DROP_REASON_PKT_TOO_SMALL,
 	/** @SKB_DROP_REASON_TCP_CSUM: TCP checksum error */
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 41b99984008a..b190ea8b8e9d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1534,6 +1534,7 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	struct sock *sk = sock->sk, *newsk = NULL, *other = NULL;
 	struct unix_sock *u = unix_sk(sk), *newu, *otheru;
 	struct net *net = sock_net(sk);
+	enum skb_drop_reason reason;
 	struct sk_buff *skb = NULL;
 	unsigned char state;
 	long timeo;
@@ -1581,6 +1582,7 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	other = unix_find_other(net, sunaddr, addr_len, sk->sk_type);
 	if (IS_ERR(other)) {
 		err = PTR_ERR(other);
+		reason = SKB_DROP_REASON_NO_SOCKET;
 		goto out_free_skb;
 	}
 
@@ -1593,15 +1595,22 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 		goto restart;
 	}
 
-	if (other->sk_state != TCP_LISTEN ||
-	    other->sk_shutdown & RCV_SHUTDOWN) {
+	if (other->sk_state != TCP_LISTEN) {
 		err = -ECONNREFUSED;
+		reason = SKB_DROP_REASON_NO_SOCKET;
+		goto out_unlock;
+	}
+
+	if (other->sk_shutdown & RCV_SHUTDOWN) {
+		err = -ECONNREFUSED;
+		reason = SKB_DROP_REASON_SOCKET_RCV_SHUTDOWN;
 		goto out_unlock;
 	}
 
 	if (unix_recvq_full_lockless(other)) {
 		if (!timeo) {
 			err = -EAGAIN;
+			reason = SKB_DROP_REASON_SOCKET_RCVBUFF;
 			goto out_unlock;
 		}
 
@@ -1609,8 +1618,10 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 		sock_put(other);
 
 		err = sock_intr_errno(timeo);
-		if (signal_pending(current))
+		if (signal_pending(current)) {
+			reason = SKB_DROP_REASON_SOCKET_RCVBUFF;
 			goto out_free_skb;
+		}
 
 		goto restart;
 	}
@@ -1621,6 +1632,7 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	state = READ_ONCE(sk->sk_state);
 	if (unlikely(state != TCP_CLOSE)) {
 		err = state == TCP_ESTABLISHED ? -EISCONN : -EINVAL;
+		reason = SKB_DROP_REASON_SOCKET_INVALID_STATE;
 		goto out_unlock;
 	}
 
@@ -1629,12 +1641,14 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	if (unlikely(sk->sk_state != TCP_CLOSE)) {
 		err = sk->sk_state == TCP_ESTABLISHED ? -EISCONN : -EINVAL;
 		unix_state_unlock(sk);
+		reason = SKB_DROP_REASON_SOCKET_INVALID_STATE;
 		goto out_unlock;
 	}
 
 	err = security_unix_stream_connect(sk, other, newsk);
 	if (err) {
 		unix_state_unlock(sk);
+		reason = SKB_DROP_REASON_SECURITY_HOOK;
 		goto out_unlock;
 	}
 
@@ -1699,7 +1713,7 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	unix_state_unlock(other);
 	sock_put(other);
 out_free_skb:
-	kfree_skb(skb);
+	kfree_skb_reason(skb, reason);
 out_free_sk:
 	unix_release_sock(newsk, 0);
 out:
-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 06/11] af_unix: Set drop reason in unix_stream_sendmsg().

[Kuniyuki Iwashima]

sendmsg() to a SOCK_STREAM socket could fail for various reasons.

Let's set drop reasons respectively.

  * NOMEM                  : Failed to allocate SCM_RIGHTS-related structs
  * UNIX_INFLIGHT_FD_LIMIT : The number of inflight fd reached RLIMIT_NOFILE
  * SKB_UCOPY_FAULT        : Failed to copy data from iov_iter to skb
  * SOCKET_CLOSE           : The peer socket was close()d
  * SOCKET_RCV_SHUTDOWN    : The peer socket called shutdown(SHUT_RD)

unix_scm_err_to_reason() will be reused in queue_oob() and
unix_dgram_sendmsg().

While at it, size and data_len are moved to the while loop scope.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/dropreason-core.h |  6 +++++
 net/unix/af_unix.c            | 50 +++++++++++++++++++++++++++--------
 2 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 1b5e962f7f33..dea6bbe3ceaa 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -11,6 +11,7 @@
 	FN(SOCKET_INVALID_STATE)	\
 	FN(SOCKET_RCVBUFF)		\
 	FN(SOCKET_RCV_SHUTDOWN)		\
+	FN(UNIX_INFLIGHT_FD_LIMIT)	\
 	FN(PKT_TOO_SMALL)		\
 	FN(TCP_CSUM)			\
 	FN(UDP_CSUM)			\
@@ -150,6 +151,11 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_SOCKET_RCVBUFF,
 	/** @SKB_DROP_REASON_SOCKET_RCV_SHUTDOWN: socket is shutdown(SHUT_RD) */
 	SKB_DROP_REASON_SOCKET_RCV_SHUTDOWN,
+	/**
+	 * @SKB_DROP_REASON_UNIX_INFLIGHT_FD_LIMIT: too many file descriptors
+	 * are passed via SCM_RIGHTS but not yet received, reaching RLIMIT_NOFILE.
+	 */
+	SKB_DROP_REASON_UNIX_INFLIGHT_FD_LIMIT,
 	/** @SKB_DROP_REASON_PKT_TOO_SMALL: packet size is too small */
 	SKB_DROP_REASON_PKT_TOO_SMALL,
 	/** @SKB_DROP_REASON_TCP_CSUM: TCP checksum error */
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index b190ea8b8e9d..8c8d8fc3cb94 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1907,6 +1907,22 @@ static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool sen
 	return err;
 }
 
+static enum skb_drop_reason unix_scm_err_to_reason(int err)
+{
+	switch (err) {
+	case -ENOMEM:
+		return SKB_DROP_REASON_NOMEM;
+	case -ETOOMANYREFS:
+		return SKB_DROP_REASON_UNIX_INFLIGHT_FD_LIMIT;
+	}
+
+	DEBUG_NET_WARN_ONCE(1,
+			    "Define a drop reason for %d in unix_scm_to_skb().",
+			    err);
+
+	return SKB_DROP_REASON_NOT_SPECIFIED;
+}
+
 static bool unix_passcred_enabled(const struct socket *sock,
 				  const struct sock *other)
 {
@@ -2249,14 +2265,13 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
 static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 			       size_t len)
 {
+	enum skb_drop_reason reason;
 	struct sock *sk = sock->sk;
 	struct sock *other = NULL;
-	int err, size;
-	struct sk_buff *skb;
-	int sent = 0;
 	struct scm_cookie scm;
 	bool fds_sent = false;
-	int data_len;
+	struct sk_buff *skb;
+	int err, sent = 0;
 
 	err = scm_send(sock, msg, &scm, false);
 	if (err < 0)
@@ -2294,7 +2309,8 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 	}
 
 	while (sent < len) {
-		size = len - sent;
+		int size = len - sent;
+		int data_len;
 
 		if (unlikely(msg->msg_flags & MSG_SPLICE_PAGES)) {
 			skb = sock_alloc_send_pskb(sk, 0, 0,
@@ -2320,8 +2336,10 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 
 		/* Only send the fds in the first buffer */
 		err = unix_scm_to_skb(&scm, skb, !fds_sent);
-		if (err < 0)
+		if (err < 0) {
+			reason = unix_scm_err_to_reason(err);
 			goto out_free;
+		}
 
 		fds_sent = true;
 
@@ -2329,8 +2347,10 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 			skb->ip_summed = CHECKSUM_UNNECESSARY;
 			err = skb_splice_from_iter(skb, &msg->msg_iter, size,
 						   sk->sk_allocation);
-			if (err < 0)
+			if (err < 0) {
+				reason = SKB_DROP_REASON_SKB_UCOPY_FAULT;
 				goto out_free;
+			}
 
 			size = err;
 			refcount_add(size, &sk->sk_wmem_alloc);
@@ -2339,15 +2359,23 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 			skb->data_len = data_len;
 			skb->len = size;
 			err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, size);
-			if (err)
+			if (err) {
+				reason = SKB_DROP_REASON_SKB_UCOPY_FAULT;
 				goto out_free;
+			}
 		}
 
 		unix_state_lock(other);
 
-		if (sock_flag(other, SOCK_DEAD) ||
-		    (other->sk_shutdown & RCV_SHUTDOWN))
+		if (sock_flag(other, SOCK_DEAD)) {
+			reason = SKB_DROP_REASON_SOCKET_CLOSE;
+			goto out_pipe;
+		}
+
+		if (other->sk_shutdown & RCV_SHUTDOWN) {
+			reason = SKB_DROP_REASON_SOCKET_RCV_SHUTDOWN;
 			goto out_pipe;
+		}
 
 		maybe_add_creds(skb, sock, other);
 		scm_stat_add(other, skb);
@@ -2376,7 +2404,7 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
 		send_sig(SIGPIPE, current, 0);
 	err = -EPIPE;
 out_free:
-	kfree_skb(skb);
+	kfree_skb_reason(skb, reason);
 out_err:
 	scm_destroy(&scm);
 	return sent ? : err;
-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 07/11] af_unix: Set drop reason in queue_oob().

[Kuniyuki Iwashima]

sendmsg(MSG_OOB) to a SOCK_STREAM socket could fail for various
reasons.

Let's set drop reasons respectively.

The drop reasons are exactly the same as in the previous patch.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 8c8d8fc3cb94..8623e3368c45 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2216,34 +2216,37 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
 		     struct scm_cookie *scm, bool fds_sent)
 {
 	struct unix_sock *ousk = unix_sk(other);
+	enum skb_drop_reason reason;
 	struct sk_buff *skb;
-	int err = 0;
+	int err;
 
 	skb = sock_alloc_send_skb(sock->sk, 1, msg->msg_flags & MSG_DONTWAIT, &err);
-
 	if (!skb)
-		return err;
+		goto out;
 
 	err = unix_scm_to_skb(scm, skb, !fds_sent);
 	if (err < 0) {
-		kfree_skb(skb);
-		return err;
+		reason = unix_scm_err_to_reason(err);
+		goto out_free;
 	}
+
 	skb_put(skb, 1);
 	err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, 1);
-
 	if (err) {
-		kfree_skb(skb);
-		return err;
+		reason = SKB_DROP_REASON_SKB_UCOPY_FAULT;
+		goto out_free;
 	}
 
 	unix_state_lock(other);
 
-	if (sock_flag(other, SOCK_DEAD) ||
-	    (other->sk_shutdown & RCV_SHUTDOWN)) {
-		unix_state_unlock(other);
-		kfree_skb(skb);
-		return -EPIPE;
+	if (sock_flag(other, SOCK_DEAD)) {
+		reason = SKB_DROP_REASON_SOCKET_CLOSE;
+		goto out_unlock;
+	}
+
+	if (other->sk_shutdown & RCV_SHUTDOWN) {
+		reason = SKB_DROP_REASON_SOCKET_RCV_SHUTDOWN;
+		goto out_unlock;
 	}
 
 	maybe_add_creds(skb, sock, other);
@@ -2258,6 +2261,14 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
 	unix_state_unlock(other);
 	other->sk_data_ready(other);
 
+	return 0;
+
+out_unlock:
+	unix_state_unlock(other);
+	err = -EPIPE;
+out_free:
+	kfree_skb_reason(skb, reason);
+out:
 	return err;
 }
 #endif
-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 08/11] af_unix: Set drop reason in manage_oob().

[Kuniyuki Iwashima]

AF_UNIX SOCK_STREAM socket supports MSG_OOB.

When OOB data is sent to a socket, recv() will break at that point.

If the next recv() does not have MSG_OOB, the normal data following
the OOB data is returned.

Then, the OOB skb is dropped.

Let's define a new drop reason for that case in manage_oob().

  # echo 1 > /sys/kernel/tracing/events/skb/kfree_skb/enable

  # python3
  >>> from socket import *
  >>> s1, s2 = socketpair(AF_UNIX)
  >>> s1.send(b'a', MSG_OOB)
  >>> s1.send(b'b')
  >>> s2.recv(2)
  b'b'

  # cat /sys/kernel/tracing/trace_pipe
  ...
     python3-223 ... kfree_skb: ... location=unix_stream_read_generic+0x59e/0xc20 reason: UNIX_SKIP_OOB

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/dropreason-core.h | 6 ++++++
 net/unix/af_unix.c            | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index dea6bbe3ceaa..43e20acaa98a 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -12,6 +12,7 @@
 	FN(SOCKET_RCVBUFF)		\
 	FN(SOCKET_RCV_SHUTDOWN)		\
 	FN(UNIX_INFLIGHT_FD_LIMIT)	\
+	FN(UNIX_SKIP_OOB)		\
 	FN(PKT_TOO_SMALL)		\
 	FN(TCP_CSUM)			\
 	FN(UDP_CSUM)			\
@@ -156,6 +157,11 @@ enum skb_drop_reason {
 	 * are passed via SCM_RIGHTS but not yet received, reaching RLIMIT_NOFILE.
 	 */
 	SKB_DROP_REASON_UNIX_INFLIGHT_FD_LIMIT,
+	/**
+	 * @SKB_DROP_REASON_UNIX_SKIP_OOB: Out-Of-Band data is skipped by
+	 * recv() without MSG_OOB so dropped.
+	 */
+	SKB_DROP_REASON_UNIX_SKIP_OOB,
 	/** @SKB_DROP_REASON_PKT_TOO_SMALL: packet size is too small */
 	SKB_DROP_REASON_PKT_TOO_SMALL,
 	/** @SKB_DROP_REASON_TCP_CSUM: TCP checksum error */
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 8623e3368c45..efd1b83b152a 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2748,7 +2748,7 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
 	spin_unlock(&sk->sk_receive_queue.lock);
 
 	consume_skb(read_skb);
-	kfree_skb(unread_skb);
+	kfree_skb_reason(unread_skb, SKB_DROP_REASON_UNIX_SKIP_OOB);
 
 	return skb;
 }
-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 09/11] af_unix: Set drop reason in unix_stream_read_skb().

[Kuniyuki Iwashima]

unix_stream_read_skb() is called when BPF SOCKMAP reads some data
from a socket in the map.

SOCKMAP does not support MSG_OOB, and reading OOB results in a drop.

Let's set drop reasons respectively.

  * SOCKET_CLOSE  : the socket in SOCKMAP was close()d
  * UNIX_SKIP_OOB : OOB was read from the socket in SOCKMAP

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/unix/af_unix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index efd1b83b152a..ed577276bd27 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2777,7 +2777,7 @@ static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
 
 		if (sock_flag(sk, SOCK_DEAD)) {
 			unix_state_unlock(sk);
-			kfree_skb(skb);
+			kfree_skb_reason(skb, SKB_DROP_REASON_SOCKET_CLOSE);
 			return -ECONNRESET;
 		}
 
@@ -2791,7 +2791,7 @@ static int unix_stream_read_skb(struct sock *sk, skb_read_actor_t recv_actor)
 		unix_state_unlock(sk);
 
 		if (drop) {
-			kfree_skb(skb);
+			kfree_skb_reason(skb, SKB_DROP_REASON_UNIX_SKIP_OOB);
 			return -EAGAIN;
 		}
 	}
-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 10/11] af_unix: Set drop reason in unix_dgram_disconnected().

[Kuniyuki Iwashima]

unix_dgram_disconnected() is called from two places:

  1. when a connect()ed socket dis-connect()s or re-connect()s to
     another socket

  2. when sendmsg() fails because the peer socket that the client
     has connect()ed to has been close()d

Then, the client's recv queue is purged to remove all messages from
the old peer socket.

Let's define a new drop reason for that case.

  # echo 1 > /sys/kernel/tracing/events/skb/kfree_skb/enable

  # python3
  >>> from socket import *
  >>>
  >>> # s1 has a message from s2
  >>> s1, s2 = socketpair(AF_UNIX, SOCK_DGRAM)
  >>> s2.send(b'hello world')
  >>>
  >>> # re-connect() drops the message from s2
  >>> s3 = socket(AF_UNIX, SOCK_DGRAM)
  >>> s3.bind('')
  >>> s1.connect(s3.getsockname())

  # cat /sys/kernel/tracing/trace_pipe
     python3-250 ... kfree_skb: ... location=skb_queue_purge_reason+0xdc/0x110 reason: UNIX_DISCONNECT

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/dropreason-core.h | 7 +++++++
 net/unix/af_unix.c            | 4 +++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 43e20acaa98a..580be34ffa4f 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -11,6 +11,7 @@
 	FN(SOCKET_INVALID_STATE)	\
 	FN(SOCKET_RCVBUFF)		\
 	FN(SOCKET_RCV_SHUTDOWN)		\
+	FN(UNIX_DISCONNECT)		\
 	FN(UNIX_INFLIGHT_FD_LIMIT)	\
 	FN(UNIX_SKIP_OOB)		\
 	FN(PKT_TOO_SMALL)		\
@@ -152,6 +153,12 @@ enum skb_drop_reason {
 	SKB_DROP_REASON_SOCKET_RCVBUFF,
 	/** @SKB_DROP_REASON_SOCKET_RCV_SHUTDOWN: socket is shutdown(SHUT_RD) */
 	SKB_DROP_REASON_SOCKET_RCV_SHUTDOWN,
+	/**
+	 * @SKB_DROP_REASON_UNIX_DISCONNECT: recv queue is purged when SOCK_DGRAM
+	 * or SOCK_SEQPACKET socket re-connect()s to another socket or notices
+	 * during send() that the peer has been close()d.
+	 */
+	SKB_DROP_REASON_UNIX_DISCONNECT,
 	/**
 	 * @SKB_DROP_REASON_UNIX_INFLIGHT_FD_LIMIT: too many file descriptors
 	 * are passed via SCM_RIGHTS but not yet received, reaching RLIMIT_NOFILE.
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index ed577276bd27..1c374bac7b48 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -622,7 +622,9 @@ 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(&sk->sk_receive_queue);
+		skb_queue_purge_reason(&sk->sk_receive_queue,
+				       SKB_DROP_REASON_UNIX_DISCONNECT);
+
 		wake_up_interruptible_all(&unix_sk(sk)->peer_wait);
 
 		/* If one link of bidirectional dgram pipe is disconnected,
-- 
2.39.5 (Apple Git-154)

[PATCH v2 net-next 11/11] af_unix: Set drop reason in unix_dgram_sendmsg().

[Kuniyuki Iwashima]

sendmsg() to a SOCK_DGRAM / SOCK_SEQPACKET socket could fail for
various reasons.

Let's set drop reasons respectively.

  * SOCKET_FILTER : BPF filter dropped the skb
  * UNIX_NO_PERM  : the peer connect()ed to another socket

To reproduce UNIX_NO_PERM:

  # echo 1 > /sys/kernel/tracing/events/skb/kfree_skb/enable

  # python3
  >>> from socket import *
  >>> s1, s2 = socketpair(AF_UNIX, SOCK_DGRAM)
  >>> s2.bind('')
  >>> s3 = socket(AF_UNIX, SOCK_DGRAM)
  >>> s3.sendto(b'hello world', s2.getsockname())
  Traceback (most recent call last):
    File "<stdin>", line 1, in <module>
  PermissionError: [Errno 1] Operation not permitted

  # cat /sys/kernel/tracing/trace_pipe
  ...
     python3-196 ... kfree_skb: ... location=unix_dgram_sendmsg+0x3d1/0x970 reason: UNIX_NO_PERM

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/dropreason-core.h |  6 ++++++
 net/unix/af_unix.c            | 28 +++++++++++++++++++++++-----
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/include/net/dropreason-core.h b/include/net/dropreason-core.h
index 580be34ffa4f..a8db05defa5a 100644
--- a/include/net/dropreason-core.h
+++ b/include/net/dropreason-core.h
@@ -13,6 +13,7 @@
 	FN(SOCKET_RCV_SHUTDOWN)		\
 	FN(UNIX_DISCONNECT)		\
 	FN(UNIX_INFLIGHT_FD_LIMIT)	\
+	FN(UNIX_NO_PERM)		\
 	FN(UNIX_SKIP_OOB)		\
 	FN(PKT_TOO_SMALL)		\
 	FN(TCP_CSUM)			\
@@ -164,6 +165,11 @@ enum skb_drop_reason {
 	 * are passed via SCM_RIGHTS but not yet received, reaching RLIMIT_NOFILE.
 	 */
 	SKB_DROP_REASON_UNIX_INFLIGHT_FD_LIMIT,
+	/**
+	 * @SKB_DROP_REASON_UNIX_NO_PERM: the peer socket is already connect()ed
+	 * to another SOCK_DGRAM/SOCK_SEQPACKET socket.
+	 */
+	SKB_DROP_REASON_UNIX_NO_PERM,
 	/**
 	 * @SKB_DROP_REASON_UNIX_SKIP_OOB: Out-Of-Band data is skipped by
 	 * recv() without MSG_OOB so dropped.
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 1c374bac7b48..c34a0cd396c2 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1991,6 +1991,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 {
 	struct sock *sk = sock->sk, *other = NULL;
 	struct unix_sock *u = unix_sk(sk);
+	enum skb_drop_reason reason;
 	struct scm_cookie scm;
 	struct sk_buff *skb;
 	int data_len = 0;
@@ -2051,15 +2052,19 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 		goto out;
 
 	err = unix_scm_to_skb(&scm, skb, true);
-	if (err < 0)
+	if (err < 0) {
+		reason = unix_scm_err_to_reason(err);
 		goto out_free;
+	}
 
 	skb_put(skb, len - data_len);
 	skb->data_len = data_len;
 	skb->len = len;
 	err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, len);
-	if (err)
+	if (err) {
+		reason = SKB_DROP_REASON_SKB_UCOPY_FAULT;
 		goto out_free;
+	}
 
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
 
@@ -2069,12 +2074,14 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 					msg->msg_namelen, sk->sk_type);
 		if (IS_ERR(other)) {
 			err = PTR_ERR(other);
+			reason = SKB_DROP_REASON_NO_SOCKET;
 			goto out_free;
 		}
 	} else {
 		other = unix_peer_get(sk);
 		if (!other) {
 			err = -ENOTCONN;
+			reason = SKB_DROP_REASON_NO_SOCKET;
 			goto out_free;
 		}
 	}
@@ -2082,6 +2089,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 	if (sk_filter(other, skb) < 0) {
 		/* Toss the packet but do not return any error to the sender */
 		err = len;
+		reason = SKB_DROP_REASON_SOCKET_FILTER;
 		goto out_sock_put;
 	}
 
@@ -2092,6 +2100,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	if (!unix_may_send(sk, other)) {
 		err = -EPERM;
+		reason = SKB_DROP_REASON_UNIX_NO_PERM;
 		goto out_unlock;
 	}
 
@@ -2106,6 +2115,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			 * unlike SOCK_DGRAM wants.
 			 */
 			err = -EPIPE;
+			reason = SKB_DROP_REASON_SOCKET_CLOSE;
 			goto out_sock_put;
 		}
 
@@ -2122,6 +2132,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			unix_dgram_disconnected(sk, other);
 			sock_put(other);
 			err = -ECONNREFUSED;
+			reason = SKB_DROP_REASON_SOCKET_CLOSE;
 			goto out_sock_put;
 		}
 
@@ -2129,6 +2140,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 
 		if (!msg->msg_namelen) {
 			err = -ECONNRESET;
+			reason = SKB_DROP_REASON_SOCKET_CLOSE;
 			goto out_sock_put;
 		}
 
@@ -2137,13 +2149,16 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 
 	if (other->sk_shutdown & RCV_SHUTDOWN) {
 		err = -EPIPE;
+		reason = SKB_DROP_REASON_SOCKET_RCV_SHUTDOWN;
 		goto out_unlock;
 	}
 
 	if (sk->sk_type != SOCK_SEQPACKET) {
 		err = security_unix_may_send(sk->sk_socket, other->sk_socket);
-		if (err)
+		if (err) {
+			reason = SKB_DROP_REASON_SECURITY_HOOK;
 			goto out_unlock;
+		}
 	}
 
 	/* other == sk && unix_peer(other) != sk if
@@ -2157,8 +2172,10 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 			timeo = unix_wait_for_peer(other, timeo);
 
 			err = sock_intr_errno(timeo);
-			if (signal_pending(current))
+			if (signal_pending(current)) {
+				reason = SKB_DROP_REASON_SOCKET_RCVBUFF;
 				goto out_sock_put;
+			}
 
 			goto restart;
 		}
@@ -2171,6 +2188,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 		if (unix_peer(sk) != other ||
 		    unix_dgram_peer_wake_me(sk, other)) {
 			err = -EAGAIN;
+			reason = SKB_DROP_REASON_SOCKET_RCVBUFF;
 			sk_locked = 1;
 			goto out_unlock;
 		}
@@ -2202,7 +2220,7 @@ static int unix_dgram_sendmsg(struct socket *sock, struct msghdr *msg,
 out_sock_put:
 	sock_put(other);
 out_free:
-	kfree_skb(skb);
+	kfree_skb_reason(skb, reason);
 out:
 	scm_destroy(&scm);
 	return err;
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2 net-next 06/11] af_unix: Set drop reason in unix_stream_sendmsg().

[Jakub Kicinski]

On Sun, 12 Jan 2025 13:08:05 +0900 Kuniyuki Iwashima wrote:
> @@ -2249,14 +2265,13 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
>  static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
>  			       size_t len)
>  {
> +	enum skb_drop_reason reason;

I feel like we should draw the line somewhere for the reason codes.
We started with annotating packet drops in the stack, which are
otherwise hard to notice, we don't even have counters for all of them.
But at this point we're annotating sendmsg() errors? The fact we free
an skb on the error path seems rather coincidental for a sendmsg error.
IOW aren't we moving from packet loss annotation into general tracing
territory here?

If there is no ambiguity and application will get an error from a system
call I'd just use consume_skb().

I'm probably the most resistant to the drop reason codes, so I defer 
to Paolo / Eric for the real judgment...

Re: [PATCH v2 net-next 06/11] af_unix: Set drop reason in unix_stream_sendmsg().

[Donald Hunter]

On Tue, 14 Jan 2025 at 20:05, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Sun, 12 Jan 2025 13:08:05 +0900 Kuniyuki Iwashima wrote:
> > @@ -2249,14 +2265,13 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
> >  static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
> >                              size_t len)
> >  {
> > +     enum skb_drop_reason reason;
>
> I feel like we should draw the line somewhere for the reason codes.
> We started with annotating packet drops in the stack, which are
> otherwise hard to notice, we don't even have counters for all of them.
> But at this point we're annotating sendmsg() errors? The fact we free
> an skb on the error path seems rather coincidental for a sendmsg error.
> IOW aren't we moving from packet loss annotation into general tracing
> territory here?
>
> If there is no ambiguity and application will get an error from a system
> call I'd just use consume_skb().
>
> I'm probably the most resistant to the drop reason codes, so I defer
> to Paolo / Eric for the real judgment...

For what it's worth, I agree that there's no need to annotate a drop
reason for sendmsg failures that return error codes to the caller.
That's why my original patch proposal just changed them to use
consume_skb(). I did misrepresent the cases as "happy path" but I
really meant that from the perspective of "no send initiated, so no
drop reason".

https://lore.kernel.org/netdev/20241116094236.28786-1-donald.hunter@gmail.com/

Thanks,
Donald.

Re: [PATCH v2 net-next 06/11] af_unix: Set drop reason in unix_stream_sendmsg().

[Paolo Abeni]

On 1/15/25 2:52 PM, Donald Hunter wrote:
> On Tue, 14 Jan 2025 at 20:05, Jakub Kicinski <kuba@kernel.org> wrote:
>>
>> On Sun, 12 Jan 2025 13:08:05 +0900 Kuniyuki Iwashima wrote:
>>> @@ -2249,14 +2265,13 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
>>>  static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
>>>                              size_t len)
>>>  {
>>> +     enum skb_drop_reason reason;
>>
>> I feel like we should draw the line somewhere for the reason codes.
>> We started with annotating packet drops in the stack, which are
>> otherwise hard to notice, we don't even have counters for all of them.
>> But at this point we're annotating sendmsg() errors? The fact we free
>> an skb on the error path seems rather coincidental for a sendmsg error.
>> IOW aren't we moving from packet loss annotation into general tracing
>> territory here?
>>
>> If there is no ambiguity and application will get an error from a system
>> call I'd just use consume_skb().
>>
>> I'm probably the most resistant to the drop reason codes, so I defer
>> to Paolo / Eric for the real judgment...
> 
> For what it's worth, I agree that there's no need to annotate a drop
> reason for sendmsg failures that return error codes to the caller.
> That's why my original patch proposal just changed them to use
> consume_skb(). I did misrepresent the cases as "happy path" but I
> really meant that from the perspective of "no send initiated, so no
> drop reason".
> 
> https://lore.kernel.org/netdev/20241116094236.28786-1-donald.hunter@gmail.com/

I also agree with Jakub with a slightly different reasoning. IMHO drop
reason goal is to let user-space easily understand where/why skbs are
dropped. If the drop reason reflects a syscall error code, the
user-space already has all the info.

IIRC the general guidance agreed upon in the last Netconf was to add
drop reasons when we can't distinguish multiple kind of drops within the
same function. IMHO such guidance fits with not using drop reason in
this specific case: as said we can discriminate the errors via the
syscall error code.

Cheers,

Paolo

Re: [PATCH v2 net-next 00/11] af_unix: Set skb drop reason in every kfree_skb() path.

[Joe Damato]

On Sun, Jan 12, 2025 at 01:07:59PM +0900, Kuniyuki Iwashima wrote:
> There is a potential user for skb drop reason for AF_UNIX.
> 
> This series sets skb drop reason for every kfree_skb() path
> in AF_UNIX code.
> 
> Link: https://lore.kernel.org/netdev/CAAf2ycmZHti95WaBR3s+L5Epm1q7sXmvZ-EqCK=-oZj=45tOwQ@mail.gmail.com/
> 
> 
> Changes:
>   v2:
>     * Drop the old patch 6 to silence false-positive uninit warning
> 
>   v1: https://lore.kernel.org/netdev/20250110092641.85905-1-kuniyu@amazon.com/
> 
> 
> Kuniyuki Iwashima (11):
>   net: dropreason: Gather SOCKET_ drop reasons.
>   af_unix: Set drop reason in unix_release_sock().
>   af_unix: Set drop reason in unix_sock_destructor().
>   af_unix: Set drop reason in __unix_gc().
>   af_unix: Set drop reason in unix_stream_connect().
>   af_unix: Set drop reason in unix_stream_sendmsg().
>   af_unix: Set drop reason in queue_oob().
>   af_unix: Set drop reason in manage_oob().
>   af_unix: Set drop reason in unix_stream_read_skb().
>   af_unix: Set drop reason in unix_dgram_disconnected().
>   af_unix: Set drop reason in unix_dgram_sendmsg().
> 
>  include/net/dropreason-core.h |  46 ++++++++--
>  net/unix/af_unix.c            | 153 +++++++++++++++++++++++++---------
>  net/unix/garbage.c            |   2 +-
>  3 files changed, 154 insertions(+), 47 deletions(-)

I know there's feedback from others on some parts of this series but
I wanted to say thank you for the detailed commit messages that
include python examples.

I found that very refreshing and helpful when attempting to review
the code you've proposed; thanks for putting in the extra effort to
include those examples in the commit messages.

Re: [PATCH v2 net-next 06/11] af_unix: Set drop reason in unix_stream_sendmsg().

[Kuniyuki Iwashima]

From: Paolo Abeni <pabeni@redhat.com>
Date: Wed, 15 Jan 2025 16:12:59 +0100
> On 1/15/25 2:52 PM, Donald Hunter wrote:
> > On Tue, 14 Jan 2025 at 20:05, Jakub Kicinski <kuba@kernel.org> wrote:
> >>
> >> On Sun, 12 Jan 2025 13:08:05 +0900 Kuniyuki Iwashima wrote:
> >>> @@ -2249,14 +2265,13 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
> >>>  static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
> >>>                              size_t len)
> >>>  {
> >>> +     enum skb_drop_reason reason;
> >>
> >> I feel like we should draw the line somewhere for the reason codes.
> >> We started with annotating packet drops in the stack, which are
> >> otherwise hard to notice, we don't even have counters for all of them.
> >> But at this point we're annotating sendmsg() errors? The fact we free
> >> an skb on the error path seems rather coincidental for a sendmsg error.
> >> IOW aren't we moving from packet loss annotation into general tracing
> >> territory here?
> >>
> >> If there is no ambiguity and application will get an error from a system
> >> call I'd just use consume_skb().
> >>
> >> I'm probably the most resistant to the drop reason codes, so I defer
> >> to Paolo / Eric for the real judgment...
> > 
> > For what it's worth, I agree that there's no need to annotate a drop
> > reason for sendmsg failures that return error codes to the caller.
> > That's why my original patch proposal just changed them to use
> > consume_skb(). I did misrepresent the cases as "happy path" but I
> > really meant that from the perspective of "no send initiated, so no
> > drop reason".
> > 
> > https://lore.kernel.org/netdev/20241116094236.28786-1-donald.hunter@gmail.com/
> 
> I also agree with Jakub with a slightly different reasoning. IMHO drop
> reason goal is to let user-space easily understand where/why skbs are
> dropped. If the drop reason reflects a syscall error code, the
> user-space already has all the info.
> 
> IIRC the general guidance agreed upon in the last Netconf was to add
> drop reasons when we can't distinguish multiple kind of drops within the
> same function. IMHO such guidance fits with not using drop reason in
> this specific case: as said we can discriminate the errors via the
> syscall error code.

Thanks for the feedback, all!

I'll change kfree_skb() to consume_skb() in

  * unix_stream_connect()
  * unix_stream_sendmsg()
  * queue_oob()
  * unix_dgram_sendmsg()

where we can distinguish what happened from errno.

Re: [PATCH v2 net-next 00/11] af_unix: Set skb drop reason in every kfree_skb() path.

[Kuniyuki Iwashima]

From: Joe Damato <jdamato@fastly.com>
Date: Wed, 15 Jan 2025 09:38:36 -0800
> On Sun, Jan 12, 2025 at 01:07:59PM +0900, Kuniyuki Iwashima wrote:
> > There is a potential user for skb drop reason for AF_UNIX.
> > 
> > This series sets skb drop reason for every kfree_skb() path
> > in AF_UNIX code.
> > 
> > Link: https://lore.kernel.org/netdev/CAAf2ycmZHti95WaBR3s+L5Epm1q7sXmvZ-EqCK=-oZj=45tOwQ@mail.gmail.com/
> > 
> > 
> > Changes:
> >   v2:
> >     * Drop the old patch 6 to silence false-positive uninit warning
> > 
> >   v1: https://lore.kernel.org/netdev/20250110092641.85905-1-kuniyu@amazon.com/
> > 
> > 
> > Kuniyuki Iwashima (11):
> >   net: dropreason: Gather SOCKET_ drop reasons.
> >   af_unix: Set drop reason in unix_release_sock().
> >   af_unix: Set drop reason in unix_sock_destructor().
> >   af_unix: Set drop reason in __unix_gc().
> >   af_unix: Set drop reason in unix_stream_connect().
> >   af_unix: Set drop reason in unix_stream_sendmsg().
> >   af_unix: Set drop reason in queue_oob().
> >   af_unix: Set drop reason in manage_oob().
> >   af_unix: Set drop reason in unix_stream_read_skb().
> >   af_unix: Set drop reason in unix_dgram_disconnected().
> >   af_unix: Set drop reason in unix_dgram_sendmsg().
> > 
> >  include/net/dropreason-core.h |  46 ++++++++--
> >  net/unix/af_unix.c            | 153 +++++++++++++++++++++++++---------
> >  net/unix/garbage.c            |   2 +-
> >  3 files changed, 154 insertions(+), 47 deletions(-)
> 
> I know there's feedback from others on some parts of this series but
> I wanted to say thank you for the detailed commit messages that
> include python examples.
> 
> I found that very refreshing and helpful when attempting to review
> the code you've proposed; thanks for putting in the extra effort to
> include those examples in the commit messages.

Thanks, Joe.

The goal was to give full information in the commit message and
the drop reason comments so that user can reference it and will
never ask why skb was dropped.

Now I think the part of the effort should've put into man-pages
instead ;) (and will do)