On Sun, 3 Aug 2014 19:14:18 -0400 Trond Myklebust
<trond.myklebust@xxxxxxxxxxxxxxx> wrote:
> On Sun, Aug 3, 2014 at 7:03 PM, NeilBrown <neilb@xxxxxxx> wrote:
> > On Mon, 04 Aug 2014 06:20:02 +0800 kbuild test robot <fengguang.wu@xxxxxxxxx>
> > wrote:
> >
> >> tree: git://git.linux-nfs.org/projects/trondmy/linux-nfs.git testing
> >> head: f682a398b2e24ae0a775ddf37cced83b897198ee
> >> commit: d51ac1a8e9b86b2d17d349bb256869cab6522787 [56/61] NFS: prepare for RCU-walk support but pushing tests later in code.
> >> reproduce: make C=1 CF=-D__CHECK_ENDIAN__
> >>
> >>
> >> sparse warnings: (new ones prefixed by >>)
> >>
> >> >> fs/nfs/dir.c:1092:26: sparse: incompatible types in comparison expression (different address spaces)
> >> >> fs/nfs/dir.c:1169:31: sparse: incompatible types in comparison expression (different address spaces)
> >>
> >> vim +1092 fs/nfs/dir.c
> >>
> >> 1086 struct nfs_fh *fhandle = NULL;
> >> 1087 struct nfs_fattr *fattr = NULL;
> >> 1088 struct nfs4_label *label = NULL;
> >> 1089 int error;
> >> 1090
> >> 1091 if (flags & LOOKUP_RCU) {
> >> > 1092 parent = rcu_dereference(dentry->d_parent);
> >> 1093 dir = ACCESS_ONCE(parent->d_inode);
> >> 1094 if (!dir)
> >> 1095 return -ECHILD;
> >
> > Hmmm.. I suspect rcu_dereference doesn't really make sense here.
> > After all, d_parent is not assigned with rcu_assign_ptr, and no-one else uses
> > rcu_dereference for it.
> >
> > The issue is that, without locks, d_parent could change at any point.
> > As dentries are freed with call_rcu it is safe to follow any pointers we find,
> > but there is a limit how much we can trust them.
> > It is very likely that any change to d_parent that mattered would increment
> > some seqlock so that RCU-walk would eventually abort.
> >
> >
> > So we may not need the
> >
> >> > 1169 if (parent != rcu_dereference(dentry->d_parent))
> >> 1170 return -ECHILD;
> >
> > at the end, as a seqlock will probably catch any problem.
>
> My main worry with that argument is whether or not the d_seq protected
> lookups are guaranteed to always cover the parent. I can't see
> anything in Documentation/filesystems/path-lookup.txt that indicates
> that they must be.
I'll look harder and come up with something more convincing.
Give me a couple of days ... if this set misses the 3.17 merge window, then
it'll be nice and ready for 3.18 :-)
>
> > Without that we don't even need to store 'parent' at all, just
> > dir = ACCESS_ONCE(dentry->d_parent->d_inode);
> >
> > If we keep it, which is probably safest, then using ACCESS_ONCE in place of
> > the current rcu_dereference() make sense.
> >
> > parent = ACCESS_ONCE(dentry->d_parent);
> > dir = ACCESS_ONCE(dir->d_inode);
> >
> > ...
> >
> > if (parent != ACCESS_ONCE(dentry->d_parent))
> > return -ECHILD;
> >
> >
> > Trond, would you like me to resend that patch, or do you want to just
> > s/rcu_derefence/ACCESS_ONCE/
> > ??
>
> Could you send an incremental patch?
I went back to look at all the rcu_derefs with a refresh perspective. There
are two patches which make the above error, and the rcu_deref in
rpcauth_lookupcred() is wrong.
In fact, the get_groups_info() is unneeded. current_cred()->group_info is
never changed. If the groups for a process change, the current_cred() is
updated, and that is already rcu protected.
So that patch can be significant simplified.
I'll send you incremental patches for anything I find and we can go from
there.
Thanks,
NeilBrown
Attachment:
signature.asc
Description: PGP signature
