Re: [nfs:testing 56/61] fs/nfs/dir.c:1092:26: sparse: incompatible types in comparison expression (different address spaces)

Re: [nfs:testing 56/61] fs/nfs/dir.c:1092:26: sparse: incompatible types in comparison expression (different address spaces)

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]

On Mon, 04 Aug 2014 06:20:02 +0800 kbuild test robot <fengguang.wu@intel.com>
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.
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/
??

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [nfs:testing 56/61] fs/nfs/dir.c:1092:26: sparse: incompatible types in comparison expression (different address spaces)

[Trond Myklebust]

On Sun, Aug 3, 2014 at 7:03 PM, NeilBrown <neilb@suse.de> wrote:
> On Mon, 04 Aug 2014 06:20:02 +0800 kbuild test robot <fengguang.wu@intel.com>
> 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.

> 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?

Thanks!
  Trond


-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: [nfs:testing 56/61] fs/nfs/dir.c:1092:26: sparse: incompatible types in comparison expression (different address spaces)

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3829 bytes --]

On Sun, 3 Aug 2014 19:14:18 -0400 Trond Myklebust
<trond.myklebust@primarydata.com> wrote:

> On Sun, Aug 3, 2014 at 7:03 PM, NeilBrown <neilb@suse.de> wrote:
> > On Mon, 04 Aug 2014 06:20:02 +0800 kbuild test robot <fengguang.wu@intel.com>
> > 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [nfs:testing 56/61] fs/nfs/dir.c:1092:26: sparse: incompatible types in comparison expression (different address spaces)

[Trond Myklebust]

On Sun, Aug 3, 2014 at 7:34 PM, NeilBrown <neilb@suse.de> wrote:
> On Sun, 3 Aug 2014 19:14:18 -0400 Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>
>> On Sun, Aug 3, 2014 at 7:03 PM, NeilBrown <neilb@suse.de> wrote:
>> > On Mon, 04 Aug 2014 06:20:02 +0800 kbuild test robot <fengguang.wu@intel.com>
>> > 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 :-)

I'm leaning towards believing that the code is safe, simply because
you do that comparison between the cached 'parent' pointer and the
same dereferenced pointer at the end, and you don't really use
'parent' for anything else.

It would be nice to get rid of the sparse warning though...

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

That sounds good.

Thanks!
  Trond


-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com