Miklos Szeredi wrote:
> On Thu, 16 Sep 2010, Paul E. McKenney wrote:
> > On Thu, Sep 16, 2010 at 03:30:56PM +0100, David Howells wrote:
> > > Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> > >
> > > > Is the rmb() really needed?
> > > >
> > > > Take this code from fs/namei.c for example:
> > > >
> > > > inode = next.dentry->d_inode;
> > > > if (!inode)
> > > > goto out_dput;
> > > >
> > > > if (inode->i_op->follow_link) {
> > > >
> > > > It happily dereferences dentry->d_inode without a barrier after
> > > > checking it for non-null, while that d_inode might have just been
> > > > initialized on another CPU with a freshly created inode. There's
> > > > absolutely no synchornization with that on this side.
> > >
> > > Perhaps it's not necessary; once set, how likely is i_op to be changed once
> > > I_NEW is cleared?
> >
> > Are the path_get()s protecting this?
>
> No, when creating a file the dentry will go from negative to positive
> independently from lookup. The dentry can get instantiated with an
> inode between the path_get() and dereferencing ->d_inode.
>
> >
> > If there is no protection, then something like rcu_dereference() is
> > needed for the assignment from next.dentry->d_inode.
>
> Do I understand correctly that the problem is that a CPU may have a
> stale cache associated with *inode, one that was loaded before the
> write barrier took effect?
>
> Funny that such a bug could stay unnoticed in so often excercised
> code. Yeah I know it's alpha only.
When I first saw read_barrier_depends(), I thought it must be Alpha's
speculative execution, fetching memory out of order and confirming
it's valid later. I was really surprised to find out it's not that -
it's a quirk of the Alpha's cache/forwarding protocol. Others
presumably don't have it because they were designed with awareness of
this coding pattern.
But...
I wonder if it can happen on IA64 with it's funky memory-alias
compiler optimisations.
I wonder if it can happen on x86 and others, if the compiler decides
this is a valid transformation (it is with a single CPU):
Original code:
foo = global_ptr_to_foo;
foo_x = foo->x;
bar = global_ptr_to_bar;
bar_y = bar->y;
// use bar_y;
Transformed by compiler:
foo = global_ptr_to_foo;
foo_x = foo->x;
bar = global_ptr_to_bar;
bar_y = (__typeof__(bar->y))foo_x;
if ((void *)bar != (void *)foo)
bar_y = bar->y;
// use bar_y;
In other words, without a barrier, the compiler doesn't have to order
the executed bar->y dereference *instruction* after the bar =
global_ptr_to_bar instruction. Thus making it a compiler property,
not a CPU one.
There is no danger of dereferencing NULL in that example, but
dereferencing the values from the wrong object is just as wrong.
-- Jamie
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
