Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1493427 > unrolled thread

[PATCH] af_unix: fix garbage collect vs. MSG_PEEK

Back to article view | Back to linux.kernel

Contents

  [PATCH] af_unix: fix garbage collect vs. MSG_PEEK Miklos Szeredi <mszeredi@redhat.com> - 2016-09-29 14:10 +0200
    Re: [PATCH] af_unix: fix garbage collect vs. MSG_PEEK David Miller <davem@davemloft.net> - 2016-10-04 04:00 +0200

#1493427 — [PATCH] af_unix: fix garbage collect vs. MSG_PEEK

Gc assumes that in-flight sockets that don't have an external ref can't
gain one while unix_gc_lock is held.  That is true because
unix_notinflight() will be called before detaching fds, which takes
unix_gc_lock.

Only MSG_PEEK was somehow overlooked.  That one also clones the fds, also
keeping them in the skb.  But through MSG_PEEK an external reference can
definitely be gained without ever touching unix_gc_lock.

This patch adds unix_gc_barrier() that waits for a garbage collect run to
finish (if there is one), before actually installing the peeked in-flight
files to file descriptors.  This prevents problems from a pure in-flight
socket having its buffers modified while the garbage collect is taking
place.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Cc: <stable@vger.kernel.org>
---
 include/net/af_unix.h |  1 +
 net/unix/af_unix.c    | 15 +++++++++++++--
 net/unix/garbage.c    |  5 +++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index fd60eccb59a6..f73ce09da2b4 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -10,6 +10,7 @@ void unix_inflight(struct user_struct *user, struct file *fp);
 void unix_notinflight(struct user_struct *user, struct file *fp);
 void unix_gc(void);
 void wait_for_unix_gc(void);
+void unix_gc_barrier(void);
 struct sock *unix_get_socket(struct file *filp);
 struct sock *unix_peer_get(struct sock *);
 
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 8309687a56b0..cece344b41b8 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1550,6 +1550,17 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
 	return max_level;
 }
 
+static void unix_peek_fds(struct scm_cookie *scm, struct sk_buff *skb)
+{
+	scm->fp = scm_fp_dup(UNIXCB(skb).fp);
+	/*
+	 * During garbage collection it is assumed that in-flight sockets don't
+	 * get a new external reference.  So we need to wait until current run
+	 * finishes.
+	 */
+	unix_gc_barrier();
+}
+
 static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool send_fds)
 {
 	int err = 0;
@@ -2182,7 +2193,7 @@ static int unix_dgram_recvmsg(struct socket *sock, struct msghdr *msg,
 		sk_peek_offset_fwd(sk, size);
 
 		if (UNIXCB(skb).fp)
-			scm.fp = scm_fp_dup(UNIXCB(skb).fp);
+			unix_peek_fds(&scm, skb);
 	}
 	err = (flags & MSG_TRUNC) ? skb->len - skip : size;
 
@@ -2422,7 +2433,7 @@ unlock:
 			/* It is questionable, see note in unix_dgram_recvmsg.
 			 */
 			if (UNIXCB(skb).fp)
-				scm.fp = scm_fp_dup(UNIXCB(skb).fp);
+				unix_peek_fds(&scm, skb);
 
 			sk_peek_offset_fwd(sk, chunk);
 
diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index 6a0d48525fcf..d4fb6ff8024d 100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -266,6 +266,11 @@ void wait_for_unix_gc(void)
 	wait_event(unix_gc_wait, gc_in_progress == false);
 }
 
+void unix_gc_barrier(void)
+{
+	spin_unlock_wait(&unix_gc_lock);
+}
+
 /* The external entry point: unix_gc() */
 void unix_gc(void)
 {
-- 
2.5.5

[toc] | [next] | [standalone]

#1495139

From: Miklos Szeredi <mszeredi@redhat.com>
Date: Thu, 29 Sep 2016 14:09:14 +0200

> @@ -1550,6 +1550,17 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
>  	return max_level;
>  }
>  
> +static void unix_peek_fds(struct scm_cookie *scm, struct sk_buff *skb)
> +{
> +	scm->fp = scm_fp_dup(UNIXCB(skb).fp);
> +	/*
> +	 * During garbage collection it is assumed that in-flight sockets don't
> +	 * get a new external reference.  So we need to wait until current run
> +	 * finishes.
> +	 */
> +	unix_gc_barrier();
> +}
 ...
> @@ -266,6 +266,11 @@ void wait_for_unix_gc(void)
>  	wait_event(unix_gc_wait, gc_in_progress == false);
>  }
>  
> +void unix_gc_barrier(void)
> +{
> +	spin_unlock_wait(&unix_gc_lock);
> +}

Can you explain why wait_for_unix_gc() isn't appropriate?  I'm a little
bit uncomfortable with a spinlock wait like this, and would rather see
something like the existing helper used.

Thanks.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web