linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] af_unix: fix garbage collect vs. MSG_PEEK
@ 2016-09-29 12:09 Miklos Szeredi
  2016-10-04  1:51 ` David Miller
  0 siblings, 1 reply; 3+ messages in thread
From: Miklos Szeredi @ 2016-09-29 12:09 UTC (permalink / raw)
  To: netdev; +Cc: linux-kernel, hannes, kernel, davem

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

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] af_unix: fix garbage collect vs. MSG_PEEK
  2016-09-29 12:09 [PATCH] af_unix: fix garbage collect vs. MSG_PEEK Miklos Szeredi
@ 2016-10-04  1:51 ` David Miller
  2016-12-19 11:21   ` Miklos Szeredi
  0 siblings, 1 reply; 3+ messages in thread
From: David Miller @ 2016-10-04  1:51 UTC (permalink / raw)
  To: mszeredi; +Cc: netdev, linux-kernel, hannes, kernel

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.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] af_unix: fix garbage collect vs. MSG_PEEK
  2016-10-04  1:51 ` David Miller
@ 2016-12-19 11:21   ` Miklos Szeredi
  0 siblings, 0 replies; 3+ messages in thread
From: Miklos Szeredi @ 2016-12-19 11:21 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, lkml, Hannes Frederic Sowa, Nikolay Borisov

On Tue, Oct 4, 2016 at 3:51 AM, David Miller <davem@davemloft.net> wrote:
> 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.

Missed this mail, sorry for the late reply...

AFAICS wait_for_unix_gc() lacks a barrier that the spin lock provides.

The ordering needs to be strictly:

A: duplicate file pointers
B: garbage collect

Also wait_for_unix_gc() is an overkill since the complete garbage
collect (including purging the trash) will take longer than the
collection stage, which we need to have ordering with.  This probably
doesn't matter much in practice.

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2016-12-19 11:21 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-29 12:09 [PATCH] af_unix: fix garbage collect vs. MSG_PEEK Miklos Szeredi
2016-10-04  1:51 ` David Miller
2016-12-19 11:21   ` Miklos Szeredi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).