Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1229632
| From | Tejun Heo <tj@kernel.org> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: netlink: Replace rhash_portid with bound |
| Date | 2015-09-21 20:30 +0200 |
| Message-ID | <qbe0G-6c1-5@gated-at.bofh.it> (permalink) |
| References | <qa1RU-10h-15@gated-at.bofh.it> <qb2iS-69H-5@gated-at.bofh.it> <qb2sy-6Ac-7@gated-at.bofh.it> <qb2Cd-6Ly-7@gated-at.bofh.it> <qb9u3-83M-39@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
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/
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
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
csiph-web