Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1226631 > unrolled thread
| Started by | Tejun Heo <tj@kernel.org> |
|---|---|
| First post | 2015-09-17 04:30 +0200 |
| Last post | 2015-09-17 13:50 +0200 |
| Articles | 20 on this page of 55 — 7 participants |
Back to article view | Back to linux.kernel
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 1 of 3 [1] 2 3 Next page →
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2015-09-17 04:30 +0200 |
| Subject | Possible netlink autobind regression |
| Message-ID | <q9x7r-6sR-1@gated-at.bofh.it> |
Hello,
We're seeing processes piling up on audit_cmd_mutex and after some
digging it turns out sometimes audit_receive() ends up recursing into
itself causing an A-A deadlock on audit_cmd_mutex. Here's the
backtrace.
PID: 1939995 TASK: ffff88085bdde360 CPU: 27 COMMAND: "crond"
#0 [ffff880369513ae0] __schedule at ffffffff818c49e2
#1 [ffff880369513b30] schedule at ffffffff818c50e7
#2 [ffff880369513b50] schedule_preempt_disabled at ffffffff818c52ce
#3 [ffff880369513b60] __mutex_lock_slowpath at ffffffff818c6ad2
#4 [ffff880369513bc0] mutex_lock at ffffffff818c6b5b
#5 [ffff880369513be0] audit_receive at ffffffff810de5f1
#6 [ffff880369513c10] netlink_unicast at ffffffff817c419b
#7 [ffff880369513c60] netlink_ack at ffffffff817c4764
#8 [ffff880369513ca0] audit_receive at ffffffff810de65a
#9 [ffff880369513cd0] netlink_unicast at ffffffff817c419b
#10 [ffff880369513d20] netlink_sendmsg at ffffffff817c450a
#11 [ffff880369513da0] sock_sendmsg at ffffffff817792ba
#12 [ffff880369513e30] SYSC_sendto at ffffffff81779ce0
#13 [ffff880369513f70] sys_sendto at ffffffff8177a27e
#14 [ffff880369513f80] system_call_fastpath at ffffffff818c8772
RIP: 00007fbf652e6913 RSP: 00007ffca11d08d8 RFLAGS: 00010202
RAX: ffffffffffffffda RBX: ffffffff818c8772 RCX: 0000000000000000
RDX: 0000000000000070 RSI: 00007ffca11d2c50 RDI: 0000000000000003
RBP: 00007ffca11d2c50 R8: 00007ffca11d08f0 R9: 000000000000000c
R10: 0000000000000000 R11: 0000000000000246 R12: ffffffff8177a27e
R13: ffff880369513f78 R14: 0000000000000060 R15: 0000000000000070
ORIG_RAX: 000000000000002c CS: 0033 SS: 002b
The weird thing is that netlink_ack() ends up trying to send ack to
the kernel socket instead of the userland sender. This seems only
possible if the issuing socket's portid is 0 for some reason.
c0bb07df7d98 ("netlink: Reset portid after netlink_insert failure")
made netlink_insert() reset nlk_sk(sk)->portid to zero if insertion
fails. The description says that it prevents sockets which lost
autobinding from being stuck in limbo. I could be mistaken but the
description doesn't seem to be enough - for unconnected sendmsg users,
it looks like the socket would just use the wrong port number likely
occupied by someone else from the second try which would hose ack
routing.
Anyways, after the patch, it seems something like the following could
happen.
thread 1 thread 2
-----------------------------------------------------------------------
netlink_sendmsg()
netlink_autobind()
netlink_insert()
nlk->portid = portid;
__netlink_insert() fails
netlink_sendmsg()
nlk->portid is not zero,
skip autobinding
nlk->portid = 0;
NETLINK_CB(skb).portid is set to zero
...
netlink_ack(skb)
tries to send ack to kernel rx socket
If the above could happen, it would explain the deadlock. The root
problem is that whether to autobind or not is determined by testing
whether nlk->portid is zero without synchronizing against autobind
path and the autobind path may temporarily set and then clear portid
thus tricking the unsynchronized test. The issue can be fixed by only
setting portid after it's known that the port can be used.
That particular issue can be fixed by the following incorrect patch
which needs more work as it ends up temporarily hashing sockets with a
mismatching portid.
Does this make any sense or am I chasing wild geese?
Thanks.
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7f86d3b..6835426 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1037,11 +1037,12 @@ static struct sock *__netlink_lookup(struct netlink_table *table, u32 portid,
netlink_rhashtable_params);
}
-static int __netlink_insert(struct netlink_table *table, struct sock *sk)
+static int __netlink_insert(struct netlink_table *table, struct sock *sk,
+ int portid)
{
struct netlink_compare_arg arg;
- netlink_compare_arg_init(&arg, sock_net(sk), nlk_sk(sk)->portid);
+ netlink_compare_arg_init(&arg, sock_net(sk), portid);
return rhashtable_lookup_insert_key(&table->hash, &arg,
&nlk_sk(sk)->node,
netlink_rhashtable_params);
@@ -1103,11 +1104,12 @@ static int netlink_insert(struct sock *sk, u32 portid)
unlikely(atomic_read(&table->hash.nelems) >= UINT_MAX))
goto err;
- nlk_sk(sk)->portid = portid;
sock_hold(sk);
- err = __netlink_insert(table, sk);
- if (err) {
+ err = __netlink_insert(table, sk, portid);
+ if (!err) {
+ nlk_sk(sk)->portid = portid;
+ } else {
/* In case the hashtable backend returns with -EBUSY
* from here, it must not escape to the caller.
*/
@@ -1115,7 +1117,6 @@ static int netlink_insert(struct sock *sk, u32 portid)
err = -EOVERFLOW;
if (err == -EEXIST)
err = -EADDRINUSE;
- nlk_sk(sk)->portid = 0;
sock_put(sk);
}
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] | [next] | [standalone]
| From | Herbert Xu <herbert@gondor.apana.org.au> |
|---|---|
| Date | 2015-09-17 05:10 +0200 |
| Message-ID | <q9xKa-7tm-13@gated-at.bofh.it> |
| In reply to | #1226631 |
On Wed, Sep 16, 2015 at 10:29:09PM -0400, Tejun Heo wrote: > > Anyways, after the patch, it seems something like the following could > happen. Good catch! I think your explanation makes perfect sense. Linus ran into this previously too after suspend-and-resume. I'll look into it. Thanks! -- 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]
| From | Herbert Xu <herbert@gondor.apana.org.au> |
|---|---|
| Date | 2015-09-17 05:50 +0200 |
| Message-ID | <q9ymR-8es-5@gated-at.bofh.it> |
| In reply to | #1226641 |
On Thu, Sep 17, 2015 at 11:08:45AM +0800, Herbert Xu wrote:
>
> Good catch! I think your explanation makes perfect sense. Linus
> ran into this previously too after suspend-and-resume.
Unfortunately you can't just postpone the setting of portid because
once you pass it onto rhashtable the portid must never change while
it's in custody.
So what I've done is essentially revert my previous fix and instead
add a new boolean "bound" to indicate whether the socket has been
bound.
---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 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>
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7f86d3b..abcbca5 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1095,7 +1095,7 @@ static int netlink_insert(struct sock *sk, u32 portid)
lock_sock(sk);
err = -EBUSY;
- if (nlk_sk(sk)->portid)
+ if (nlk_sk(sk)->bound)
goto err;
err = -ENOMEM;
@@ -1115,10 +1115,12 @@ static int netlink_insert(struct sock *sk, u32 portid)
err = -EOVERFLOW;
if (err == -EEXIST)
err = -EADDRINUSE;
- nlk_sk(sk)->portid = 0;
sock_put(sk);
+ goto err;
}
+ nlk_sk(sk)->bound = true;
+
err:
release_sock(sk);
return err;
@@ -1285,7 +1287,7 @@ static int netlink_release(struct socket *sock)
skb_queue_purge(&sk->sk_write_queue);
- if (nlk->portid) {
+ if (nlk->bound) {
struct netlink_notify n = {
.net = sock_net(sk),
.protocol = sk->sk_protocol,
@@ -1519,7 +1521,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
return err;
}
- if (nlk->portid)
+ if (nlk->bound)
if (nladdr->nl_pid != nlk->portid)
return -EINVAL;
@@ -1537,7 +1539,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
}
}
- if (!nlk->portid) {
+ if (!nlk->bound) {
err = nladdr->nl_pid ?
netlink_insert(sk, nladdr->nl_pid) :
netlink_autobind(sock);
@@ -1585,7 +1587,7 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr,
!netlink_allowed(sock, NL_CFG_F_NONROOT_SEND))
return -EPERM;
- if (!nlk->portid)
+ if (!nlk->bound)
err = netlink_autobind(sock);
if (err == 0) {
@@ -2426,7 +2428,7 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
dst_group = nlk->dst_group;
}
- if (!nlk->portid) {
+ if (!nlk->bound) {
err = netlink_autobind(sock);
if (err)
goto out;
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index 8900840..e6aae40 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -35,6 +35,7 @@ struct netlink_sock {
unsigned long state;
size_t max_recvmsg_len;
wait_queue_head_t wait;
+ bool bound;
bool cb_running;
struct netlink_callback cb;
struct mutex *cb_mutex;
--
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]
| From | Cong Wang <cwang@twopensource.com> |
|---|---|
| Date | 2015-09-17 07:10 +0200 |
| Message-ID | <q9zCi-1NG-7@gated-at.bofh.it> |
| In reply to | #1226648 |
On Wed, Sep 16, 2015 at 8:41 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, Sep 17, 2015 at 11:08:45AM +0800, Herbert Xu wrote:
>>
>> Good catch! I think your explanation makes perfect sense. Linus
>> ran into this previously too after suspend-and-resume.
>
> Unfortunately you can't just postpone the setting of portid because
> once you pass it onto rhashtable the portid must never change while
> it's in custody.
>
> So what I've done is essentially revert my previous fix and instead
> add a new boolean "bound" to indicate whether the socket has been
> bound.
>
> ---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 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>
We saw similar soft lockup with the one Tejun reported, in our data
center.
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Just one comment below.
[...]
> @@ -1285,7 +1287,7 @@ static int netlink_release(struct socket *sock)
>
> skb_queue_purge(&sk->sk_write_queue);
>
> - if (nlk->portid) {
> + if (nlk->bound) {
> struct netlink_notify n = {
> .net = sock_net(sk),
> .protocol = sk->sk_protocol,
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...
Other than this, looks good to me:
Reviewed-by: Cong Wang <cwang@twopensource.com>
--
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]
| From | Herbert Xu <herbert@gondor.apana.org.au> |
|---|---|
| Date | 2015-09-17 07:20 +0200 |
| Message-ID | <q9zLY-1Zr-15@gated-at.bofh.it> |
| In reply to | #1226664 |
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>
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index dea9253..42013c5 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1068,7 +1068,7 @@ static int netlink_insert(struct sock *sk, u32 portid)
lock_sock(sk);
err = -EBUSY;
- if (nlk_sk(sk)->portid)
+ if (nlk_sk(sk)->bound)
goto err;
err = -ENOMEM;
@@ -1083,10 +1083,12 @@ static int netlink_insert(struct sock *sk, u32 portid)
if (err) {
if (err == -EEXIST)
err = -EADDRINUSE;
- nlk_sk(sk)->portid = 0;
sock_put(sk);
+ goto err;
}
+ nlk_sk(sk)->bound = !!portid;
+
err:
release_sock(sk);
return err;
@@ -1253,7 +1255,7 @@ static int netlink_release(struct socket *sock)
skb_queue_purge(&sk->sk_write_queue);
- if (nlk->portid) {
+ if (nlk->bound) {
struct netlink_notify n = {
.net = sock_net(sk),
.protocol = sk->sk_protocol,
@@ -1487,7 +1489,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
return err;
}
- if (nlk->portid)
+ if (nlk->bound)
if (nladdr->nl_pid != nlk->portid)
return -EINVAL;
@@ -1505,7 +1507,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
}
}
- if (!nlk->portid) {
+ if (!nlk->bound) {
err = nladdr->nl_pid ?
netlink_insert(sk, nladdr->nl_pid) :
netlink_autobind(sock);
@@ -1553,7 +1555,7 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr,
!netlink_allowed(sock, NL_CFG_F_NONROOT_SEND))
return -EPERM;
- if (!nlk->portid)
+ if (!nlk->bound)
err = netlink_autobind(sock);
if (err == 0) {
@@ -2371,7 +2373,7 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
dst_group = nlk->dst_group;
}
- if (!nlk->portid) {
+ if (!nlk->bound) {
err = netlink_autobind(sock);
if (err)
goto out;
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index 8900840..e6aae40 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -35,6 +35,7 @@ struct netlink_sock {
unsigned long state;
size_t max_recvmsg_len;
wait_queue_head_t wait;
+ bool bound;
bool cb_running;
struct netlink_callback cb;
struct mutex *cb_mutex;
--
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]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2015-09-17 13:40 +0200 |
| Message-ID | <q9FHI-2gj-19@gated-at.bofh.it> |
| In reply to | #1226670 |
Hello, Herbert.
On Thu, Sep 17, 2015 at 01:15:03PM +0800, Herbert Xu wrote:
> 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>
Maybe add that this led to a deadlock and add a Link tag to this
thread?
> @@ -1083,10 +1083,12 @@ static int netlink_insert(struct sock *sk, u32 portid)
> if (err) {
> if (err == -EEXIST)
> err = -EADDRINUSE;
> - nlk_sk(sk)->portid = 0;
> sock_put(sk);
> + goto err;
> }
>
> + nlk_sk(sk)->bound = !!portid;
!! isn't necessasry and this creates ordering between two stores.
->bound must be visible only after ->portid is visible, so this should
be smp_store_release().
> @@ -2371,7 +2373,7 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
> dst_group = nlk->dst_group;
> }
>
> - if (!nlk->portid) {
> + if (!nlk->bound) {
And all unlocked reads should be smp_load_acquire().
> err = netlink_autobind(sock);
> if (err)
> goto out;
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]
| From | Herbert Xu <herbert@gondor.apana.org.au> |
|---|---|
| Date | 2015-09-18 08:40 +0200 |
| Subject | [PATCH v3] netlink: Fix autobind race condition that leads to zero port ID |
| Message-ID | <q9XuV-35x-1@gated-at.bofh.it> |
| In reply to | #1226933 |
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.
> > + 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.
> ->bound must be visible only after ->portid is visible, so this should
> be smp_store_release().
But yes there are ordering issues here so I've decided to make
rhashtable use a new field for its hash instead.
Note that I've dropped the acks as this patch is now substantially
different.
---8<---
The commit c0bb07df7d981e4091432754e30c9c720e2c0c78 ("netlink:
Reset portid after netlink_insert failure") introduced a race
condition where if two threads try to autobind the same socket
one of them may end up with a zero port ID. This led to kernel
deadlocks that were observed by multiple people.
This patch reverts that commit and instead fixes it by introducing
a separte rhash_portid variable so that the real portid is only set
after the socket has been successfully hashed.
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>
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index dea9253..7157bad 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -988,7 +988,7 @@ static inline int netlink_compare(struct rhashtable_compare_arg *arg,
const struct netlink_compare_arg *x = arg->key;
const struct netlink_sock *nlk = ptr;
- return nlk->portid != x->portid ||
+ return nlk->rhash_portid != x->portid ||
!net_eq(sock_net(&nlk->sk), read_pnet(&x->pnet));
}
@@ -1014,7 +1014,7 @@ static int __netlink_insert(struct netlink_table *table, struct sock *sk)
{
struct netlink_compare_arg arg;
- netlink_compare_arg_init(&arg, sock_net(sk), nlk_sk(sk)->portid);
+ netlink_compare_arg_init(&arg, sock_net(sk), nlk_sk(sk)->rhash_portid);
return rhashtable_lookup_insert_key(&table->hash, &arg,
&nlk_sk(sk)->node,
netlink_rhashtable_params);
@@ -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;
+
err:
release_sock(sk);
return err;
@@ -3197,7 +3199,7 @@ static inline u32 netlink_hash(const void *data, u32 len, u32 seed)
const struct netlink_sock *nlk = data;
struct netlink_compare_arg arg;
- netlink_compare_arg_init(&arg, sock_net(&nlk->sk), nlk->portid);
+ netlink_compare_arg_init(&arg, sock_net(&nlk->sk), nlk->rhash_portid);
return jhash2((u32 *)&arg, netlink_compare_arg_len / sizeof(u32), seed);
}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index 8900840..c96dfa3 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -25,6 +25,7 @@ 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;
--
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]
| From | Herbert Xu <herbert@gondor.apana.org.au> |
|---|---|
| Date | 2015-09-18 13:20 +0200 |
| Subject | [PATCH v4] netlink: Fix autobind race condition that leads to zero port ID |
| Message-ID | <qa1RU-10h-15@gated-at.bofh.it> |
| In reply to | #1227594 |
On Fri, Sep 18, 2015 at 02:36:09PM +0800, Herbert Xu wrote:
>
> But yes there are ordering issues here so I've decided to make
> rhashtable use a new field for its hash instead.
>
> Note that I've dropped the acks as this patch is now substantially
> different.
v3 doesn't apply to the current tree anymore so here is a respin
rebased on rc1.
---8<---
The commit c0bb07df7d981e4091432754e30c9c720e2c0c78 ("netlink:
Reset portid after netlink_insert failure") introduced a race
condition where if two threads try to autobind the same socket
one of them may end up with a zero port ID. This led to kernel
deadlocks that were observed by multiple people.
This patch reverts that commit and instead fixes it by introducing
a separte rhash_portid variable so that the real portid is only set
after the socket has been successfully hashed.
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>
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7f86d3b..303efb7 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1015,7 +1015,7 @@ static inline int netlink_compare(struct rhashtable_compare_arg *arg,
const struct netlink_compare_arg *x = arg->key;
const struct netlink_sock *nlk = ptr;
- return nlk->portid != x->portid ||
+ return nlk->rhash_portid != x->portid ||
!net_eq(sock_net(&nlk->sk), read_pnet(&x->pnet));
}
@@ -1041,7 +1041,7 @@ static int __netlink_insert(struct netlink_table *table, struct sock *sk)
{
struct netlink_compare_arg arg;
- netlink_compare_arg_init(&arg, sock_net(sk), nlk_sk(sk)->portid);
+ netlink_compare_arg_init(&arg, sock_net(sk), nlk_sk(sk)->rhash_portid);
return rhashtable_lookup_insert_key(&table->hash, &arg,
&nlk_sk(sk)->node,
netlink_rhashtable_params);
@@ -1103,7 +1103,7 @@ 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);
@@ -1115,10 +1115,12 @@ static int netlink_insert(struct sock *sk, u32 portid)
err = -EOVERFLOW;
if (err == -EEXIST)
err = -EADDRINUSE;
- nlk_sk(sk)->portid = 0;
sock_put(sk);
+ goto err;
}
+ nlk_sk(sk)->portid = portid;
+
err:
release_sock(sk);
return err;
@@ -3255,7 +3257,7 @@ static inline u32 netlink_hash(const void *data, u32 len, u32 seed)
const struct netlink_sock *nlk = data;
struct netlink_compare_arg arg;
- netlink_compare_arg_init(&arg, sock_net(&nlk->sk), nlk->portid);
+ netlink_compare_arg_init(&arg, sock_net(&nlk->sk), nlk->rhash_portid);
return jhash2((u32 *)&arg, netlink_compare_arg_len / sizeof(u32), seed);
}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index 8900840..c96dfa3 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -25,6 +25,7 @@ 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;
--
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]
| From | David Miller <davem@davemloft.net> |
|---|---|
| Date | 2015-09-21 08:00 +0200 |
| Subject | Re: [PATCH v4] netlink: Fix autobind race condition that leads to zero port ID |
| Message-ID | <qb2iS-69H-5@gated-at.bofh.it> |
| In reply to | #1227775 |
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 18 Sep 2015 19:16:50 +0800
> The commit c0bb07df7d981e4091432754e30c9c720e2c0c78 ("netlink:
> Reset portid after netlink_insert failure") introduced a race
> condition where if two threads try to autobind the same socket
> one of them may end up with a zero port ID. This led to kernel
> deadlocks that were observed by multiple people.
>
> This patch reverts that commit and instead fixes it by introducing
> a separte rhash_portid variable so that the real portid is only set
> after the socket has been successfully hashed.
>
> 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>
Applied and queued up for -stable, thanks Herbert.
--
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]
| From | Herbert Xu <herbert@gondor.apana.org.au> |
|---|---|
| Date | 2015-09-21 08:10 +0200 |
| Subject | Re: [PATCH v4] netlink: Fix autobind race condition that leads to zero port ID |
| Message-ID | <qb2sy-6Ac-7@gated-at.bofh.it> |
| In reply to | #1229059 |
On Sun, Sep 20, 2015 at 10:55:21PM -0700, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Fri, 18 Sep 2015 19:16:50 +0800
>
> > The commit c0bb07df7d981e4091432754e30c9c720e2c0c78 ("netlink:
> > Reset portid after netlink_insert failure") introduced a race
> > condition where if two threads try to autobind the same socket
> > one of them may end up with a zero port ID. This led to kernel
> > deadlocks that were observed by multiple people.
> >
> > This patch reverts that commit and instead fixes it by introducing
> > a separte rhash_portid variable so that the real portid is only set
> > after the socket has been successfully hashed.
> >
> > 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>
>
> Applied and queued up for -stable, thanks Herbert.
Sorry but Dave but there are still races with v4 as Tejun pointed
out. I'm still working on it and I could post them as incremental
patches if that's the easiest.
Thanks,
--
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]
| From | David Miller <davem@davemloft.net> |
|---|---|
| Date | 2015-09-21 08:20 +0200 |
| Subject | Re: [PATCH v4] netlink: Fix autobind race condition that leads to zero port ID |
| Message-ID | <qb2Cd-6Ly-7@gated-at.bofh.it> |
| In reply to | #1229060 |
From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 21 Sep 2015 14:06:36 +0800
> On Sun, Sep 20, 2015 at 10:55:21PM -0700, David Miller wrote:
>> From: Herbert Xu <herbert@gondor.apana.org.au>
>> Date: Fri, 18 Sep 2015 19:16:50 +0800
>>
>> > The commit c0bb07df7d981e4091432754e30c9c720e2c0c78 ("netlink:
>> > Reset portid after netlink_insert failure") introduced a race
>> > condition where if two threads try to autobind the same socket
>> > one of them may end up with a zero port ID. This led to kernel
>> > deadlocks that were observed by multiple people.
>> >
>> > This patch reverts that commit and instead fixes it by introducing
>> > a separte rhash_portid variable so that the real portid is only set
>> > after the socket has been successfully hashed.
>> >
>> > 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>
>>
>> Applied and queued up for -stable, thanks Herbert.
>
> Sorry but Dave but there are still races with v4 as Tejun pointed
> out. I'm still working on it and I could post them as incremental
> patches if that's the easiest.
Oops, sorry about that.
Yeah at this point incremental patches work the best.
Thanks.
--
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]
| From | Herbert Xu <herbert@gondor.apana.org.au> |
|---|---|
| Date | 2015-09-21 15:40 +0200 |
| Subject | netlink: Replace rhash_portid with bound |
| Message-ID | <qb9u3-83M-39@gated-at.bofh.it> |
| In reply to | #1229067 |
On Sun, Sep 20, 2015 at 11:11:04PM -0700, David Miller wrote:
>
> Yeah at this point incremental patches work the best.
OK here is the patch:
---8<---
The commit 1f770c0a09da855a2b51af6d19de97fb955eca85 ("netlink:
Fix autobind race condition that leads to zero port ID") created
some new races that can occur due to inconcsistencies between the
two port IDs.
Tejun is right that a barrier is unavoidable. Therefore I am
reverting to the original patch that used a boolean to indicate
that a user netlink socket has been bound.
Barriers have been added where necessary to ensure that a valid
portid is used.
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>
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 303efb7..f5362aae 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -24,6 +24,7 @@
#include <linux/module.h>
+#include <asm/barrier.h>
#include <linux/capability.h>
#include <linux/kernel.h>
#include <linux/init.h>
@@ -1015,7 +1016,7 @@ static inline int netlink_compare(struct rhashtable_compare_arg *arg,
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 +1042,7 @@ static int __netlink_insert(struct netlink_table *table, struct sock *sk)
{
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 +1096,7 @@ static int netlink_insert(struct sock *sk, u32 portid)
lock_sock(sk);
err = -EBUSY;
- if (nlk_sk(sk)->portid)
+ if (nlk_sk(sk)->bound)
goto err;
err = -ENOMEM;
@@ -1103,7 +1104,7 @@ static int netlink_insert(struct sock *sk, u32 portid)
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 +1120,11 @@ static int netlink_insert(struct sock *sk, u32 portid)
goto err;
}
- nlk_sk(sk)->portid = portid;
+ /* rhashtable_insert carries an implicit write memory barrier
+ * so we don't need an smp_wmb here in order to ensure that
+ * portid is set before bound.
+ */
+ nlk_sk(sk)->bound = portid;
err:
release_sock(sk);
@@ -1521,9 +1526,11 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
return err;
}
- if (nlk->portid)
+ /* Ensure nlk->portid is up-to-date. */
+ if (smp_load_acquire(&nlk->bound)) {
if (nladdr->nl_pid != nlk->portid)
return -EINVAL;
+ }
if (nlk->netlink_bind && groups) {
int group;
@@ -1539,7 +1546,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
}
}
- if (!nlk->portid) {
+ if (!nlk->bound) {
err = nladdr->nl_pid ?
netlink_insert(sk, nladdr->nl_pid) :
netlink_autobind(sock);
@@ -1587,7 +1594,7 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr,
!netlink_allowed(sock, NL_CFG_F_NONROOT_SEND))
return -EPERM;
- if (!nlk->portid)
+ if (!nlk->bound)
err = netlink_autobind(sock);
if (err == 0) {
@@ -2428,7 +2435,8 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
dst_group = nlk->dst_group;
}
- if (!nlk->portid) {
+ /* Ensure nlk->portid is up-to-date. */
+ if (!smp_load_acquire(&nlk->bound)) {
err = netlink_autobind(sock);
if (err)
goto out;
@@ -3257,7 +3265,7 @@ static inline u32 netlink_hash(const void *data, u32 len, u32 seed)
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);
}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index c96dfa3..e6aae40 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -25,7 +25,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 +35,7 @@ struct netlink_sock {
unsigned long state;
size_t max_recvmsg_len;
wait_queue_head_t wait;
+ bool bound;
bool cb_running;
struct netlink_callback cb;
struct mutex *cb_mutex;
--
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]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2015-09-21 20:30 +0200 |
| Subject | Re: netlink: Replace rhash_portid with bound |
| Message-ID | <qbe0G-6c1-5@gated-at.bofh.it> |
| In reply to | #1229328 |
Hello, Herbert.
On Mon, Sep 21, 2015 at 09:34:16PM +0800, Herbert Xu wrote:
> @@ -1119,7 +1120,11 @@ static int netlink_insert(struct sock *sk, u32 portid)
> goto err;
> }
>
> - nlk_sk(sk)->portid = portid;
> + /* rhashtable_insert carries an implicit write memory barrier
> + * so we don't need an smp_wmb here in order to ensure that
> + * portid is set before bound.
> + */
> + nlk_sk(sk)->bound = portid;
store_release and load_acquire are different from the usual memory
barriers and can't be paired this way. You have to pair store_release
and load_acquire. Besides, it isn't a particularly good idea to
depend on memory barriers embedded in other data structures like the
above. Here, especially, rhashtable_insert() would have write barrier
*before* the entry is hashed not necessarily *after*, which means that
in the above case, a socket which appears to have set bound to a
reader might not visible when the reader tries to look up the socket
on the hashtable.
There's no reason to be overly smart here. This isn't a crazy hot
path, write barriers tend to be very cheap, store_release more so.
Please just do smp_store_release() and note what it's paired with.
> @@ -1539,7 +1546,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
> }
> }
>
> - if (!nlk->portid) {
> + if (!nlk->bound) {
I don't think you can skip load_acquire here just because this is the
second deref of the variable. That doesn't change anything. Race
condition could still happen between the first and second tests and
skipping the second would lead to the same kind of bug.
> @@ -1587,7 +1594,7 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr,
> !netlink_allowed(sock, NL_CFG_F_NONROOT_SEND))
> return -EPERM;
>
> - if (!nlk->portid)
> + if (!nlk->bound)
Don't we need load_acquire here too? Is this path holding a lock
which makes that unnecessary?
I'd suggest making it clear that ->bound is internal (name it
->__bound or sth) and provide a test macro which always uses
load_acquire. It could be that there are a couple places which can
avoid load_acquire but it just isn't worth it. load_acquire is very
cheap but bugs around it can be extremely subtle. Let's please keep
it straight-forward.
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]
| From | Herbert Xu <herbert@gondor.apana.org.au> |
|---|---|
| Date | 2015-09-22 05:40 +0200 |
| Subject | [PATCH v2] netlink: Replace rhash_portid with bound |
| Message-ID | <qbmAV-1Hc-5@gated-at.bofh.it> |
| In reply to | #1229632 |
On Mon, Sep 21, 2015 at 02:20:22PM -0400, Tejun Heo wrote:
>
> store_release and load_acquire are different from the usual memory
> barriers and can't be paired this way. You have to pair store_release
> and load_acquire. Besides, it isn't a particularly good idea to
OK I've decided to drop the acquire/release helpers as they don't
help us at all and simply pessimises the code by using full memory
barriers (on some architectures) where only a write or read barrier
is needed.
> depend on memory barriers embedded in other data structures like the
> above. Here, especially, rhashtable_insert() would have write barrier
> *before* the entry is hashed not necessarily *after*, which means that
> in the above case, a socket which appears to have set bound to a
> reader might not visible when the reader tries to look up the socket
> on the hashtable.
But you are right we do need an explicit write barrier here to
ensure that the hashing is visible.
> There's no reason to be overly smart here. This isn't a crazy hot
> path, write barriers tend to be very cheap, store_release more so.
> Please just do smp_store_release() and note what it's paired with.
It's not about being overly smart. It's about actually understanding
what's going on with the code. I've seen too many instances of
people simply sprinkling synchronisation primitives around without
any knowledge of what is happening underneath, which is just a recipe
for creating hard-to-debug races.
> > @@ -1539,7 +1546,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
> > }
> > }
> >
> > - if (!nlk->portid) {
> > + if (!nlk->bound) {
>
> I don't think you can skip load_acquire here just because this is the
> second deref of the variable. That doesn't change anything. Race
> condition could still happen between the first and second tests and
> skipping the second would lead to the same kind of bug.
The reason this one is OK is because we do not use nlk->portid or
try to get nlk from the hash table before we return to user-space.
However, there is a real bug here that none of these acquire/release
helpers discovered. The two bound tests here used to be a single
one. Now that they are separate it is entirely possible for another
thread to come in the middle and bind the socket. So we need to
repeat the portid check in order to maintain consistency.
> > @@ -1587,7 +1594,7 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr,
> > !netlink_allowed(sock, NL_CFG_F_NONROOT_SEND))
> > return -EPERM;
> >
> > - if (!nlk->portid)
> > + if (!nlk->bound)
>
> Don't we need load_acquire here too? Is this path holding a lock
> which makes that unnecessary?
Ditto.
---8<---
The commit 1f770c0a09da855a2b51af6d19de97fb955eca85 ("netlink:
Fix autobind race condition that leads to zero port ID") created
some new races that can occur due to inconcsistencies between the
two port IDs.
Tejun is right that a barrier is unavoidable. Therefore I am
reverting to the original patch that used a boolean to indicate
that a user netlink socket has been bound.
Barriers have been added where necessary to ensure that a valid
portid and the hashed socket is visible.
I have also changed netlink_insert to only return EBUSY if the
socket is bound to a portid different to the requested one. This
combined with only reading nlk->bound once in netlink_bind fixes
a race where two threads that bind the socket at the same time
with different port IDs may both succeed.
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>
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 303efb7..2c15fae 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1015,7 +1015,7 @@ static inline int netlink_compare(struct rhashtable_compare_arg *arg,
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 netlink_table *table, struct sock *sk)
{
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);
@@ -1094,8 +1094,8 @@ static int netlink_insert(struct sock *sk, u32 portid)
lock_sock(sk);
- err = -EBUSY;
- if (nlk_sk(sk)->portid)
+ err = nlk_sk(sk)->portid == portid ? 0 : -EBUSY;
+ if (nlk_sk(sk)->bound)
goto err;
err = -ENOMEM;
@@ -1103,7 +1103,7 @@ static int netlink_insert(struct sock *sk, u32 portid)
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,9 @@ static int netlink_insert(struct sock *sk, u32 portid)
goto err;
}
- nlk_sk(sk)->portid = portid;
+ /* We need to ensure that the socket is hashed and visible. */
+ smp_wmb();
+ nlk_sk(sk)->bound = portid;
err:
release_sock(sk);
@@ -1505,6 +1507,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr;
int err;
long unsigned int groups = nladdr->nl_groups;
+ bool bound;
if (addr_len < sizeof(struct sockaddr_nl))
return -EINVAL;
@@ -1521,9 +1524,14 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
return err;
}
- if (nlk->portid)
+ bound = nlk->bound;
+ if (bound) {
+ /* Ensure nlk->portid is up-to-date. */
+ smp_rmb();
+
if (nladdr->nl_pid != nlk->portid)
return -EINVAL;
+ }
if (nlk->netlink_bind && groups) {
int group;
@@ -1539,7 +1547,10 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
}
}
- if (!nlk->portid) {
+ /* 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) :
netlink_autobind(sock);
@@ -1587,7 +1598,10 @@ static int netlink_connect(struct socket *sock, struct sockaddr *addr,
!netlink_allowed(sock, NL_CFG_F_NONROOT_SEND))
return -EPERM;
- if (!nlk->portid)
+ /* No need for barriers here as we return to user-space without
+ * using any of the bound attributes.
+ */
+ if (!nlk->bound)
err = netlink_autobind(sock);
if (err == 0) {
@@ -2428,10 +2442,13 @@ static int netlink_sendmsg(struct socket *sock, struct msghdr *msg, size_t len)
dst_group = nlk->dst_group;
}
- if (!nlk->portid) {
+ if (!nlk->bound) {
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
@@ -3257,7 +3274,7 @@ static inline u32 netlink_hash(const void *data, u32 len, u32 seed)
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);
}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index c96dfa3..e6aae40 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -25,7 +25,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 +35,7 @@ struct netlink_sock {
unsigned long state;
size_t max_recvmsg_len;
wait_queue_head_t wait;
+ bool bound;
bool cb_running;
struct netlink_callback cb;
struct mutex *cb_mutex;
--
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]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2015-09-22 18:20 +0200 |
| Subject | Re: [PATCH v2] netlink: Replace rhash_portid with bound |
| Message-ID | <qbysq-219-9@gated-at.bofh.it> |
| In reply to | #1229896 |
Hello, Herbert.
On Tue, Sep 22, 2015 at 11:38:56AM +0800, Herbert Xu wrote:
> On Mon, Sep 21, 2015 at 02:20:22PM -0400, Tejun Heo wrote:
> > store_release and load_acquire are different from the usual memory
> > barriers and can't be paired this way. You have to pair store_release
> > and load_acquire. Besides, it isn't a particularly good idea to
>
> OK I've decided to drop the acquire/release helpers as they don't
> help us at all and simply pessimises the code by using full memory
> barriers (on some architectures) where only a write or read barrier
> is needed.
That's a pentium pro era errata. Virtually no working machine is
affected by that anymore and nobody builds kernel with that option.
In most cases, store_release and load_acquire are cheaper as they're
more specific. On x86, store_release and load_acquire boil down to
compiler reordering barriers. You're running in the opposite
direction.
> > There's no reason to be overly smart here. This isn't a crazy hot
> > path, write barriers tend to be very cheap, store_release more so.
> > Please just do smp_store_release() and note what it's paired with.
>
> It's not about being overly smart. It's about actually understanding
> what's going on with the code. I've seen too many instances of
> people simply sprinkling synchronisation primitives around without
> any knowledge of what is happening underneath, which is just a recipe
> for creating hard-to-debug races.
I mean, read this thread. It's one subtle breakage after another, one
confusion after another. The problematic usages of memory barriers
are usually of two types.
1. Misunderstand what barriers do and fail to, most frequently, pair
the barriers correctly. This leads to things like lone smp_wmb()s
which don't do anything but provide false sense of security.
2. The usage itself is correct but not properly documented and it's
not clear what's being synchronized. Because there's nothing
inherently pairing the matching barrier pairs, w/o proper
documentation, it can be very challenging to track down what is
being synchronized making it difficult to tell this case from 1.
Note that this is another reason smp_store_release() and
smp_load_acquire() are just better. Their specificity not only
makes them lighter but also makes it a lot easier to track down
what's going on.
So, when using barriers, we want to first pick the most specific
barrier pairs that suit the use case and clearly identify the matching
pairs and describe any subtlties if there's any. Here, it's a
straight-forward writer to reader interlocking.
Once we meet the above criteria, we do want to be as simple as the use
case allows it to be. e.g. if there are multiples readers,
encapsulate them and document them centrally. If there are different
classes of readers and it's worthwhile to apply different
synchronization schemes to them, use separate helpers or clearly mark
and document exceptions with rationale.
> > I don't think you can skip load_acquire here just because this is the
> > second deref of the variable. That doesn't change anything. Race
> > condition could still happen between the first and second tests and
> > skipping the second would lead to the same kind of bug.
>
> The reason this one is OK is because we do not use nlk->portid or
> try to get nlk from the hash table before we return to user-space.
What happens if somebody later adds code below that which happens to
use portid? You're creating a booby trap and the patch isn't even
properly documenting what's going on.
> ---8<---
> The commit 1f770c0a09da855a2b51af6d19de97fb955eca85 ("netlink:
> Fix autobind race condition that leads to zero port ID") created
> some new races that can occur due to inconcsistencies between the
> two port IDs.
>
> Tejun is right that a barrier is unavoidable. Therefore I am
> reverting to the original patch that used a boolean to indicate
> that a user netlink socket has been bound.
>
> Barriers have been added where necessary to ensure that a valid
> portid and the hashed socket is visible.
>
> I have also changed netlink_insert to only return EBUSY if the
> socket is bound to a portid different to the requested one. This
> combined with only reading nlk->bound once in netlink_bind fixes
> a race where two threads that bind the socket at the same time
> with different port IDs may both succeed.
>
> 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>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
This is a pretty misguided use of barriers.
Nacked-by: Tejun Heo <tj@kernel.org>
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]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-22 20:50 +0200 |
| Subject | Re: [PATCH v2] netlink: Replace rhash_portid with bound |
| Message-ID | <qbAND-5hQ-79@gated-at.bofh.it> |
| In reply to | #1230427 |
On Tue, Sep 22, 2015 at 9:10 AM, Tejun Heo <tj@kernel.org> wrote:
>
> That's a pentium pro era errata. Virtually no working machine is
> affected by that anymore and nobody builds kernel with that option.
> In most cases, store_release and load_acquire are cheaper as they're
> more specific. On x86, store_release and load_acquire boil down to
> compiler reordering barriers. You're running in the opposite
> direction.
Well, to be fair, there are lots of machines where acquire/release is
actually quite expensive.
In general, the cheapest barrier there is (apart from the "no barrier
at all" or just "compiler barrier") is "smp_wmb()". If an
architecture gets that one wrong, the architects were f*cking morons.
It should be a fundamentally cheap operation, since writes are
buffered and it should simply be a buffer barrier.
The acquire/release things are generally fairly cheap on modern
architectures. Not free (except on x86), but fairly low-cost. HOWEVER,
they are not at all free on some older architectures, including 32-bit
ARM.
smp_rmb() should generally be about the same cost as an acquire. It
can go either way.
So *if* the algorithm is amenable to smp_wmb()/smp_rmb() kind of
barriers, that's actually quite possibly better than acquire/release.
smp_mb() is expensive pretty much everywhere.
Looking forward, I suspect long-term acquire/release is what hardware
is going to be "reasonably good at", but as things are right now, you
can't necessarily rely on them being fast.
Linus
--
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]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2015-09-22 21:00 +0200 |
| Subject | Re: [PATCH v2] netlink: Replace rhash_portid with bound |
| Message-ID | <qbAXh-5tC-61@gated-at.bofh.it> |
| In reply to | #1230756 |
Hello, Linus. On Tue, Sep 22, 2015 at 11:42:33AM -0700, Linus Torvalds wrote: ... > smp_rmb() should generally be about the same cost as an acquire. It > can go either way. > > So *if* the algorithm is amenable to smp_wmb()/smp_rmb() kind of > barriers, that's actually quite possibly better than acquire/release. I see. The write path here is cold so the competition is between rmb and acquire. Unless some significant archs completely screwed it up, acquire still seems like the better option. It's essentially free on x86 after all. 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]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-22 21:30 +0200 |
| Subject | Re: [PATCH v2] netlink: Replace rhash_portid with bound |
| Message-ID | <qbBqi-6hK-21@gated-at.bofh.it> |
| In reply to | #1230781 |
On Tue, Sep 22, 2015 at 11:53 AM, Tejun Heo <tj@kernel.org> wrote:
>
> I see. The write path here is cold so the competition is between rmb
> and acquire. Unless some significant archs completely screwed it up,
> acquire still seems like the better option. It's essentially free on
> x86 after all.
Both acquire and smp_rmb() are equally free on x86.
It appears that we don't do the X86_PPRO_FENCE bug handling for
acquire, but I guess we should.
Or possibly we should start deprecating the insane X86_PPRO_FENCE
entirely. It's very costly, for dubious advantages.
Linus
--
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]
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Date | 2015-09-22 22:00 +0200 |
| Subject | Re: [PATCH v2] netlink: Replace rhash_portid with bound |
| Message-ID | <qbBTk-6Qf-29@gated-at.bofh.it> |
| In reply to | #1230855 |
Hello, On Tue, Sep 22, 2015 at 12:28:45PM -0700, Linus Torvalds wrote: > Both acquire and smp_rmb() are equally free on x86. Was this always like this? Ah, okay, from 2007. Was thinking it's still doing an actual rmb. Talk about being confused. > It appears that we don't do the X86_PPRO_FENCE bug handling for > acquire, but I guess we should. Hmmm? We're doing that. PPRO_FENCE makes both acquire and release use smp_mb(). 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]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-22 22:10 +0200 |
| Subject | Re: [PATCH v2] netlink: Replace rhash_portid with bound |
| Message-ID | <qbC2Z-7gK-5@gated-at.bofh.it> |
| In reply to | #1230899 |
On Tue, Sep 22, 2015 at 12:50 PM, Tejun Heo <tj@kernel.org> wrote:
>
> Hmmm? We're doing that. PPRO_FENCE makes both acquire and release
> use smp_mb().
Oh, right you are. I just grepped for rmb, and missed it because as
you say, it's a full mb.
The PPRO_FENCE_BUG thing is rather questionable. I'm not sure it's
even documented, but I can't find the official PPro errata now. I
think I discussed it with Andy Glew back when Intel was starting to
document the memory ordering rules.
I'd be willing to get rid of it. It was apparently just an early
stepping or two.
Anybody with better google-fu than me who can find the official Intel
PPro errata?
Linus
--
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]
Page 1 of 3 [1] 2 3 Next page →
Back to top | Article view | linux.kernel
csiph-web