netdev - Re: [PATCH v2] [net] af_unix: return data from multiple SKBs on recv() with MSG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1442767564.29850.35.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Sun, 20 Sep 2015 09:46:04 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Aaron Conole <aconole@...heb.org>
Cc:	netdev <netdev@...r.kernel.org>, Aaron Conole <aaron@...heb.org>
Subject: Re: [PATCH v2] [net] af_unix: return data from multiple SKBs on
 recv() with MSG_PEEK flag

On Sun, 2015-09-20 at 05:18 -0400, Aaron Conole wrote:
> From: Aaron Conole <aaron@...heb.org>
> 
> 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 <aaron@...heb.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..988fbbd4 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);

I am wondering what this is expected to do, and how this code would
possibly not trigger a crash.

Are you 100% sure you tested this patch and code path ?

Before resending v3, please make sure to compile and test with
CONFIG_LOCKDEP=y. Add a temporary (in your tree, not final patch)

pr_err_once("went there at least one time\n");

(to make sure this code path was tested)

It might be time to get rid of unix_sk macro for a proper function to
avoid these kind of errors.

diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 4a167b30a12f..cb1b9bbda332 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
 

Thanks.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html