Re: netlink: Add barrier to netlink_connect for theoretical case

[Linus Torvalds]

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
nlk->bound.

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;
   smp_rmb();
   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)
      smp_rmb();
   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
x86.

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.

             Linus
--
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/