Re: [PATCH v2 net 2/2] af_unix: Don't peek OOB data without MSG_OOB.

[Rao Shoaib <rao.shoaib@oracle.com>]

[-- Attachment #1: Type: text/plain, Size: 2624 bytes --]

The proposed fix is not the correct fix as among other things it does 
not allow going pass the OOB if data is present. TCP allows that.

I have attached a patch that fixes this issue and other issues that I 
encountered in my testing. I compared TCP.

Shoaib

On 4/10/24 10:10, Kuniyuki Iwashima wrote:
> Currently, we can read OOB data without MSG_OOB by using MSG_PEEK
> when OOB data is sitting on the front row, which is apparently
> wrong.
> 
>    >>> from socket import *
>    >>> c1, c2 = socketpair(AF_UNIX, SOCK_STREAM)
>    >>> c1.send(b'a', MSG_OOB)
>    1
>    >>> c2.recv(1, MSG_PEEK | MSG_DONTWAIT)
>    b'a'
> 
> If manage_oob() is called when no data has been copied, we only
> check if the socket enables SO_OOBINLINE or MSG_PEEK is not used.
> Otherwise, the skb is returned as is.
> 
> However, here we should return NULL if MSG_PEEK is set and no data
> has been copied.
> 
> Also, in such a case, we should not jump to the redo label because
> we will be caught in the loop and hog the CPU until normal data
> comes in.
> 
> Then, we need to handle skb == NULL case with the if-clause below
> the manage_oob() block.
> 
> With this patch:
> 
>    >>> from socket import *
>    >>> c1, c2 = socketpair(AF_UNIX, SOCK_STREAM)
>    >>> c1.send(b'a', MSG_OOB)
>    1
>    >>> c2.recv(1, MSG_PEEK | MSG_DONTWAIT)
>    Traceback (most recent call last):
>      File "<stdin>", line 1, in <module>
>    BlockingIOError: [Errno 11] Resource temporarily unavailable
> 
> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
>   net/unix/af_unix.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index f297320438bf..9a6ad5974dff 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2663,7 +2663,9 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
>   					WRITE_ONCE(u->oob_skb, NULL);
>   					consume_skb(skb);
>   				}
> -			} else if (!(flags & MSG_PEEK)) {
> +			} else if (flags & MSG_PEEK) {
> +				skb = NULL;
> +			} else {
>   				skb_unlink(skb, &sk->sk_receive_queue);
>   				WRITE_ONCE(u->oob_skb, NULL);
>   				if (!WARN_ON_ONCE(skb_unref(skb)))
> @@ -2745,11 +2747,9 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
>   #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
>   		if (skb) {
>   			skb = manage_oob(skb, sk, flags, copied);
> -			if (!skb) {
> +			if (!skb && copied) {
>   				unix_state_unlock(sk);
> -				if (copied)
> -					break;
> -				goto redo;
> +				break;
>   			}
>   		}
>   #endif

[-- Attachment #2: 0001-AF_UNIX-Fix-handling-of-OOB-Data.patch --]
[-- Type: text/x-patch, Size: 2772 bytes --]

From bf541423b785ae14ee2c7015d96dba6ac368358c Mon Sep 17 00:00:00 2001
From: Rao Shoaib <Rao.Shoaib@oracle.com>
Date: Tue, 16 Apr 2024 12:39:54 -0700
Subject: [PATCH] AF_UNIX: Fix handling of OOB Data

This patch fixes corner cases in reading when there is a
pending OOB and MSG_PEEK flag is set.
Since there is no standard for handling MSG_OOB and MSG_PEEK,
behavior of TCP is used as a reference.

First byte in the read queue is OOB byte
First byte in the read queue is OOB byte followed by normal data
New OOB arrives before the previous one was consumed

Signed-off-by: Rao Shoaib <Rao.Shoaib@oracle.com>
---
 net/unix/af_unix.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index d032eb5fa6df..50cae03a1f98 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2217,8 +2217,13 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other
 	maybe_add_creds(skb, sock, other);
 	skb_get(skb);
 
-	if (ousk->oob_skb)
-		consume_skb(ousk->oob_skb);
+	if (ousk->oob_skb) {
+		skb_unref(ousk->oob_skb);
+		if (!sock_flag(other, SOCK_URGINLINE)) {
+			skb_unlink(ousk->oob_skb, &other->sk_receive_queue);
+			consume_skb(ousk->oob_skb);
+		}
+	}
 
 	WRITE_ONCE(ousk->oob_skb, skb);
 
@@ -2658,17 +2663,20 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
 		if (skb == u->oob_skb) {
 			if (copied) {
 				skb = NULL;
-			} else if (sock_flag(sk, SOCK_URGINLINE)) {
-				if (!(flags & MSG_PEEK)) {
+			} else if (!(flags & MSG_PEEK)) {
+				if (sock_flag(sk, SOCK_URGINLINE)) {
 					WRITE_ONCE(u->oob_skb, NULL);
 					consume_skb(skb);
+				} else {
+					skb_unlink(skb, &sk->sk_receive_queue);
+					WRITE_ONCE(u->oob_skb, NULL);
+					if (!WARN_ON_ONCE(skb_unref(skb)))
+						kfree_skb(skb);
+					skb = skb_peek(&sk->sk_receive_queue);
 				}
-			} else if (!(flags & MSG_PEEK)) {
-				skb_unlink(skb, &sk->sk_receive_queue);
-				WRITE_ONCE(u->oob_skb, NULL);
-				if (!WARN_ON_ONCE(skb_unref(skb)))
-					kfree_skb(skb);
-				skb = skb_peek(&sk->sk_receive_queue);
+			} else {
+				if (!sock_flag(sk, SOCK_URGINLINE))
+					skb = skb_peek_next(skb, &sk->sk_receive_queue);
 			}
 		}
 	}
@@ -2741,18 +2749,18 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
 		last = skb = skb_peek(&sk->sk_receive_queue);
 		last_len = last ? last->len : 0;
 
+again:
 #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
 		if (skb) {
 			skb = manage_oob(skb, sk, flags, copied);
 			if (!skb) {
 				unix_state_unlock(sk);
-				if (copied)
+				if (copied || (flags & MSG_PEEK))
 					break;
 				goto redo;
 			}
 		}
 #endif
-again:
 		if (skb == NULL) {
 			if (copied >= target)
 				goto unlock;
-- 
2.39.3