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

Groups > linux.kernel > #1226631 > unrolled thread

Possible netlink autobind regression

Back to article view | Back to linux.kernel

Contents

  Possible netlink autobind regression Tejun Heo <tj@kernel.org> - 2015-09-17 04:30 +0200
    Re: Possible netlink autobind regression Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-17 05:10 +0200
      Re: Possible netlink autobind regression Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-17 05:50 +0200
        Re: Possible netlink autobind regression Cong Wang <cwang@twopensource.com> - 2015-09-17 07:10 +0200
          Re: Possible netlink autobind regression Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-17 07:20 +0200
            Re: Possible netlink autobind regression Tejun Heo <tj@kernel.org> - 2015-09-17 13:40 +0200
              [PATCH v3] netlink: Fix autobind race condition that leads to zero  port ID Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-18 08:40 +0200
                [PATCH v4] netlink: Fix autobind race condition that leads to zero  port ID Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-18 13:20 +0200
                  Re: [PATCH v4] netlink: Fix autobind race condition that leads to  zero port ID David Miller <davem@davemloft.net> - 2015-09-21 08:00 +0200
                    Re: [PATCH v4] netlink: Fix autobind race condition that leads to  zero port ID Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-21 08:10 +0200
                      Re: [PATCH v4] netlink: Fix autobind race condition that leads to  zero port ID David Miller <davem@davemloft.net> - 2015-09-21 08:20 +0200
                        netlink: Replace rhash_portid with bound Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-21 15:40 +0200
                          Re: netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-21 20:30 +0200
                            [PATCH v2] netlink: Replace rhash_portid with bound Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-22 05:40 +0200
                              Re: [PATCH v2] netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-22 18:20 +0200
                                Re: [PATCH v2] netlink: Replace rhash_portid with bound Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-22 20:50 +0200
                                  Re: [PATCH v2] netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-22 21:00 +0200
                                    Re: [PATCH v2] netlink: Replace rhash_portid with bound Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-22 21:30 +0200
                                      Re: [PATCH v2] netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-22 22:00 +0200
                                        Re: [PATCH v2] netlink: Replace rhash_portid with bound Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-22 22:10 +0200
                                          Re: [PATCH v2] netlink: Replace rhash_portid with bound Bjørn Mork <bjorn@mork.no> - 2015-09-22 22:50 +0200
                                            Re: [PATCH v2] netlink: Replace rhash_portid with bound Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-22 23:10 +0200
                                Re: [PATCH v2] netlink: Replace rhash_portid with bound Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-23 08:20 +0200
                                  Re: [PATCH v2] netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-23 18:00 +0200
                                    Re: [PATCH v2] netlink: Replace rhash_portid with bound Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-24 04:40 +0200
                                      Re: [PATCH v2] netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-24 04:50 +0200
                                        Re: [PATCH v2] netlink: Replace rhash_portid with bound Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-24 05:00 +0200
                                          Re: [PATCH v2] netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-24 05:10 +0200
                                            Re: [PATCH v2] netlink: Replace rhash_portid with bound Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-24 05:30 +0200
                                              Re: [PATCH v2] netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-24 05:30 +0200
                                                Re: [PATCH v2] netlink: Replace rhash_portid with bound Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-24 05:40 +0200
                                                  Re: [PATCH v2] netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-24 05:50 +0200
                                                    Re: [PATCH v2] netlink: Replace rhash_portid with bound Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-24 05:50 +0200
                                                  Re: [PATCH v2] netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-24 05:50 +0200
                                                    Re: [PATCH v2] netlink: Replace rhash_portid with bound Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-24 05:50 +0200
                              Re: [PATCH v2] netlink: Replace rhash_portid with bound David Miller <davem@davemloft.net> - 2015-09-24 21:20 +0200
                                Re: [PATCH v2] netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-24 22:10 +0200
                                  netlink: Add barrier to netlink_connect for theoretical case Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-25 03:50 +0200
                                    Re: netlink: Add barrier to netlink_connect for theoretical case Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-25 05:30 +0200
                                      Re: netlink: Add barrier to netlink_connect for theoretical case Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-25 05:50 +0200
                                        Re: netlink: Add barrier to netlink_connect for theoretical case Tejun Heo <tj@kernel.org> - 2015-09-25 17:10 +0200
                                    Re: netlink: Add barrier to netlink_connect for theoretical case Tejun Heo <tj@kernel.org> - 2015-09-25 17:10 +0200
                                      netlink: Add netlink_bound helper and use it in netlink_getname Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-26 15:20 +0200
                                        Re: netlink: Add netlink_bound helper and use it in netlink_getname Tejun Heo <tj@kernel.org> - 2015-09-26 20:10 +0200
                                          Re: netlink: Add netlink_bound helper and use it in netlink_getname Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-26 21:50 +0200
                                            Re: netlink: Add netlink_bound helper and use it in netlink_getname Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-26 22:00 +0200
                                              Re: netlink: Add netlink_bound helper and use it in netlink_getname Tejun Heo <tj@kernel.org> - 2015-09-26 22:10 +0200
                                                Re: netlink: Add netlink_bound helper and use it in netlink_getname Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-26 22:20 +0200
                                                  Re: netlink: Add netlink_bound helper and use it in netlink_getname Tejun Heo <tj@kernel.org> - 2015-09-26 22:20 +0200
                                            Re: netlink: Add netlink_bound helper and use it in netlink_getname Tejun Heo <tj@kernel.org> - 2015-09-26 22:00 +0200
                                          Re: netlink: Add netlink_bound helper and use it in netlink_getname Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-26 21:50 +0200
                                            Re: netlink: Add netlink_bound helper and use it in netlink_getname Tejun Heo <tj@kernel.org> - 2015-09-26 21:50 +0200
                          [PATCH] netlink: Replace rhash_portid with load_acquire protected  boolean Tejun Heo <tj@kernel.org> - 2015-09-21 23:00 +0200
                Re: [PATCH v3] netlink: Fix autobind race condition that leads to  zero port ID Tejun Heo <tj@kernel.org> - 2015-09-18 15:40 +0200
            Re: Possible netlink autobind regression Thomas Graf <tgraf@suug.ch> - 2015-09-17 13:50 +0200

Page 3 of 3 — ← Prev page 1 2 [3]

#1232870 — Re: netlink: Add barrier to netlink_connect for theoretical case

Hello, Herbert.

On Fri, Sep 25, 2015 at 11:39:57AM +0800, Herbert Xu wrote:
> +static inline bool netlink_bound(struct netlink_sock *nlk)
> +{
> +	bool bound = READ_ONCE(nlk->bound);
> +
> +	/* Ensure nlk is hashed and visible. */
> +	if (bound)
> +		smp_rmb();
> +
> +	return bound;
> +}

While I can't see anything wrong with the above, I'm not a fan of it
for whatever worth that may be.  I don't think it adds anything in
terms of readability or clarity of the code.  It does avoid smp_rmb()
when @bound is false but that's unlikely to be helfpul - where the
barrier is being avoided is a cold path.  This is largely a generic
characteristic because if where the barrier is being avoided is a hot
path, why wouldn't the code just grab a lock in that path instead of
using a gated barrier?  So, there's a reason why we don't see code
like the above commonly.  It doesn't buy us anything meaningful while
making the code more complicated and sometimes more fragile.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232872 — Re: netlink: Add barrier to netlink_connect for theoretical case

Hello, Herbert.

On Fri, Sep 25, 2015 at 09:43:27AM +0800, Herbert Xu wrote:
> Well had you said this in the first place I would've fixed it a
> long time ago.  There aren't any in-kernel users right now and
> even if there were they'd have to do a connect/bind/sendmsg on
> the same socket in two threads at the same time.  But let's close
> this theoretical hole:

I'm not even sure we guarantee memory barrier on kernel/user
crossings.  In practice, we probably have enough barriers (e.g. some
syscall traps imply barrier) but I can't think of a reason why we'd
guarantee the existence of barrier there.  As an extreme example,
imagine UML on an architecture with relaxed memory model.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233199 — netlink: Add netlink_bound helper and use it in netlink_getname

On Fri, Sep 25, 2015 at 11:01:13AM -0400, Tejun Heo wrote:
> 
> I'm not even sure we guarantee memory barrier on kernel/user
> crossings.  In practice, we probably have enough barriers (e.g. some
> syscall traps imply barrier) but I can't think of a reason why we'd
> guarantee the existence of barrier there.  As an extreme example,
> imagine UML on an architecture with relaxed memory model.

You misunderstood what I wrote.  I was not basing this on whether
user-space transitions contained a barrier, but on the fact that
the next syscall must recheck nlk->bound before using nlk->portid.

In fact thanks to your email I now realise that my fix to the
getsockname problem is wrong.  Instead of adding a barrier to
netlink_connect I should be adding a nlk->bound check to getname.

---8<---
netlink_getname must check nlk->bound before using nlk->portid
as otherwise nlk->portid may contain garbage.

This patch also adds a netlink_bound helper that encapsulate the
barrier, as was suggested by Tejun.  Also as suggested by Linus,
the lockless read of nlk->bound in netlink_bound is protected with
READ_ONCE to ensure that the compiler doesn't do double reads that
may screw up our use of the barrier.

Fixes: da314c9923fe ("netlink: Replace rhash_portid with bound")
Reported-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 2c15fae..c860464 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -125,6 +125,17 @@ static inline u32 netlink_group_mask(u32 group)
 	return group ? 1 << (group - 1) : 0;
 }
 
+static inline bool netlink_bound(struct netlink_sock *nlk)
+{
+	bool bound = READ_ONCE(nlk->bound);
+
+	/* Ensure nlk is hashed and visible. */
+	if (bound)
+		smp_rmb();
+
+	return bound;
+}
+
 int netlink_add_tap(struct netlink_tap *nt)
 {
 	if (unlikely(nt->dev->type != ARPHRD_NETLINK))
@@ -1524,14 +1535,10 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 			return err;
 	}
 
-	bound = nlk->bound;
-	if (bound) {
-		/* Ensure nlk->portid is up-to-date. */
-		smp_rmb();
-
+	bound = netlink_bound(nlk);
+	if (bound)
 		if (nladdr->nl_pid != nlk->portid)
 			return -EINVAL;
-	}
 
 	if (nlk->netlink_bind && groups) {
 		int group;
@@ -1547,9 +1554,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 		}
 	}
 
-	/* No need for barriers here as we return to user-space without
-	 * using any of the bound attributes.
-	 */
 	if (!bound) {
 		err = nladdr->nl_pid ?
 			netlink_insert(sk, nladdr->nl_pid) :
@@ -1628,7 +1632,7 @@ static int netlink_getname(struct socket *sock, struct sockaddr *addr,
 		nladdr->nl_pid = nlk->dst_portid;
 		nladdr->nl_groups = netlink_group_mask(nlk->dst_group);
 	} else {
-		nladdr->nl_pid = nlk->portid;
+		nladdr->nl_pid = netlink_bound(nlk) ? nlk->portid : 0;
 		nladdr->nl_groups = nlk->groups ? nlk->groups[0] : 0;
 	}
 	return 0;
@@ -2442,13 +2446,10 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
 		dst_group = nlk->dst_group;
 	}
 
-	if (!nlk->bound) {
+	if (!netlink_bound(nlk)) {
 		err = netlink_autobind(sock);
 		if (err)
 			goto out;
-	} else {
-		/* Ensure nlk is hashed and visible. */
-		smp_rmb();
 	}
 
 	/* It's a really convoluted way for userland to ask for mmaped
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233255 — Re: netlink: Add netlink_bound helper and use it in netlink_getname

Hello, Herbert.

On Sat, Sep 26, 2015 at 09:16:21PM +0800, Herbert Xu wrote:
> You misunderstood what I wrote.  I was not basing this on whether
> user-space transitions contained a barrier, but on the fact that
> the next syscall must recheck nlk->bound before using nlk->portid.

But that isn't what you wrote in the comment.

	/* No need for barriers here as we return to user-space without
	 * using any of the bound attributes.
	 */

> In fact thanks to your email I now realise that my fix to the
> getsockname problem is wrong.  Instead of adding a barrier to
> netlink_connect I should be adding a nlk->bound check to getname.

I don't know, man.  This thread almost feels surreal at this point.

> @@ -1628,7 +1632,7 @@ static int netlink_getname(struct socket *sock, struct sockaddr *addr,
>  		nladdr->nl_pid = nlk->dst_portid;
>  		nladdr->nl_groups = netlink_group_mask(nlk->dst_group);
>  	} else {
> -		nladdr->nl_pid = nlk->portid;
> +		nladdr->nl_pid = netlink_bound(nlk) ? nlk->portid : 0;
>  		nladdr->nl_groups = nlk->groups ? nlk->groups[0] : 0;
>  	}
>  	return 0;

So, this is really weird because netlink_getname() doens't participate
in the autobind race and thus it's perfectly fine for it to not worry
about whether ->bound is set or the memory barrier - whoever its
caller may be, the caller is of course responsible for ensuring that
the port is bound and visible if it expects to read back the number -
ie. if the caller doesn't know (in memory ordering sense) that
bind/connect/sendmsg succeeded, it of course can't expect to reliably
read back the port number.  getname never needed the barrier.  The
above is shifting synchronization from the source to its users.  This
is a bad thing to do.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233275 — Re: netlink: Add netlink_bound helper and use it in netlink_getname

On Sat, Sep 26, 2015 at 03:45:54PM -0400, Tejun Heo wrote:
> Hello, Herbert.
> 
> On Sun, Sep 27, 2015 at 03:41:10AM +0800, Herbert Xu wrote:
> > Thread 1			Thread 2
> > sendmsg				getsockname
> > 	netlink_autobind		netlink_getname
> > 
> > Thread 2 should not have to do anything special to guarantee that
> > getsockname does not return garbage.  It must either be the bound
> > portid if the autobind completed in thread 1 and is visible or it
> > should return zero.
> > 
> > As it stands thread 2 may see a portid belonging to somebody else
> > if it catches the autobind in thread 1 trying different portids
> > while roving.
> 
> If the fact that thread 1 finished autobind isn't visible to thread 2,
> it's valid for getsockname to return zero.  No ordering between the
> two operations is defined.  If the fact that thread 1 finished
> autobind is visible to thread 2, ordering is defined and because
> ordering is transitive, by that very ordering, the port number is
> visible to thread 2 too as long as thread 1 does proper barriering.

If the autobind is not complete then netlink_getname must return
zero rather than some garbage portid that belongs to somebody
else's socket.  That's what we did before any of this lockless
code was introduced.

If you don't check nlk->bound then you may return garbage.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233279 — Re: netlink: Add netlink_bound helper and use it in netlink_getname

On Sat, Sep 26, 2015 at 03:52:45PM -0400, Tejun Heo wrote:
> 
> Ah, yeah, you're right.  We need to check that there because it may
> contain a garbage value.  I still think it'd better to use
> netlink_bound() test in connect() too tho.

Well I disagree.  When I say that it returns to user-space I really
mean that the next time we use portid via the same call path that
triggered the connect we must be checking nlk->bound anyway.

Good luck finding more bugs in this code :)

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233289 — Re: netlink: Add netlink_bound helper and use it in netlink_getname

Hello,

On Sun, Sep 27, 2015 at 03:55:48AM +0800, Herbert Xu wrote:
> Well I disagree.  When I say that it returns to user-space I really
> mean that the next time we use portid via the same call path that
> triggered the connect we must be checking nlk->bound anyway.
> 
> Good luck finding more bugs in this code :)

Frankly, I don't understand what you've been trying to achieve.
You're actively disregarding best practices (like terminating
synchronization where it starts) and reach the target state by doing a
browian motion in the solution space.  Sure, if you do enough of that,
eventually you can arrive somewhere where it's not broken but it leads
to a lot more overhead for everyone involved - the author, reviewers
and later readers of the code and if you spread barrier usages like
this across the kernel, we'll end up with code base which is a lot
harder to verify and maintain.  I hope you stop doing things this way
but suppose that you're ignoring any conceptual arguments in this
thread.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233292 — Re: netlink: Add netlink_bound helper and use it in netlink_getname

On Sat, Sep 26, 2015 at 04:05:18PM -0400, Tejun Heo wrote:
> 
> Frankly, I don't understand what you've been trying to achieve.
> You're actively disregarding best practices (like terminating
> synchronization where it starts) and reach the target state by doing a
> browian motion in the solution space.  Sure, if you do enough of that,
> eventually you can arrive somewhere where it's not broken but it leads
> to a lot more overhead for everyone involved - the author, reviewers
> and later readers of the code and if you spread barrier usages like
> this across the kernel, we'll end up with code base which is a lot
> harder to verify and maintain.  I hope you stop doing things this way
> but suppose that you're ignoring any conceptual arguments in this
> thread.

Your point so far has been that if you add barriers or use primitives
everywhere then races magically disappear.

Well guess what the bug that you have discovered supposedly due to
a missing barrier in netlink_connect has nothing to do with the
barrier.  Instead it is caused by a logical error elsewhere that
would have gone unnoticed otherwise.

So I retain my position that blindly adding barriers do not make
bugs go away.  Instead you need to have real understanding of what
the code is doing and every spot where a barrier may be needed must
be audited manually.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233294 — Re: netlink: Add netlink_bound helper and use it in netlink_getname

Hello,

On Sun, Sep 27, 2015 at 04:10:41AM +0800, Herbert Xu wrote:
> Well guess what the bug that you have discovered supposedly due to
> a missing barrier in netlink_connect has nothing to do with the
> barrier.  Instead it is caused by a logical error elsewhere that
> would have gone unnoticed otherwise.

It's a combination of two problems.  The garbage port number is a
logical error but there still is an ordering problem there between
->bound and ->portid.  We need to test ->bind there again because of
the garbage port problem.

> So I retain my position that blindly adding barriers do not make
> bugs go away.  Instead you need to have real understanding of what

That's a dishonest summary of what I've been saying.

> the code is doing and every spot where a barrier may be needed must
> be audited manually.

What I've been saying is that we do need to be careful and audit each
barrier usages but at the same time there are established patterns
that we can use to make the process significantly easier and more
reliable.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233284 — Re: netlink: Add netlink_bound helper and use it in netlink_getname

Hello,

On Sun, Sep 27, 2015 at 03:49:16AM +0800, Herbert Xu wrote:
> If the autobind is not complete then netlink_getname must return
> zero rather than some garbage portid that belongs to somebody
> else's socket.  That's what we did before any of this lockless
> code was introduced.
> 
> If you don't check nlk->bound then you may return garbage.

Ah, yeah, you're right.  We need to check that there because it may
contain a garbage value.  I still think it'd better to use
netlink_bound() test in connect() too tho.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233277 — Re: netlink: Add netlink_bound helper and use it in netlink_getname

On Sat, Sep 26, 2015 at 02:09:03PM -0400, Tejun Heo wrote:
>
> > @@ -1628,7 +1632,7 @@ static int netlink_getname(struct socket *sock, struct sockaddr *addr,
> >  		nladdr->nl_pid = nlk->dst_portid;
> >  		nladdr->nl_groups = netlink_group_mask(nlk->dst_group);
> >  	} else {
> > -		nladdr->nl_pid = nlk->portid;
> > +		nladdr->nl_pid = netlink_bound(nlk) ? nlk->portid : 0;
> >  		nladdr->nl_groups = nlk->groups ? nlk->groups[0] : 0;
> >  	}
> >  	return 0;
> 
> So, this is really weird because netlink_getname() doens't participate
> in the autobind race and thus it's perfectly fine for it to not worry
> about whether ->bound is set or the memory barrier - whoever its
> caller may be, the caller is of course responsible for ensuring that
> the port is bound and visible if it expects to read back the number -
> ie. if the caller doesn't know (in memory ordering sense) that
> bind/connect/sendmsg succeeded, it of course can't expect to reliably
> read back the port number.  getname never needed the barrier.  The
> above is shifting synchronization from the source to its users.  This
> is a bad thing to do.

Thread 1			Thread 2
sendmsg				getsockname
	netlink_autobind		netlink_getname

Thread 2 should not have to do anything special to guarantee that
getsockname does not return garbage.  It must either be the bound
portid if the autobind completed in thread 1 and is visible or it
should return zero.

As it stands thread 2 may see a portid belonging to somebody else
if it catches the autobind in thread 1 trying different portids
while roving.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233278 — Re: netlink: Add netlink_bound helper and use it in netlink_getname

Hello, Herbert.

On Sun, Sep 27, 2015 at 03:41:10AM +0800, Herbert Xu wrote:
> Thread 1			Thread 2
> sendmsg				getsockname
> 	netlink_autobind		netlink_getname
> 
> Thread 2 should not have to do anything special to guarantee that
> getsockname does not return garbage.  It must either be the bound
> portid if the autobind completed in thread 1 and is visible or it
> should return zero.
> 
> As it stands thread 2 may see a portid belonging to somebody else
> if it catches the autobind in thread 1 trying different portids
> while roving.

If the fact that thread 1 finished autobind isn't visible to thread 2,
it's valid for getsockname to return zero.  No ordering between the
two operations is defined.  If the fact that thread 1 finished
autobind is visible to thread 2, ordering is defined and because
ordering is transitive, by that very ordering, the port number is
visible to thread 2 too as long as thread 1 does proper barriering.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229734 — [PATCH] netlink: Replace rhash_portid with load_acquire protected boolean

Hello,

Here's an updated version of Herbert's patch which always uses
load_acquire through a helper.

Thanks.
----- 8< -----
The commit 1f770c0a09da855a2b51af6d19de97fb955eca85 ("netlink: Fix
autobind race condition that leads to zero port ID") created some new
races that can occur due to inconsistencies between the two port IDs -
a reader may see zero nlk->portid after seeing non-zero
nlk->rhash_portid.

This patch reverts the original patch and instead uses a load_acquire
protected boolean to indicate that a user netlink socket has been
bound.  The boolean is set with store_release only after the portid is
assigned and the socket is hashed.  The readers test with load_acquire
so that the socket is guaranteed to be visible with a valid port
number and hashed on a true return.

As this sort of lockless tests can be broken in ways which are very
difficult to track down, the boolean field is prefixed with double
underscores and a dedicated test helper with load_acquire is always
used.  While a couple test sites might not strictly require
load_acquire, micro-optimization at this level doens't make sense
given the danger of subtle breakages.

tj: Took Herbert's patch and updated so that all readers test via a
    helper which does load_acquire.

Fixes: 1f770c0a09da ("netlink: Fix autobind race condition that leads to zero port ID")
Reported-by: Tejun Heo <tj@kernel.org>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Original-patch-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Tejun Heo <tj@kernel.org>
---
 net/netlink/af_netlink.c |   24 ++++++++++++++----------
 net/netlink/af_netlink.h |   20 +++++++++++++++++++-
 2 files changed, 33 insertions(+), 11 deletions(-)

--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1015,7 +1015,7 @@ static inline int netlink_compare(struct
 	const struct netlink_compare_arg *x = arg->key;
 	const struct netlink_sock *nlk = ptr;
 
-	return nlk->rhash_portid != x->portid ||
+	return nlk->portid != x->portid ||
 	       !net_eq(sock_net(&nlk->sk), read_pnet(&x->pnet));
 }
 
@@ -1041,7 +1041,7 @@ static int __netlink_insert(struct netli
 {
 	struct netlink_compare_arg arg;
 
-	netlink_compare_arg_init(&arg, sock_net(sk), nlk_sk(sk)->rhash_portid);
+	netlink_compare_arg_init(&arg, sock_net(sk), nlk_sk(sk)->portid);
 	return rhashtable_lookup_insert_key(&table->hash, &arg,
 					    &nlk_sk(sk)->node,
 					    netlink_rhashtable_params);
@@ -1095,7 +1095,7 @@ static int netlink_insert(struct sock *s
 	lock_sock(sk);
 
 	err = -EBUSY;
-	if (nlk_sk(sk)->portid)
+	if (nlk_bound(nlk_sk(sk)))
 		goto err;
 
 	err = -ENOMEM;
@@ -1103,7 +1103,7 @@ static int netlink_insert(struct sock *s
 	    unlikely(atomic_read(&table->hash.nelems) >= UINT_MAX))
 		goto err;
 
-	nlk_sk(sk)->rhash_portid = portid;
+	nlk_sk(sk)->portid = portid;
 	sock_hold(sk);
 
 	err = __netlink_insert(table, sk);
@@ -1119,7 +1119,8 @@ static int netlink_insert(struct sock *s
 		goto err;
 	}
 
-	nlk_sk(sk)->portid = portid;
+	/* See nlk_bound(). */
+	smp_store_release(&nlk_sk(sk)->__bound, portid);
 
 err:
 	release_sock(sk);
@@ -1521,9 +1522,11 @@ static int netlink_bind(struct socket *s
 			return err;
 	}
 
-	if (nlk->portid)
+	/* Ensure nlk->portid is up-to-date. */
+	if (nlk_bound(nlk)) {
 		if (nladdr->nl_pid != nlk->portid)
 			return -EINVAL;
+	}
 
 	if (nlk->netlink_bind && groups) {
 		int group;
@@ -1539,7 +1542,7 @@ static int netlink_bind(struct socket *s
 		}
 	}
 
-	if (!nlk->portid) {
+	if (!nlk_bound(nlk)) {
 		err = nladdr->nl_pid ?
 			netlink_insert(sk, nladdr->nl_pid) :
 			netlink_autobind(sock);
@@ -1587,7 +1590,7 @@ static int netlink_connect(struct socket
 	    !netlink_allowed(sock, NL_CFG_F_NONROOT_SEND))
 		return -EPERM;
 
-	if (!nlk->portid)
+	if (!nlk_bound(nlk))
 		err = netlink_autobind(sock);
 
 	if (err == 0) {
@@ -2428,7 +2431,8 @@ static int netlink_sendmsg(struct socket
 		dst_group = nlk->dst_group;
 	}
 
-	if (!nlk->portid) {
+	/* Ensure nlk->portid is up-to-date. */
+	if (!nlk_bound(nlk)) {
 		err = netlink_autobind(sock);
 		if (err)
 			goto out;
@@ -3257,7 +3261,7 @@ static inline u32 netlink_hash(const voi
 	const struct netlink_sock *nlk = data;
 	struct netlink_compare_arg arg;
 
-	netlink_compare_arg_init(&arg, sock_net(&nlk->sk), nlk->rhash_portid);
+	netlink_compare_arg_init(&arg, sock_net(&nlk->sk), nlk->portid);
 	return jhash2((u32 *)&arg, netlink_compare_arg_len / sizeof(u32), seed);
 }
 
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -3,6 +3,7 @@
 
 #include <linux/rhashtable.h>
 #include <linux/atomic.h>
+#include <asm/barrier.h>
 #include <net/sock.h>
 
 #define NLGRPSZ(x)	(ALIGN(x, sizeof(unsigned long) * 8) / 8)
@@ -25,7 +26,6 @@ struct netlink_ring {
 struct netlink_sock {
 	/* struct sock has to be the first member of netlink_sock */
 	struct sock		sk;
-	u32			rhash_portid;
 	u32			portid;
 	u32			dst_portid;
 	u32			dst_group;
@@ -36,6 +36,7 @@ struct netlink_sock {
 	unsigned long		state;
 	size_t			max_recvmsg_len;
 	wait_queue_head_t	wait;
+	bool			__bound;	/* always use nlk_bound() */
 	bool			cb_running;
 	struct netlink_callback	cb;
 	struct mutex		*cb_mutex;
@@ -60,6 +61,23 @@ static inline struct netlink_sock *nlk_s
 	return container_of(sk, struct netlink_sock, sk);
 }
 
+/**
+ * nlk_bound - test whether a netlink_sock is bound to a port number
+ * @nlk: netlink_sock of interest
+ *
+ * Test whether @nlk is bound to a port number.  Can be called without any
+ * locks and guarantees no false positive - @nlk has a valid port number
+ * and is hashed on a true return.
+ */
+static inline bool nlk_bound(struct netlink_sock *nlk)
+{
+	/*
+	 * Paired with smp_store_release() in netlink_insert() to guarantee
+	 * the visibility of port number and hashing.
+	 */
+	return smp_load_acquire(&nlk->__bound);
+}
+
 struct netlink_table {
 	struct rhashtable	hash;
 	struct hlist_head	mc_list;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227866 — Re: [PATCH v3] netlink: Fix autobind race condition that leads to zero port ID

Hello, Herbert.

On Fri, Sep 18, 2015 at 02:36:10PM +0800, Herbert Xu wrote:
> On Thu, Sep 17, 2015 at 07:30:34AM -0400, Tejun Heo wrote:
> >
> > Maybe add that this led to a deadlock and add a Link tag to this
> > thread?
> 
> I'll add a note about the deadlock but I don't like Link tags
> because websites die and you can always just google the patch
> subject.

That's why we use http://lkml.kernel.org/r/MSG_ID links.

> > > +	nlk_sk(sk)->bound = !!portid;
> > 
> > !! isn't necessasry and this creates ordering between two stores.
> 
> !! was necessary because we're going from a u32 to a bool.

bool casting actually collapses the source value to a boolean value.
No need for casting regardless of data type.

> @@ -1076,17 +1076,19 @@ static int netlink_insert(struct sock *sk, u32 portid)
>  	    unlikely(atomic_read(&table->hash.nelems) >= UINT_MAX))
>  		goto err;
>  
> -	nlk_sk(sk)->portid = portid;
> +	nlk_sk(sk)->rhash_portid = portid;
>  	sock_hold(sk);
>  
>  	err = __netlink_insert(table, sk);
>  	if (err) {
>  		if (err == -EEXIST)
>  			err = -EADDRINUSE;
> -		nlk_sk(sk)->portid = 0;
>  		sock_put(sk);
> +		goto err;
>  	}
>  
> +	nlk_sk(sk)->portid = portid;

So, this doesn't necessarily make the ordering problem go away.  The
hash lookup would be fine but imagine a code path like the following.

	rcu_read_lock();
	sock = rhash lookup(some port number);
	do some operation which may use sock->portid;
	rcu_read_unlock();

Now, that some operation may see 0 as the port number.  I don't think
you can avoid doing some type of memory barrier operations if you
wanna gate autobind w/o grabbing locks.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226936

On 09/17/15 at 01:15pm, Herbert Xu wrote:
> On Wed, Sep 16, 2015 at 10:02:00PM -0700, Cong Wang wrote:
> >
> > This part doesn't look correct, seems it is checking if this is a kernel
> > netlink socket rather than if it is bound. But I am not sure...
> 
> Good point.  I've changed it so that bound is only set for non-kernel
> sockets.
> 
> ---8<---
> netlink: Fix autobind race condition that leads to zero port ID
> 
> The commit c0bb07df7d981e4091432754e30c9c720e2c0c78 ("netlink:
> Reset portid after netlink_insert failure") introduced a race
> condition where if two threads tried to autobind the same socket
> one of them may end up with a zero port ID.
> 
> This patch reverts that commit and instead fixes it by introducing
> a separte "bound" variable to indicate whether a user-space socket
> has been bound.
> 
> Fixes: c0bb07df7d98 ("netlink: Reset portid after netlink_insert failure")
> Reported-by: Tejun Heo <tj@kernel.org>
> Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> Reviewed-by: Cong Wang <cwang@twopensource.com>

Acked-by: Thomas Graf <tgraf@suug.ch>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 3 of 3 — ← Prev page 1 2 [3]

Back to top | Article view | linux.kernel

csiph-web