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 netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html