[PATCH v3 0/2] [net] af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag

[PATCH v3 0/2] [net] af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag

[Aaron Conole]

This patch set implements a bugfix for kernel.org bugzilla #12323, allowing
MSG_PEEK to return all queued data on the unix domain socket, not just the
data contained in a single SKB. 

This is the v3 version of this patch, which includes a suggested modification
by Eric Dumazet to convert the unix_sk() conversion macro to a static inline
function. These patches are independent and can be applied separately.

This set was tested over a 24-hour period, utilizing a loop continually 
executing the bugzilla issue attached python code. It was instrumented with
a pr_err_once() ([   13.798683] unix: went there at least one time).

Aaron Conole (2):
  [net] af_unix: Convert the unix_sk macro to an inline function for    
    type safety
  [net] af_unix: return data from multiple SKBs on recv() with MSG_PEEK
    flag

 include/net/af_unix.h |  6 +++++-
 net/unix/af_unix.c    | 19 +++++++++++++++++--
 2 files changed, 22 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH v3 1/2] [net] af_unix: Convert the unix_sk macro to an inline function for type safety

[Aaron Conole]

As suggested by Eric Dumazet in
http://marc.info/?l=linux-netdev&m=144276759421433&w=2

This change replaces the #define with a static inline function to
enjoy complaints by the compiler when misusing the API.

Signed-off-by: Aaron Conole <aconole@bytheb.org>
---
 include/net/af_unix.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 4a167b3..cb1b9bb 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -63,7 +63,11 @@ struct unix_sock {
 #define UNIX_GC_MAYBE_CYCLE	1
 	struct socket_wq	peer_wq;
 };
-#define unix_sk(__sk) ((struct unix_sock *)__sk)
+
+static inline struct unix_sock *unix_sk(struct sock *sk)
+{
+	return (struct unix_sock *)sk;
+}
 
 #define peer_wait peer_wq.wait
 
-- 
1.9.1

[PATCH v3 2/2] [net] af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag

[Aaron Conole]

AF_UNIX sockets now return multiple skbs from recv() when MSG_PEEK flag
is set.

This is referenced in kernel bugzilla #12323 @
https://bugzilla.kernel.org/show_bug.cgi?id=12323

As described both in the BZ and lkml thread @
http://lkml.org/lkml/2008/1/8/444 calling recv() with MSG_PEEK on an
AF_UNIX socket only reads a single skb, where the desired effect is
to return as much skb data has been queued, until hitting the recv
buffer size (whichever comes first).

The modified MSG_PEEK path will now move to the next skb in the tree
and jump to the again: label, rather than following the natural loop
structure. This requires duplicating some of the loop head actions.

This was tested using the python socketpair python code attached to
the bugzilla issue.

Signed-off-by: Aaron Conole <aconole@bytheb.org>
---
 net/unix/af_unix.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 03ee4d3..f8ef53f 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2179,9 +2179,24 @@ unlock:
 			if (UNIXCB(skb).fp)
 				scm.fp = scm_fp_dup(UNIXCB(skb).fp);
 
-			sk_peek_offset_fwd(sk, chunk);
+			if (skip) {
+				sk_peek_offset_fwd(sk, chunk);
+				skip -= chunk;
+			}
 
-			break;
+			if (UNIXCB(skb).fp)
+				break;
+
+			/* XXX - this is ugly; a better approach would be
+			 * rewriting this function
+			 */
+			last = skb;
+			last_len = skb->len;
+			unix_state_lock(sk);
+			skb = skb_peek_next(skb, &sk->sk_receive_queue);
+			if (skb)
+				goto again;
+			goto unlock;
 		}
 	} while (size);
 
-- 
1.9.1

Re: [PATCH v3 2/2] [net] af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag

[David Miller]

From: Aaron Conole <aconole@bytheb.org>
Date: Mon, 21 Sep 2015 20:27:02 -0400

> +			goto unlock;

Sorry, I don't want to see goto's from one loop into a completely
different one.

The XXX comment is probably not appropriate either.

Re: [PATCH v3 2/2] [net] af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag

[Aaron Conole]

Thanks for your time and the review.

David Miller <davem@davemloft.net> writes:
> From: Aaron Conole <aconole@bytheb.org>
> Date: Mon, 21 Sep 2015 20:27:02 -0400
>
>> +			goto unlock;
>
> Sorry, I don't want to see goto's from one loop into a completely
> different one.

I think you misread. This goto does not do that; it goes from the else 
side of one if block, into a different if block within the same do/while
pair. I assumed this was okay, because in 
commit:b48732e4a48d80ed4a14812f0bab09560846514e the same type of goto
unlock jump is used. Since that commit was accepted 4 months ago, I
figured the style hadn't changed. 

Would you rather see:
+			unix_state_unlock(sk);
+			break;

> The XXX comment is probably not appropriate either.

Agreed, any v4 post will not include it.

[PATCH v4 0/2] [net] af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag

[Aaron Conole]

This patch set implements a bugfix for kernel.org bugzilla #12323, allowing
MSG_PEEK to return all queued data on the unix domain socket, not just the
data contained in a single SKB. 

This is the v3 version of this patch, which includes a suggested modification
by Eric Dumazet to convert the unix_sk() conversion macro to a static inline
function. These patches are independent and can be applied separately.

This set was tested over a 24-hour period, utilizing a loop continually 
executing the bugzilla issue attached python code. It was instrumented with
a pr_err_once() ([   13.798683] unix: went there at least one time).

v2->v3:
 - Added Eric Dumazet's suggestion for #define to static inline
 - Fixed an issue calling unix_state_lock() with an invalid argument

v3->v4:
 - Eliminated an XXX comment
 - Changed from goto unlock to explicit unix_state_unlock() and break

Aaron Conole (2):
  [net] af_unix: Convert the unix_sk macro to an inline function for    
    type safety
  [net] af_unix: return data from multiple SKBs on recv() with MSG_PEEK
    flag

 include/net/af_unix.h |  6 +++++-
 net/unix/af_unix.c    | 19 +++++++++++++++++--
 2 files changed, 22 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH v4 1/2] [net] af_unix: Convert the unix_sk macro to an inline function for type safety

[Aaron Conole]

As suggested by Eric Dumazet this change replaces the 
#define with a static inline function to enjoy 
complaints by the compiler when misusing the API.

Signed-off-by: Aaron Conole <aconole@bytheb.org>
---
 include/net/af_unix.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 4a167b3..cb1b9bb 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -63,7 +63,11 @@ struct unix_sock {
 #define UNIX_GC_MAYBE_CYCLE	1
 	struct socket_wq	peer_wq;
 };
-#define unix_sk(__sk) ((struct unix_sock *)__sk)
+
+static inline struct unix_sock *unix_sk(struct sock *sk)
+{
+	return (struct unix_sock *)sk;
+}
 
 #define peer_wait peer_wq.wait
 
-- 
1.9.1

[PATCH v4 2/2] [net] af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag

[Aaron Conole]

AF_UNIX sockets now return multiple skbs from recv() when MSG_PEEK flag
is set.

This is referenced in kernel bugzilla #12323 @
https://bugzilla.kernel.org/show_bug.cgi?id=12323

As described both in the BZ and lkml thread @
http://lkml.org/lkml/2008/1/8/444 calling recv() with MSG_PEEK on an
AF_UNIX socket only reads a single skb, where the desired effect is
to return as much skb data has been queued, until hitting the recv
buffer size (whichever comes first).

The modified MSG_PEEK path will now move to the next skb in the tree
and jump to the again: label, rather than following the natural loop
structure. This requires duplicating some of the loop head actions.

This was tested using the python socketpair python code attached to
the bugzilla issue.

Signed-off-by: Aaron Conole <aconole@bytheb.org>
---
 net/unix/af_unix.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 03ee4d3..ef31b40 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2179,8 +2179,21 @@ unlock:
 			if (UNIXCB(skb).fp)
 				scm.fp = scm_fp_dup(UNIXCB(skb).fp);
 
-			sk_peek_offset_fwd(sk, chunk);
+			if (skip) {
+				sk_peek_offset_fwd(sk, chunk);
+				skip -= chunk;
+			}
 
+			if (UNIXCB(skb).fp)
+				break;
+
+			last = skb;
+			last_len = skb->len;
+			unix_state_lock(sk);
+			skb = skb_peek_next(skb, &sk->sk_receive_queue);
+			if (skb)
+				goto again;
+			unix_state_unlock(sk);
 			break;
 		}
 	} while (size);
-- 
1.9.1

Re: [PATCH v4 0/2] [net] af_unix: return data from multiple SKBs on recv() with MSG_PEEK flag

[David Miller]

From: Aaron Conole <aconole@bytheb.org>
Date: Sat, 26 Sep 2015 18:50:41 -0400

> This patch set implements a bugfix for kernel.org bugzilla #12323, allowing
> MSG_PEEK to return all queued data on the unix domain socket, not just the
> data contained in a single SKB. 
> 
> This is the v3 version of this patch, which includes a suggested modification
> by Eric Dumazet to convert the unix_sk() conversion macro to a static inline
> function. These patches are independent and can be applied separately.
> 
> This set was tested over a 24-hour period, utilizing a loop continually 
> executing the bugzilla issue attached python code. It was instrumented with
> a pr_err_once() ([   13.798683] unix: went there at least one time).
> 
> v2->v3:
>  - Added Eric Dumazet's suggestion for #define to static inline
>  - Fixed an issue calling unix_state_lock() with an invalid argument
> 
> v3->v4:
>  - Eliminated an XXX comment
>  - Changed from goto unlock to explicit unix_state_unlock() and break

Series applied, thanks.