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;
return 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
tricks. So

int bound = READ_ONCE(nlk->bound);
if (bound)
return 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
Please read the FAQ at