Re: netlink: Add barrier to netlink_connect for theoretical case
From: Linus Torvalds
Date: Thu Sep 24 2015 - 23:25:02 EST
On Thu, Sep 24, 2015 at 6:43 PM, Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Sep 24, 2015 at 04:05:10PM -0400, Tejun Heo wrote:
> +static inline bool netlink_bound(struct netlink_sock *nlk)
> + /* Ensure nlk is hashed and visible. */
> + if (nlk->bound)
> + smp_rmb();
> + return nlk->bound;
The above looks very suspicious.
If "nlk->bound" isn't stable, then you might read 0 the first time,
not do the smp_rmb(), and then read 1 on the second access to
In other words, you just ended up returning 1 without actually doing
the mb, so there will be no serialization between the "bound" variable
and reading the portid afterwards.
That makes no sense.
And if nlk->bound *is* stable, then the smp_rmb() doesn't make any
sense that I can see.
So for the code to actually make sense, it should either do:
int bound = nlk->bound;
which is fine on x86, but might be expensive on other architectures
due to the unconditional rmb.
So you *could* write it with a conditional rmb, but then you need to
use a READ_ONCE(), to make sure that gcc really does the read exactly
once, because at that point the "rmb" no longer keeps gcc from playing
int bound = READ_ONCE(nlk->bound);
could also be correct. Sadly, while "smp_rmb()" is a no-op on x86, it
*is* a barrier, so the above conditional smp_rmb() actually sucks on
x86, because I suspect that gcc will create a jump around an empty
asm. So the unconditional rmb is actually simpler better on at least
But the function as you wrote it does not make sense. When you do a
barrier, you really have to think about where the accesses are.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/