[PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[Nakajima Akira]

When refer file "directly" (e.g. ls -li <filename>),
 if file is same name, old inode cache is used.
This causes that client shows wrong(old) inode number.
So this patch is that if uniqueid is different, return error.

## But this patch is applicable to when Server is UNIX.
## When Server is Windows, we need another new patch.


Reproducible sample :
1. create file 'a' at cifs client.
2. rm 'a' and touch 'b a' at server.
3. ls -li 'a' at client, then client shows wrong(old) inode number.

Bug link:
https://bugzilla.kernel.org/show_bug.cgi?id=90021



Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c
--- linux-3.18-vanilla/fs/cifs/inode.c	2014-12-08 07:21:05.000000000 +0900
+++ linux-3.18/fs/cifs/inode.c	2014-12-19 11:07:59.127000000 +0900
@@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod
 			rc = -ENOMEM;
 	} else {
 		/* we already have inode, update it */
+
+		/* if uniqueid is different, return error */
+		if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
+		    CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
+			rc = -ENOENT;
+			goto cgiiu_exit;
+		}
+
 		cifs_fattr_to_inode(*pinode, &fattr);
 	}
 
+cgiiu_exit:
 	return rc;
 }
 

Re: [PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[Shirish Pargaonkar]

Looks correct.

Reviewed-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>


On Tue, Dec 23, 2014 at 8:27 PM, Nakajima Akira
<nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
> When refer file "directly" (e.g. ls -li <filename>),
>  if file is same name, old inode cache is used.
> This causes that client shows wrong(old) inode number.
> So this patch is that if uniqueid is different, return error.
>
> ## But this patch is applicable to when Server is UNIX.
> ## When Server is Windows, we need another new patch.
>
>
> Reproducible sample :
> 1. create file 'a' at cifs client.
> 2. rm 'a' and touch 'b a' at server.
> 3. ls -li 'a' at client, then client shows wrong(old) inode number.
>
> Bug link:
> https://bugzilla.kernel.org/show_bug.cgi?id=90021
>
>
>
> Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
> diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c
> --- linux-3.18-vanilla/fs/cifs/inode.c  2014-12-08 07:21:05.000000000 +0900
> +++ linux-3.18/fs/cifs/inode.c  2014-12-19 11:07:59.127000000 +0900
> @@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod
>                         rc = -ENOMEM;
>         } else {
>                 /* we already have inode, update it */
> +
> +               /* if uniqueid is different, return error */
> +               if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
> +                   CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
> +                       rc = -ENOENT;
> +                       goto cgiiu_exit;
> +               }
> +
>                 cifs_fattr_to_inode(*pinode, &fattr);
>         }
>
> +cgiiu_exit:
>         return rc;
>  }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[Jeff Layton]

On Wed, 24 Dec 2014 11:27:38 +0900
Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:

> When refer file "directly" (e.g. ls -li <filename>),
>  if file is same name, old inode cache is used.
> This causes that client shows wrong(old) inode number.
> So this patch is that if uniqueid is different, return error.
> 
> ## But this patch is applicable to when Server is UNIX.
> ## When Server is Windows, we need another new patch.
> 
> 
> Reproducible sample :
> 1. create file 'a' at cifs client.
> 2. rm 'a' and touch 'b a' at server.
> 3. ls -li 'a' at client, then client shows wrong(old) inode number.
> 
> Bug link:
> https://bugzilla.kernel.org/show_bug.cgi?id=90021
> 
> 
> 
> Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
> diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c
> --- linux-3.18-vanilla/fs/cifs/inode.c	2014-12-08 07:21:05.000000000 +0900
> +++ linux-3.18/fs/cifs/inode.c	2014-12-19 11:07:59.127000000 +0900
> @@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod
>  			rc = -ENOMEM;
>  	} else {
>  		/* we already have inode, update it */
> +
> +		/* if uniqueid is different, return error */
> +		if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
> +		    CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
> +			rc = -ENOENT;
> +			goto cgiiu_exit;
> +		}
> +
>  		cifs_fattr_to_inode(*pinode, &fattr);
>  	}
>  
> +cgiiu_exit:
>  	return rc;
>  }
>  

Returning ENOENT here seems like the wrong error to me. That path does
exist, it just no longer refers to the same file as before.

Maybe ESTALE would be better as it would allow the VFS layer
to revalidate the dcache and invalidate the old dentry?

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[Steve French]

On Tue, Apr 7, 2015 at 5:45 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Wed, 24 Dec 2014 11:27:38 +0900
> Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
>
>> When refer file "directly" (e.g. ls -li <filename>),
>>  if file is same name, old inode cache is used.
>> This causes that client shows wrong(old) inode number.
>> So this patch is that if uniqueid is different, return error.
>>
>> ## But this patch is applicable to when Server is UNIX.
>> ## When Server is Windows, we need another new patch.
>>
>>
>> Reproducible sample :
>> 1. create file 'a' at cifs client.
>> 2. rm 'a' and touch 'b a' at server.
>> 3. ls -li 'a' at client, then client shows wrong(old) inode number.
>>
>> Bug link:
>> https://bugzilla.kernel.org/show_bug.cgi?id=90021
>>
>>
>>
>> Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
>> diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c
>> --- linux-3.18-vanilla/fs/cifs/inode.c        2014-12-08 07:21:05.000000000 +0900
>> +++ linux-3.18/fs/cifs/inode.c        2014-12-19 11:07:59.127000000 +0900
>> @@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod
>>                       rc = -ENOMEM;
>>       } else {
>>               /* we already have inode, update it */
>> +
>> +             /* if uniqueid is different, return error */
>> +             if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
>> +                 CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
>> +                     rc = -ENOENT;
>> +                     goto cgiiu_exit;
>> +             }
>> +
>>               cifs_fattr_to_inode(*pinode, &fattr);
>>       }
>>
>> +cgiiu_exit:
>>       return rc;
>>  }
>>
>
> Returning ENOENT here seems like the wrong error to me. That path does
> exist, it just no longer refers to the same file as before.
>
> Maybe ESTALE would be better as it would allow the VFS layer
> to revalidate the dcache and invalidate the old dentry?
>
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Similar to what Jeff mentioned, isn't the nfs_relavidate_inode path
roughly equivalent to what we want here (where nfs.ko returns ESTALE
on various cases where we detect an inode that doesn't match what we
expect)?

-- 
Thanks,

Steve

Re: [PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[Nakajima Akira]

On 2015/04/07 23:39, Steve French wrote:
> On Tue, Apr 7, 2015 at 5:45 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> On Wed, 24 Dec 2014 11:27:38 +0900
>> Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
>>
>>> When refer file "directly" (e.g. ls -li <filename>),
>>>  if file is same name, old inode cache is used.
>>> This causes that client shows wrong(old) inode number.
>>> So this patch is that if uniqueid is different, return error.
>>>
>>> ## But this patch is applicable to when Server is UNIX.
>>> ## When Server is Windows, we need another new patch.
>>>
>>>
>>> Reproducible sample :
>>> 1. create file 'a' at cifs client.
>>> 2. rm 'a' and touch 'b a' at server.
>>> 3. ls -li 'a' at client, then client shows wrong(old) inode number.
>>>
>>> Bug link:
>>> https://bugzilla.kernel.org/show_bug.cgi?id=90021
>>>
>>>
>>>
>>> Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
>>> diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c
>>> --- linux-3.18-vanilla/fs/cifs/inode.c        2014-12-08 07:21:05.000000000 +0900
>>> +++ linux-3.18/fs/cifs/inode.c        2014-12-19 11:07:59.127000000 +0900
>>> @@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod
>>>                       rc = -ENOMEM;
>>>       } else {
>>>               /* we already have inode, update it */
>>> +
>>> +             /* if uniqueid is different, return error */
>>> +             if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
>>> +                 CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
>>> +                     rc = -ENOENT;
>>> +                     goto cgiiu_exit;
>>> +             }
>>> +
>>>               cifs_fattr_to_inode(*pinode, &fattr);
>>>       }
>>>
>>> +cgiiu_exit:
>>>       return rc;
>>>  }
>>>
>>
>> Returning ENOENT here seems like the wrong error to me. That path does
>> exist, it just no longer refers to the same file as before.
>>
>> Maybe ESTALE would be better as it would allow the VFS layer
>> to revalidate the dcache and invalidate the old dentry?
>>
>> --
>> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> 
> Similar to what Jeff mentioned, isn't the nfs_relavidate_inode path
> roughly equivalent to what we want here (where nfs.ko returns ESTALE
> on various cases where we detect an inode that doesn't match what we
> expect)?

If uniqueid is different, return -ESTALE.
If filetype is different, return -ENOENT.
That's right?

+		/* if filetype is different, return error */
+		if (unlikely(((*pinode)->i_mode & S_IFMT) !=
+		    (fattr.cf_mode & S_IFMT))) {
+			rc = -ENOENT;
+			goto cgiiu_exit;
+		}

Re: [PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[Jeff Layton]

On Thu, 9 Apr 2015 17:07:56 +0900
Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:

> On 2015/04/07 23:39, Steve French wrote:
> > On Tue, Apr 7, 2015 at 5:45 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> >> On Wed, 24 Dec 2014 11:27:38 +0900
> >> Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
> >>
> >>> When refer file "directly" (e.g. ls -li <filename>),
> >>>  if file is same name, old inode cache is used.
> >>> This causes that client shows wrong(old) inode number.
> >>> So this patch is that if uniqueid is different, return error.
> >>>
> >>> ## But this patch is applicable to when Server is UNIX.
> >>> ## When Server is Windows, we need another new patch.
> >>>
> >>>
> >>> Reproducible sample :
> >>> 1. create file 'a' at cifs client.
> >>> 2. rm 'a' and touch 'b a' at server.
> >>> 3. ls -li 'a' at client, then client shows wrong(old) inode number.
> >>>
> >>> Bug link:
> >>> https://bugzilla.kernel.org/show_bug.cgi?id=90021
> >>>
> >>>
> >>>
> >>> Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
> >>> diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c
> >>> --- linux-3.18-vanilla/fs/cifs/inode.c        2014-12-08 07:21:05.000000000 +0900
> >>> +++ linux-3.18/fs/cifs/inode.c        2014-12-19 11:07:59.127000000 +0900
> >>> @@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod
> >>>                       rc = -ENOMEM;
> >>>       } else {
> >>>               /* we already have inode, update it */
> >>> +
> >>> +             /* if uniqueid is different, return error */
> >>> +             if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
> >>> +                 CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
> >>> +                     rc = -ENOENT;
> >>> +                     goto cgiiu_exit;
> >>> +             }
> >>> +
> >>>               cifs_fattr_to_inode(*pinode, &fattr);
> >>>       }
> >>>
> >>> +cgiiu_exit:
> >>>       return rc;
> >>>  }
> >>>
> >>
> >> Returning ENOENT here seems like the wrong error to me. That path does
> >> exist, it just no longer refers to the same file as before.
> >>
> >> Maybe ESTALE would be better as it would allow the VFS layer
> >> to revalidate the dcache and invalidate the old dentry?
> >>
> >> --
> >> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> > 
> > Similar to what Jeff mentioned, isn't the nfs_relavidate_inode path
> > roughly equivalent to what we want here (where nfs.ko returns ESTALE
> > on various cases where we detect an inode that doesn't match what we
> > expect)?
> 
> If uniqueid is different, return -ESTALE.
> If filetype is different, return -ENOENT.
> That's right?
> 
> +		/* if filetype is different, return error */
> +		if (unlikely(((*pinode)->i_mode & S_IFMT) !=
> +		    (fattr.cf_mode & S_IFMT))) {
> +			rc = -ENOENT;
> +			goto cgiiu_exit;
> +		}
> 

No, I don't think so. In both cases, the dcache is wrong and the dentry
should be dropped and reinstantiated to point to a new inode. An ESTALE
return is the trigger for that to occur. An ENOENT return is going to
mean a stat() failure in your testcase, I think.

So I think you want to return ESTALE in both cases. That said, please
do test it and ensure that it does the right thing.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[Shirish Pargaonkar]

I am not sure if ESTALE or ENOENT would have an effect on a dcache entry.
A dcache entry and dentry are two different things, as I understand.
In this case, dcache entry has not changed, what has changed is the dentry,
specifically the inode it points to, so there is really no reason to purge
and reinstate a dcache entry.

On Fri, Apr 10, 2015 at 8:16 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Thu, 9 Apr 2015 17:07:56 +0900
> Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
>
>> On 2015/04/07 23:39, Steve French wrote:
>> > On Tue, Apr 7, 2015 at 5:45 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> >> On Wed, 24 Dec 2014 11:27:38 +0900
>> >> Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
>> >>
>> >>> When refer file "directly" (e.g. ls -li <filename>),
>> >>>  if file is same name, old inode cache is used.
>> >>> This causes that client shows wrong(old) inode number.
>> >>> So this patch is that if uniqueid is different, return error.
>> >>>
>> >>> ## But this patch is applicable to when Server is UNIX.
>> >>> ## When Server is Windows, we need another new patch.
>> >>>
>> >>>
>> >>> Reproducible sample :
>> >>> 1. create file 'a' at cifs client.
>> >>> 2. rm 'a' and touch 'b a' at server.
>> >>> 3. ls -li 'a' at client, then client shows wrong(old) inode number.
>> >>>
>> >>> Bug link:
>> >>> https://bugzilla.kernel.org/show_bug.cgi?id=90021
>> >>>
>> >>>
>> >>>
>> >>> Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
>> >>> diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c
>> >>> --- linux-3.18-vanilla/fs/cifs/inode.c        2014-12-08 07:21:05.000000000 +0900
>> >>> +++ linux-3.18/fs/cifs/inode.c        2014-12-19 11:07:59.127000000 +0900
>> >>> @@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod
>> >>>                       rc = -ENOMEM;
>> >>>       } else {
>> >>>               /* we already have inode, update it */
>> >>> +
>> >>> +             /* if uniqueid is different, return error */
>> >>> +             if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
>> >>> +                 CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
>> >>> +                     rc = -ENOENT;
>> >>> +                     goto cgiiu_exit;
>> >>> +             }
>> >>> +
>> >>>               cifs_fattr_to_inode(*pinode, &fattr);
>> >>>       }
>> >>>
>> >>> +cgiiu_exit:
>> >>>       return rc;
>> >>>  }
>> >>>
>> >>
>> >> Returning ENOENT here seems like the wrong error to me. That path does
>> >> exist, it just no longer refers to the same file as before.
>> >>
>> >> Maybe ESTALE would be better as it would allow the VFS layer
>> >> to revalidate the dcache and invalidate the old dentry?
>> >>
>> >> --
>> >> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>> >
>> > Similar to what Jeff mentioned, isn't the nfs_relavidate_inode path
>> > roughly equivalent to what we want here (where nfs.ko returns ESTALE
>> > on various cases where we detect an inode that doesn't match what we
>> > expect)?
>>
>> If uniqueid is different, return -ESTALE.
>> If filetype is different, return -ENOENT.
>> That's right?
>>
>> +             /* if filetype is different, return error */
>> +             if (unlikely(((*pinode)->i_mode & S_IFMT) !=
>> +                 (fattr.cf_mode & S_IFMT))) {
>> +                     rc = -ENOENT;
>> +                     goto cgiiu_exit;
>> +             }
>>
>
> No, I don't think so. In both cases, the dcache is wrong and the dentry
> should be dropped and reinstantiated to point to a new inode. An ESTALE
> return is the trigger for that to occur. An ENOENT return is going to
> mean a stat() failure in your testcase, I think.
>
> So I think you want to return ESTALE in both cases. That said, please
> do test it and ensure that it does the right thing.
>
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[Jeff Layton]

On Sun, 12 Apr 2015 23:24:59 -0500
Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> I am not sure if ESTALE or ENOENT would have an effect on a dcache entry.
> A dcache entry and dentry are two different things, as I understand.

Oh, in this case I was specifically referring to the kernel's cache of
dentries as the dcache.

> In this case, dcache entry has not changed, what has changed is the dentry,
> specifically the inode it points to, so there is really no reason to purge
> and reinstate a dcache entry.
> 

No, the dentry has changed. We did an operation against the server and
found that the underlying inode type is different.

In practical terms, the Linux dcache should handle that by dropping the
old dentry and instantiating a new one. So, I think that returning
ESTALE is a better error there since it should trigger the upper VFS
layers to do just that.

> On Fri, Apr 10, 2015 at 8:16 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > On Thu, 9 Apr 2015 17:07:56 +0900
> > Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
> >
> >> On 2015/04/07 23:39, Steve French wrote:
> >> > On Tue, Apr 7, 2015 at 5:45 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> >> >> On Wed, 24 Dec 2014 11:27:38 +0900
> >> >> Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
> >> >>
> >> >>> When refer file "directly" (e.g. ls -li <filename>),
> >> >>>  if file is same name, old inode cache is used.
> >> >>> This causes that client shows wrong(old) inode number.
> >> >>> So this patch is that if uniqueid is different, return error.
> >> >>>
> >> >>> ## But this patch is applicable to when Server is UNIX.
> >> >>> ## When Server is Windows, we need another new patch.
> >> >>>
> >> >>>
> >> >>> Reproducible sample :
> >> >>> 1. create file 'a' at cifs client.
> >> >>> 2. rm 'a' and touch 'b a' at server.
> >> >>> 3. ls -li 'a' at client, then client shows wrong(old) inode number.
> >> >>>
> >> >>> Bug link:
> >> >>> https://bugzilla.kernel.org/show_bug.cgi?id=90021
> >> >>>
> >> >>>
> >> >>>
> >> >>> Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
> >> >>> diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c
> >> >>> --- linux-3.18-vanilla/fs/cifs/inode.c        2014-12-08 07:21:05.000000000 +0900
> >> >>> +++ linux-3.18/fs/cifs/inode.c        2014-12-19 11:07:59.127000000 +0900
> >> >>> @@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod
> >> >>>                       rc = -ENOMEM;
> >> >>>       } else {
> >> >>>               /* we already have inode, update it */
> >> >>> +
> >> >>> +             /* if uniqueid is different, return error */
> >> >>> +             if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
> >> >>> +                 CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
> >> >>> +                     rc = -ENOENT;
> >> >>> +                     goto cgiiu_exit;
> >> >>> +             }
> >> >>> +
> >> >>>               cifs_fattr_to_inode(*pinode, &fattr);
> >> >>>       }
> >> >>>
> >> >>> +cgiiu_exit:
> >> >>>       return rc;
> >> >>>  }
> >> >>>
> >> >>
> >> >> Returning ENOENT here seems like the wrong error to me. That path does
> >> >> exist, it just no longer refers to the same file as before.
> >> >>
> >> >> Maybe ESTALE would be better as it would allow the VFS layer
> >> >> to revalidate the dcache and invalidate the old dentry?
> >> >>
> >> >> --
> >> >> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> >> >
> >> > Similar to what Jeff mentioned, isn't the nfs_relavidate_inode path
> >> > roughly equivalent to what we want here (where nfs.ko returns ESTALE
> >> > on various cases where we detect an inode that doesn't match what we
> >> > expect)?
> >>
> >> If uniqueid is different, return -ESTALE.
> >> If filetype is different, return -ENOENT.
> >> That's right?
> >>
> >> +             /* if filetype is different, return error */
> >> +             if (unlikely(((*pinode)->i_mode & S_IFMT) !=
> >> +                 (fattr.cf_mode & S_IFMT))) {
> >> +                     rc = -ENOENT;
> >> +                     goto cgiiu_exit;
> >> +             }
> >>
> >
> > No, I don't think so. In both cases, the dcache is wrong and the dentry
> > should be dropped and reinstantiated to point to a new inode. An ESTALE
> > return is the trigger for that to occur. An ENOENT return is going to
> > mean a stat() failure in your testcase, I think.
> >
> > So I think you want to return ESTALE in both cases. That said, please
> > do test it and ensure that it does the right thing.
> >
> > --
> > Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[Nakajima Akira]

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

On 2015/04/14 5:42, Jeff Layton wrote:
> On Sun, 12 Apr 2015 23:24:59 -0500
> Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>> I am not sure if ESTALE or ENOENT would have an effect on a dcache entry.
>> A dcache entry and dentry are two different things, as I understand.
> 
> Oh, in this case I was specifically referring to the kernel's cache of
> dentries as the dcache.
> 
>> In this case, dcache entry has not changed, what has changed is the dentry,
>> specifically the inode it points to, so there is really no reason to purge
>> and reinstate a dcache entry.
>>
> 
> No, the dentry has changed. We did an operation against the server and
> found that the underlying inode type is different.
> 
> In practical terms, the Linux dcache should handle that by dropping the
> old dentry and instantiating a new one. So, I think that returning
> ESTALE is a better error there since it should trigger the upper VFS
> layers to do just that.
> 
>> On Fri, Apr 10, 2015 at 8:16 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>>> On Thu, 9 Apr 2015 17:07:56 +0900
>>> Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
>>>
>>>> On 2015/04/07 23:39, Steve French wrote:
>>>>> On Tue, Apr 7, 2015 at 5:45 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>>>>>> On Wed, 24 Dec 2014 11:27:38 +0900
>>>>>> Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
>>>>>>
>>>>>>> When refer file "directly" (e.g. ls -li <filename>),
>>>>>>>  if file is same name, old inode cache is used.
>>>>>>> This causes that client shows wrong(old) inode number.
>>>>>>> So this patch is that if uniqueid is different, return error.
>>>>>>>
>>>>>>> ## But this patch is applicable to when Server is UNIX.
>>>>>>> ## When Server is Windows, we need another new patch.
>>>>>>>
>>>>>>>
>>>>>>> Reproducible sample :
>>>>>>> 1. create file 'a' at cifs client.
>>>>>>> 2. rm 'a' and touch 'b a' at server.
>>>>>>> 3. ls -li 'a' at client, then client shows wrong(old) inode number.
>>>>>>>
>>>>>>> Bug link:
>>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=90021
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
>>>>>>> diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c
>>>>>>> --- linux-3.18-vanilla/fs/cifs/inode.c        2014-12-08 07:21:05.000000000 +0900
>>>>>>> +++ linux-3.18/fs/cifs/inode.c        2014-12-19 11:07:59.127000000 +0900
>>>>>>> @@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod
>>>>>>>                       rc = -ENOMEM;
>>>>>>>       } else {
>>>>>>>               /* we already have inode, update it */
>>>>>>> +
>>>>>>> +             /* if uniqueid is different, return error */
>>>>>>> +             if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
>>>>>>> +                 CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
>>>>>>> +                     rc = -ENOENT;
>>>>>>> +                     goto cgiiu_exit;
>>>>>>> +             }
>>>>>>> +
>>>>>>>               cifs_fattr_to_inode(*pinode, &fattr);
>>>>>>>       }
>>>>>>>
>>>>>>> +cgiiu_exit:
>>>>>>>       return rc;
>>>>>>>  }
>>>>>>>
>>>>>>
>>>>>> Returning ENOENT here seems like the wrong error to me. That path does
>>>>>> exist, it just no longer refers to the same file as before.
>>>>>>
>>>>>> Maybe ESTALE would be better as it would allow the VFS layer
>>>>>> to revalidate the dcache and invalidate the old dentry?
>>>>>>
>>>>>> --
>>>>>> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>>>>>
>>>>> Similar to what Jeff mentioned, isn't the nfs_relavidate_inode path
>>>>> roughly equivalent to what we want here (where nfs.ko returns ESTALE
>>>>> on various cases where we detect an inode that doesn't match what we
>>>>> expect)?
>>>>
>>>> If uniqueid is different, return -ESTALE.
>>>> If filetype is different, return -ENOENT.
>>>> That's right?
>>>>
>>>> +             /* if filetype is different, return error */
>>>> +             if (unlikely(((*pinode)->i_mode & S_IFMT) !=
>>>> +                 (fattr.cf_mode & S_IFMT))) {
>>>> +                     rc = -ENOENT;
>>>> +                     goto cgiiu_exit;
>>>> +             }
>>>>
>>>
>>> No, I don't think so. In both cases, the dcache is wrong and the dentry
>>> should be dropped and reinstantiated to point to a new inode. An ESTALE
>>> return is the trigger for that to occur. An ENOENT return is going to
>>> mean a stat() failure in your testcase, I think.
>>>
>>> So I think you want to return ESTALE in both cases. That said, please
>>> do test it and ensure that it does the right thing.
>>>
>>> --
>>> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>


I fixed to return -ESTALE in cifs_get_inode_info_unix().

This case (ls <filename>, cat <filename>) passes through following paths.
ls <filename>
-> lookup_fast
-> .d_revalidate
-> cifs_d_revalidate          (even though EATALE or ENOENT, return 0)
-> cifs_revalidate_dentry
-> cifs_revalidate_dentry_attr
-> cifs_get_inode_info_unix   (return ESTALE)

In either ESTALE or ENOENT, cifs_d_revalidate() returns 0.


I didn't find out
 what commands path through
 other passes not including cifs_d_revalidate().
   (cifs_do_create, cifs_mknod, cifs_lookup, cifs_symlink, etc..) 
But I checked that this patch works properly by various commands/patterns.




>From 0ff83baa2069c86aa35a8081cdb6e4f4380e66a1 Mon Sep 17 00:00:00 2001
From: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
Date: Wed, 22 Apr 2015 15:24:44 +0900
Subject: [PATCH] Fix to check Unique id and FileType when client refer file directly.

When you refer file directly on cifs client,
 (e.g. ls -li <filename>, cd <dir>, stat <filename>)
 the function return old inode number and filetype from old inode cache,
 though server has different inode number or filetype.

When server is Windows, cifs client has same problem.
When Server is Windows
, This patch fixes bug in different filetype, 
  but does not fix bug in different inode number.
Because QUERY_PATH_INFO response by Windows does not include inode number(Index Number) .

BUG INFO
https://bugzilla.kernel.org/show_bug.cgi?id=90021
https://bugzilla.kernel.org/show_bug.cgi?id=90031

Reported-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
Reviewed-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

---
 fs/cifs/inode.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3e126d7..0d42354 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -402,9 +402,25 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 			rc = -ENOMEM;
 	} else {
 		/* we already have inode, update it */
+
+		/* if uniqueid is different, return error */
+		if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
+		    CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
+			rc = -ESTALE;
+			goto cgiiu_exit;
+		}
+
+		/* if filetype is different, return error */
+		if (unlikely(((*pinode)->i_mode & S_IFMT) !=
+		    (fattr.cf_mode & S_IFMT))) {
+			rc = -ESTALE;
+			goto cgiiu_exit;
+		}
+
 		cifs_fattr_to_inode(*pinode, &fattr);
 	}
 
+cgiiu_exit:
 	return rc;
 }
 
@@ -839,6 +855,15 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
 		if (!*inode)
 			rc = -ENOMEM;
 	} else {
+		/* we already have inode, update it */
+
+		/* if filetype is different, return error */
+		if (unlikely(((*inode)->i_mode & S_IFMT) !=
+		    (fattr.cf_mode & S_IFMT))) {
+			rc = -ESTALE;
+			goto cgii_exit;
+		}
+
 		cifs_fattr_to_inode(*inode, &fattr);
 	}
 
-- 
1.7.1


[-- Attachment #2: 0001-Fix-to-check-Unique-id-and-FileType-when-client-refe.patch --]
[-- Type: text/plain, Size: 2287 bytes --]

From 0ff83baa2069c86aa35a8081cdb6e4f4380e66a1 Mon Sep 17 00:00:00 2001
From: Nakajima Akira <nakajima.akira@nttcom.co.jp>
Date: Wed, 22 Apr 2015 15:24:44 +0900
Subject: [PATCH] Fix to check Unique id and FileType when client refer file directly.

When you refer file directly on cifs client,
 (e.g. ls -li <filename>, cd <dir>, stat <filename>)
 the function return old inode number and filetype from old inode cache,
 though server has different inode number or filetype.

When server is Windows, cifs client has same problem.
When Server is Windows
, This patch fixes bug in different filetype, 
  but does not fix bug in different inode number.
Because QUERY_PATH_INFO response by Windows does not include inode number(Index Number) .

BUG INFO
https://bugzilla.kernel.org/show_bug.cgi?id=90021
https://bugzilla.kernel.org/show_bug.cgi?id=90031

Reported-by: Nakajima Akira <nakajima.akira@nttcom.co.jp>
Signed-off-by: Nakajima Akira <nakajima.akira@nttcom.co.jp>
Reviewed-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>

---
 fs/cifs/inode.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3e126d7..0d42354 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -402,9 +402,25 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 			rc = -ENOMEM;
 	} else {
 		/* we already have inode, update it */
+
+		/* if uniqueid is different, return error */
+		if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
+		    CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
+			rc = -ESTALE;
+			goto cgiiu_exit;
+		}
+
+		/* if filetype is different, return error */
+		if (unlikely(((*pinode)->i_mode & S_IFMT) !=
+		    (fattr.cf_mode & S_IFMT))) {
+			rc = -ESTALE;
+			goto cgiiu_exit;
+		}
+
 		cifs_fattr_to_inode(*pinode, &fattr);
 	}
 
+cgiiu_exit:
 	return rc;
 }
 
@@ -839,6 +855,15 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
 		if (!*inode)
 			rc = -ENOMEM;
 	} else {
+		/* we already have inode, update it */
+
+		/* if filetype is different, return error */
+		if (unlikely(((*inode)->i_mode & S_IFMT) !=
+		    (fattr.cf_mode & S_IFMT))) {
+			rc = -ESTALE;
+			goto cgii_exit;
+		}
+
 		cifs_fattr_to_inode(*inode, &fattr);
 	}
 
-- 
1.7.1

Fwd: [PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[Steve French]

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

Adding linux-fsdevel since this case is similar to what other
network/cluster fs have to deal with


---------- Forwarded message ----------
From: Nakajima Akira <nakajima.akira@nttcom.co.jp>
Date: Wed, Apr 22, 2015 at 1:31 AM
Subject: Re: [PATCH] cifs: When "refer file directly", make new inode
cache if "uniqueid is different"
To: "linux-cifs@vger.kernel.org" <linux-cifs@vger.kernel.org>


On 2015/04/14 5:42, Jeff Layton wrote:
> On Sun, 12 Apr 2015 23:24:59 -0500
> Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:
>
>> I am not sure if ESTALE or ENOENT would have an effect on a dcache entry.
>> A dcache entry and dentry are two different things, as I understand.
>
> Oh, in this case I was specifically referring to the kernel's cache of
> dentries as the dcache.
>
>> In this case, dcache entry has not changed, what has changed is the dentry,
>> specifically the inode it points to, so there is really no reason to purge
>> and reinstate a dcache entry.
>>
>
> No, the dentry has changed. We did an operation against the server and
> found that the underlying inode type is different.
>
> In practical terms, the Linux dcache should handle that by dropping the
> old dentry and instantiating a new one. So, I think that returning
> ESTALE is a better error there since it should trigger the upper VFS
> layers to do just that.
>
>> On Fri, Apr 10, 2015 at 8:16 AM, Jeff Layton <jlayton@samba.org> wrote:
>>> On Thu, 9 Apr 2015 17:07:56 +0900
>>> Nakajima Akira <nakajima.akira@nttcom.co.jp> wrote:
>>>
>>>> On 2015/04/07 23:39, Steve French wrote:
>>>>> On Tue, Apr 7, 2015 at 5:45 AM, Jeff Layton <jlayton@samba.org> wrote:
>>>>>> On Wed, 24 Dec 2014 11:27:38 +0900
>>>>>> Nakajima Akira <nakajima.akira@nttcom.co.jp> wrote:
>>>>>>
>>>>>>> When refer file "directly" (e.g. ls -li <filename>),
>>>>>>>  if file is same name, old inode cache is used.
>>>>>>> This causes that client shows wrong(old) inode number.
>>>>>>> So this patch is that if uniqueid is different, return error.
>>>>>>>
>>>>>>> ## But this patch is applicable to when Server is UNIX.
>>>>>>> ## When Server is Windows, we need another new patch.
>>>>>>>
>>>>>>>
>>>>>>> Reproducible sample :
>>>>>>> 1. create file 'a' at cifs client.
>>>>>>> 2. rm 'a' and touch 'b a' at server.
>>>>>>> 3. ls -li 'a' at client, then client shows wrong(old) inode number.
>>>>>>>
>>>>>>> Bug link:
>>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=90021
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Nakajima Akira <nakajima.akira@nttcom.co.jp>
>>>>>>> diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c
>>>>>>> --- linux-3.18-vanilla/fs/cifs/inode.c        2014-12-08 07:21:05.000000000 +0900
>>>>>>> +++ linux-3.18/fs/cifs/inode.c        2014-12-19 11:07:59.127000000 +0900
>>>>>>> @@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod
>>>>>>>                       rc = -ENOMEM;
>>>>>>>       } else {
>>>>>>>               /* we already have inode, update it */
>>>>>>> +
>>>>>>> +             /* if uniqueid is different, return error */
>>>>>>> +             if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
>>>>>>> +                 CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
>>>>>>> +                     rc = -ENOENT;
>>>>>>> +                     goto cgiiu_exit;
>>>>>>> +             }
>>>>>>> +
>>>>>>>               cifs_fattr_to_inode(*pinode, &fattr);
>>>>>>>       }
>>>>>>>
>>>>>>> +cgiiu_exit:
>>>>>>>       return rc;
>>>>>>>  }
>>>>>>>
>>>>>>
>>>>>> Returning ENOENT here seems like the wrong error to me. That path does
>>>>>> exist, it just no longer refers to the same file as before.
>>>>>>
>>>>>> Maybe ESTALE would be better as it would allow the VFS layer
>>>>>> to revalidate the dcache and invalidate the old dentry?
>>>>>>
>>>>>> --
>>>>>> Jeff Layton <jlayton@samba.org>
>>>>>
>>>>> Similar to what Jeff mentioned, isn't the nfs_relavidate_inode path
>>>>> roughly equivalent to what we want here (where nfs.ko returns ESTALE
>>>>> on various cases where we detect an inode that doesn't match what we
>>>>> expect)?
>>>>
>>>> If uniqueid is different, return -ESTALE.
>>>> If filetype is different, return -ENOENT.
>>>> That's right?
>>>>
>>>> +             /* if filetype is different, return error */
>>>> +             if (unlikely(((*pinode)->i_mode & S_IFMT) !=
>>>> +                 (fattr.cf_mode & S_IFMT))) {
>>>> +                     rc = -ENOENT;
>>>> +                     goto cgiiu_exit;
>>>> +             }
>>>>
>>>
>>> No, I don't think so. In both cases, the dcache is wrong and the dentry
>>> should be dropped and reinstantiated to point to a new inode. An ESTALE
>>> return is the trigger for that to occur. An ENOENT return is going to
>>> mean a stat() failure in your testcase, I think.
>>>
>>> So I think you want to return ESTALE in both cases. That said, please
>>> do test it and ensure that it does the right thing.
>>>
>>> --
>>> Jeff Layton <jlayton@samba.org>


I fixed to return -ESTALE in cifs_get_inode_info_unix().

This case (ls <filename>, cat <filename>) passes through following paths.
ls <filename>
-> lookup_fast
-> .d_revalidate
-> cifs_d_revalidate          (even though EATALE or ENOENT, return 0)
-> cifs_revalidate_dentry
-> cifs_revalidate_dentry_attr
-> cifs_get_inode_info_unix   (return ESTALE)

In either ESTALE or ENOENT, cifs_d_revalidate() returns 0.


I didn't find out
 what commands path through
 other passes not including cifs_d_revalidate().
   (cifs_do_create, cifs_mknod, cifs_lookup, cifs_symlink, etc..)
But I checked that this patch works properly by various commands/patterns.




>From 0ff83baa2069c86aa35a8081cdb6e4f4380e66a1 Mon Sep 17 00:00:00 2001
From: Nakajima Akira <nakajima.akira@nttcom.co.jp>
Date: Wed, 22 Apr 2015 15:24:44 +0900
Subject: [PATCH] Fix to check Unique id and FileType when client refer
file directly.

When you refer file directly on cifs client,
 (e.g. ls -li <filename>, cd <dir>, stat <filename>)
 the function return old inode number and filetype from old inode cache,
 though server has different inode number or filetype.

When server is Windows, cifs client has same problem.
When Server is Windows
, This patch fixes bug in different filetype,
  but does not fix bug in different inode number.
Because QUERY_PATH_INFO response by Windows does not include inode
number(Index Number) .

BUG INFO
https://bugzilla.kernel.org/show_bug.cgi?id=90021
https://bugzilla.kernel.org/show_bug.cgi?id=90031

Reported-by: Nakajima Akira <nakajima.akira@nttcom.co.jp>
Signed-off-by: Nakajima Akira <nakajima.akira@nttcom.co.jp>
Reviewed-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>

---
 fs/cifs/inode.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3e126d7..0d42354 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -402,9 +402,25 @@ int cifs_get_inode_info_unix(struct inode **pinode,
                        rc = -ENOMEM;
        } else {
                /* we already have inode, update it */
+
+               /* if uniqueid is different, return error */
+               if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
+                   CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
+                       rc = -ESTALE;
+                       goto cgiiu_exit;
+               }
+
+               /* if filetype is different, return error */
+               if (unlikely(((*pinode)->i_mode & S_IFMT) !=
+                   (fattr.cf_mode & S_IFMT))) {
+                       rc = -ESTALE;
+                       goto cgiiu_exit;
+               }
+
                cifs_fattr_to_inode(*pinode, &fattr);
        }

+cgiiu_exit:
        return rc;
 }

@@ -839,6 +855,15 @@ cifs_get_inode_info(struct inode **inode, const
char *full_path,
                if (!*inode)
                        rc = -ENOMEM;
        } else {
+               /* we already have inode, update it */
+
+               /* if filetype is different, return error */
+               if (unlikely(((*inode)->i_mode & S_IFMT) !=
+                   (fattr.cf_mode & S_IFMT))) {
+                       rc = -ESTALE;
+                       goto cgii_exit;
+               }
+
                cifs_fattr_to_inode(*inode, &fattr);
        }

--
1.7.1



-- 
Thanks,

Steve

[-- Attachment #2: 0001-Fix-to-check-Unique-id-and-FileType-when-client-refe.patch --]
[-- Type: text/plain, Size: 2287 bytes --]

From 0ff83baa2069c86aa35a8081cdb6e4f4380e66a1 Mon Sep 17 00:00:00 2001
From: Nakajima Akira <nakajima.akira@nttcom.co.jp>
Date: Wed, 22 Apr 2015 15:24:44 +0900
Subject: [PATCH] Fix to check Unique id and FileType when client refer file directly.

When you refer file directly on cifs client,
 (e.g. ls -li <filename>, cd <dir>, stat <filename>)
 the function return old inode number and filetype from old inode cache,
 though server has different inode number or filetype.

When server is Windows, cifs client has same problem.
When Server is Windows
, This patch fixes bug in different filetype, 
  but does not fix bug in different inode number.
Because QUERY_PATH_INFO response by Windows does not include inode number(Index Number) .

BUG INFO
https://bugzilla.kernel.org/show_bug.cgi?id=90021
https://bugzilla.kernel.org/show_bug.cgi?id=90031

Reported-by: Nakajima Akira <nakajima.akira@nttcom.co.jp>
Signed-off-by: Nakajima Akira <nakajima.akira@nttcom.co.jp>
Reviewed-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>

---
 fs/cifs/inode.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
index 3e126d7..0d42354 100644
--- a/fs/cifs/inode.c
+++ b/fs/cifs/inode.c
@@ -402,9 +402,25 @@ int cifs_get_inode_info_unix(struct inode **pinode,
 			rc = -ENOMEM;
 	} else {
 		/* we already have inode, update it */
+
+		/* if uniqueid is different, return error */
+		if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
+		    CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
+			rc = -ESTALE;
+			goto cgiiu_exit;
+		}
+
+		/* if filetype is different, return error */
+		if (unlikely(((*pinode)->i_mode & S_IFMT) !=
+		    (fattr.cf_mode & S_IFMT))) {
+			rc = -ESTALE;
+			goto cgiiu_exit;
+		}
+
 		cifs_fattr_to_inode(*pinode, &fattr);
 	}
 
+cgiiu_exit:
 	return rc;
 }
 
@@ -839,6 +855,15 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
 		if (!*inode)
 			rc = -ENOMEM;
 	} else {
+		/* we already have inode, update it */
+
+		/* if filetype is different, return error */
+		if (unlikely(((*inode)->i_mode & S_IFMT) !=
+		    (fattr.cf_mode & S_IFMT))) {
+			rc = -ESTALE;
+			goto cgii_exit;
+		}
+
 		cifs_fattr_to_inode(*inode, &fattr);
 	}
 
-- 
1.7.1

Re: [PATCH] cifs: When "refer file directly", make new inode cache if "uniqueid is different"

[Steve French]

Merged into cifs-2.6.git for-next

On Wed, Apr 22, 2015 at 1:31 AM, Nakajima Akira
<nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
> On 2015/04/14 5:42, Jeff Layton wrote:
>> On Sun, 12 Apr 2015 23:24:59 -0500
>> Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>> I am not sure if ESTALE or ENOENT would have an effect on a dcache entry.
>>> A dcache entry and dentry are two different things, as I understand.
>>
>> Oh, in this case I was specifically referring to the kernel's cache of
>> dentries as the dcache.
>>
>>> In this case, dcache entry has not changed, what has changed is the dentry,
>>> specifically the inode it points to, so there is really no reason to purge
>>> and reinstate a dcache entry.
>>>
>>
>> No, the dentry has changed. We did an operation against the server and
>> found that the underlying inode type is different.
>>
>> In practical terms, the Linux dcache should handle that by dropping the
>> old dentry and instantiating a new one. So, I think that returning
>> ESTALE is a better error there since it should trigger the upper VFS
>> layers to do just that.
>>
>>> On Fri, Apr 10, 2015 at 8:16 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>>>> On Thu, 9 Apr 2015 17:07:56 +0900
>>>> Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
>>>>
>>>>> On 2015/04/07 23:39, Steve French wrote:
>>>>>> On Tue, Apr 7, 2015 at 5:45 AM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>>>>>>> On Wed, 24 Dec 2014 11:27:38 +0900
>>>>>>> Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org> wrote:
>>>>>>>
>>>>>>>> When refer file "directly" (e.g. ls -li <filename>),
>>>>>>>>  if file is same name, old inode cache is used.
>>>>>>>> This causes that client shows wrong(old) inode number.
>>>>>>>> So this patch is that if uniqueid is different, return error.
>>>>>>>>
>>>>>>>> ## But this patch is applicable to when Server is UNIX.
>>>>>>>> ## When Server is Windows, we need another new patch.
>>>>>>>>
>>>>>>>>
>>>>>>>> Reproducible sample :
>>>>>>>> 1. create file 'a' at cifs client.
>>>>>>>> 2. rm 'a' and touch 'b a' at server.
>>>>>>>> 3. ls -li 'a' at client, then client shows wrong(old) inode number.
>>>>>>>>
>>>>>>>> Bug link:
>>>>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=90021
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
>>>>>>>> diff -uprN -X linux-3.18-vanilla/Documentation/dontdiff linux-3.18-vanilla/fs/cifs/inode.c linux-3.18/fs/cifs/inode.c
>>>>>>>> --- linux-3.18-vanilla/fs/cifs/inode.c        2014-12-08 07:21:05.000000000 +0900
>>>>>>>> +++ linux-3.18/fs/cifs/inode.c        2014-12-19 11:07:59.127000000 +0900
>>>>>>>> @@ -402,9 +402,18 @@ int cifs_get_inode_info_unix(struct inod
>>>>>>>>                       rc = -ENOMEM;
>>>>>>>>       } else {
>>>>>>>>               /* we already have inode, update it */
>>>>>>>> +
>>>>>>>> +             /* if uniqueid is different, return error */
>>>>>>>> +             if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
>>>>>>>> +                 CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
>>>>>>>> +                     rc = -ENOENT;
>>>>>>>> +                     goto cgiiu_exit;
>>>>>>>> +             }
>>>>>>>> +
>>>>>>>>               cifs_fattr_to_inode(*pinode, &fattr);
>>>>>>>>       }
>>>>>>>>
>>>>>>>> +cgiiu_exit:
>>>>>>>>       return rc;
>>>>>>>>  }
>>>>>>>>
>>>>>>>
>>>>>>> Returning ENOENT here seems like the wrong error to me. That path does
>>>>>>> exist, it just no longer refers to the same file as before.
>>>>>>>
>>>>>>> Maybe ESTALE would be better as it would allow the VFS layer
>>>>>>> to revalidate the dcache and invalidate the old dentry?
>>>>>>>
>>>>>>> --
>>>>>>> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>>>>>>
>>>>>> Similar to what Jeff mentioned, isn't the nfs_relavidate_inode path
>>>>>> roughly equivalent to what we want here (where nfs.ko returns ESTALE
>>>>>> on various cases where we detect an inode that doesn't match what we
>>>>>> expect)?
>>>>>
>>>>> If uniqueid is different, return -ESTALE.
>>>>> If filetype is different, return -ENOENT.
>>>>> That's right?
>>>>>
>>>>> +             /* if filetype is different, return error */
>>>>> +             if (unlikely(((*pinode)->i_mode & S_IFMT) !=
>>>>> +                 (fattr.cf_mode & S_IFMT))) {
>>>>> +                     rc = -ENOENT;
>>>>> +                     goto cgiiu_exit;
>>>>> +             }
>>>>>
>>>>
>>>> No, I don't think so. In both cases, the dcache is wrong and the dentry
>>>> should be dropped and reinstantiated to point to a new inode. An ESTALE
>>>> return is the trigger for that to occur. An ENOENT return is going to
>>>> mean a stat() failure in your testcase, I think.
>>>>
>>>> So I think you want to return ESTALE in both cases. That said, please
>>>> do test it and ensure that it does the right thing.
>>>>
>>>> --
>>>> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
>
>
> I fixed to return -ESTALE in cifs_get_inode_info_unix().
>
> This case (ls <filename>, cat <filename>) passes through following paths.
> ls <filename>
> -> lookup_fast
> -> .d_revalidate
> -> cifs_d_revalidate          (even though EATALE or ENOENT, return 0)
> -> cifs_revalidate_dentry
> -> cifs_revalidate_dentry_attr
> -> cifs_get_inode_info_unix   (return ESTALE)
>
> In either ESTALE or ENOENT, cifs_d_revalidate() returns 0.
>
>
> I didn't find out
>  what commands path through
>  other passes not including cifs_d_revalidate().
>    (cifs_do_create, cifs_mknod, cifs_lookup, cifs_symlink, etc..)
> But I checked that this patch works properly by various commands/patterns.
>
>
>
>
> From 0ff83baa2069c86aa35a8081cdb6e4f4380e66a1 Mon Sep 17 00:00:00 2001
> From: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
> Date: Wed, 22 Apr 2015 15:24:44 +0900
> Subject: [PATCH] Fix to check Unique id and FileType when client refer file directly.
>
> When you refer file directly on cifs client,
>  (e.g. ls -li <filename>, cd <dir>, stat <filename>)
>  the function return old inode number and filetype from old inode cache,
>  though server has different inode number or filetype.
>
> When server is Windows, cifs client has same problem.
> When Server is Windows
> , This patch fixes bug in different filetype,
>   but does not fix bug in different inode number.
> Because QUERY_PATH_INFO response by Windows does not include inode number(Index Number) .
>
> BUG INFO
> https://bugzilla.kernel.org/show_bug.cgi?id=90021
> https://bugzilla.kernel.org/show_bug.cgi?id=90031
>
> Reported-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
> Signed-off-by: Nakajima Akira <nakajima.akira-o7dWnD6vFTHqq2nvvmkE/A@public.gmane.org>
> Reviewed-by: Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> ---
>  fs/cifs/inode.c |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c
> index 3e126d7..0d42354 100644
> --- a/fs/cifs/inode.c
> +++ b/fs/cifs/inode.c
> @@ -402,9 +402,25 @@ int cifs_get_inode_info_unix(struct inode **pinode,
>                         rc = -ENOMEM;
>         } else {
>                 /* we already have inode, update it */
> +
> +               /* if uniqueid is different, return error */
> +               if (unlikely(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_SERVER_INUM &&
> +                   CIFS_I(*pinode)->uniqueid != fattr.cf_uniqueid)) {
> +                       rc = -ESTALE;
> +                       goto cgiiu_exit;
> +               }
> +
> +               /* if filetype is different, return error */
> +               if (unlikely(((*pinode)->i_mode & S_IFMT) !=
> +                   (fattr.cf_mode & S_IFMT))) {
> +                       rc = -ESTALE;
> +                       goto cgiiu_exit;
> +               }
> +
>                 cifs_fattr_to_inode(*pinode, &fattr);
>         }
>
> +cgiiu_exit:
>         return rc;
>  }
>
> @@ -839,6 +855,15 @@ cifs_get_inode_info(struct inode **inode, const char *full_path,
>                 if (!*inode)
>                         rc = -ENOMEM;
>         } else {
> +               /* we already have inode, update it */
> +
> +               /* if filetype is different, return error */
> +               if (unlikely(((*inode)->i_mode & S_IFMT) !=
> +                   (fattr.cf_mode & S_IFMT))) {
> +                       rc = -ESTALE;
> +                       goto cgii_exit;
> +               }
> +
>                 cifs_fattr_to_inode(*inode, &fattr);
>         }
>
> --
> 1.7.1
>



-- 
Thanks,

Steve