[PATCH] cifs - check S_AUTOMOUNT in revalidate

[PATCH] cifs - check S_AUTOMOUNT in revalidate

[Ian Kent]

When revalidating a dentry, if the inode wasn't known to be a dfs
entry when the dentry was instantiated, such as when created via
->readdir(), the DCACHE_NEED_AUTOMOUNT flag needs to be set on the
dentry in ->d_revalidate().

The false return from cifs_d_revalidate(), due to the inode now
being marked with the S_AUTOMOUNT flag, might not invalidate the
dentry if there is a concurrent unlazy path walk. This is because
the dentry reference count will be at least 2 in this case causing
d_invalidate() to return EBUSY. So the asumption that the dentry
will be discarded then correctly instantiated via ->lookup() might
not hold.

Signed-off-by: Ian Kent <raven@themaw.net>
Reviewed-by: Jeff Layton <jlayton@redhat.com>
Cc: Steve French <smfrench@gmail.com>
Cc: linux-cifs@vger.kernel.org
---

 fs/cifs/dir.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index d172c8e..ec4e9a2 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -668,12 +668,19 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
 			return 0;
 		else {
 			/*
-			 * Forcibly invalidate automounting directory inodes
-			 * (remote DFS directories) so to have them
-			 * instantiated again for automount
+			 * If the inode wasn't known to be a dfs entry when
+			 * the dentry was instantiated, such as when created
+			 * via ->readdir(), it needs to be set now since the
+			 * attributes will have been updated by
+			 * cifs_revalidate_dentry().
 			 */
-			if (IS_AUTOMOUNT(direntry->d_inode))
-				return 0;
+			if (IS_AUTOMOUNT(direntry->d_inode) &&
+			   !(direntry->d_flags & DCACHE_NEED_AUTOMOUNT)) {
+				spin_lock(&direntry->d_lock);
+				direntry->d_flags |= DCACHE_NEED_AUTOMOUNT;
+				spin_unlock(&direntry->d_lock);
+			}
+
 			return 1;
 		}
 	}

Re: [PATCH] cifs - check S_AUTOMOUNT in revalidate

[Linus Torvalds]

Steve? I have a CIFS pull request from you now, and it doesn't contain
this one. I don't know the code well enough to say one way or the
other, so I haven't applied this. Pls Ack/Nak.

                   Linus

On Fri, Apr 27, 2012 at 1:18 AM, Ian Kent <raven@themaw.net> wrote:
> When revalidating a dentry, if the inode wasn't known to be a dfs
> entry when the dentry was instantiated, such as when created via
> ->readdir(), the DCACHE_NEED_AUTOMOUNT flag needs to be set on the
> dentry in ->d_revalidate().
>
> The false return from cifs_d_revalidate(), due to the inode now
> being marked with the S_AUTOMOUNT flag, might not invalidate the
> dentry if there is a concurrent unlazy path walk. This is because
> the dentry reference count will be at least 2 in this case causing
> d_invalidate() to return EBUSY. So the asumption that the dentry
> will be discarded then correctly instantiated via ->lookup() might
> not hold.
>
> Signed-off-by: Ian Kent <raven@themaw.net>
> Reviewed-by: Jeff Layton <jlayton@redhat.com>
> Cc: Steve French <smfrench@gmail.com>
> Cc: linux-cifs@vger.kernel.org
> ---
>
>  fs/cifs/dir.c |   17 ++++++++++++-----
>  1 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> index d172c8e..ec4e9a2 100644
> --- a/fs/cifs/dir.c
> +++ b/fs/cifs/dir.c
> @@ -668,12 +668,19 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
>                        return 0;
>                else {
>                        /*
> -                        * Forcibly invalidate automounting directory inodes
> -                        * (remote DFS directories) so to have them
> -                        * instantiated again for automount
> +                        * If the inode wasn't known to be a dfs entry when
> +                        * the dentry was instantiated, such as when created
> +                        * via ->readdir(), it needs to be set now since the
> +                        * attributes will have been updated by
> +                        * cifs_revalidate_dentry().
>                         */
> -                       if (IS_AUTOMOUNT(direntry->d_inode))
> -                               return 0;
> +                       if (IS_AUTOMOUNT(direntry->d_inode) &&
> +                          !(direntry->d_flags & DCACHE_NEED_AUTOMOUNT)) {
> +                               spin_lock(&direntry->d_lock);
> +                               direntry->d_flags |= DCACHE_NEED_AUTOMOUNT;
> +                               spin_unlock(&direntry->d_lock);
> +                       }
> +
>                        return 1;
>                }
>        }
>

Re: [PATCH] cifs - check S_AUTOMOUNT in revalidate

[Steve French]

The fix makes sense, but it is fairly recent and I haven't had a
chance to try it,
so unless a new release is imminent, I would prefer to put in the next merge
request (I have at least one more fix likely as well) next week.

On Fri, Apr 27, 2012 at 9:46 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Steve? I have a CIFS pull request from you now, and it doesn't contain
> this one. I don't know the code well enough to say one way or the
> other, so I haven't applied this. Pls Ack/Nak.
>
>                   Linus
>
> On Fri, Apr 27, 2012 at 1:18 AM, Ian Kent <raven@themaw.net> wrote:
>> When revalidating a dentry, if the inode wasn't known to be a dfs
>> entry when the dentry was instantiated, such as when created via
>> ->readdir(), the DCACHE_NEED_AUTOMOUNT flag needs to be set on the
>> dentry in ->d_revalidate().
>>
>> The false return from cifs_d_revalidate(), due to the inode now
>> being marked with the S_AUTOMOUNT flag, might not invalidate the
>> dentry if there is a concurrent unlazy path walk. This is because
>> the dentry reference count will be at least 2 in this case causing
>> d_invalidate() to return EBUSY. So the asumption that the dentry
>> will be discarded then correctly instantiated via ->lookup() might
>> not hold.
>>
>> Signed-off-by: Ian Kent <raven@themaw.net>
>> Reviewed-by: Jeff Layton <jlayton@redhat.com>
>> Cc: Steve French <smfrench@gmail.com>
>> Cc: linux-cifs@vger.kernel.org
>> ---
>>
>>  fs/cifs/dir.c |   17 ++++++++++++-----
>>  1 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
>> index d172c8e..ec4e9a2 100644
>> --- a/fs/cifs/dir.c
>> +++ b/fs/cifs/dir.c
>> @@ -668,12 +668,19 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
>>                        return 0;
>>                else {
>>                        /*
>> -                        * Forcibly invalidate automounting directory inodes
>> -                        * (remote DFS directories) so to have them
>> -                        * instantiated again for automount
>> +                        * If the inode wasn't known to be a dfs entry when
>> +                        * the dentry was instantiated, such as when created
>> +                        * via ->readdir(), it needs to be set now since the
>> +                        * attributes will have been updated by
>> +                        * cifs_revalidate_dentry().
>>                         */
>> -                       if (IS_AUTOMOUNT(direntry->d_inode))
>> -                               return 0;
>> +                       if (IS_AUTOMOUNT(direntry->d_inode) &&
>> +                          !(direntry->d_flags & DCACHE_NEED_AUTOMOUNT)) {
>> +                               spin_lock(&direntry->d_lock);
>> +                               direntry->d_flags |= DCACHE_NEED_AUTOMOUNT;
>> +                               spin_unlock(&direntry->d_lock);
>> +                       }
>> +
>>                        return 1;
>>                }
>>        }
>>



-- 
Thanks,

Steve

Re: [PATCH] cifs - check S_AUTOMOUNT in revalidate

[Linus Torvalds]

On Fri, Apr 27, 2012 at 8:33 PM, Steve French <smfrench@gmail.com> wrote:
> The fix makes sense, but it is fairly recent and I haven't had a
> chance to try it,
> so unless a new release is imminent, I would prefer to put in the next merge
> request (I have at least one more fix likely as well) next week.

Ok, no problem. Just wanted to check.

                Linus

Re: [PATCH] cifs - check S_AUTOMOUNT in revalidate

[Suresh Jayaraman]

On 04/28/2012 09:03 AM, Steve French wrote:
> The fix makes sense, but it is fairly recent and I haven't had a
> chance to try it,
> so unless a new release is imminent, I would prefer to put in the next merge
> request (I have at least one more fix likely as well) next week.

We need this fix for -stable as well, right? Please include

   Cc: stable@vger.kernel.org


Suresh

Re: [PATCH] cifs - check S_AUTOMOUNT in revalidate

[Jeff Layton]

On Wed, 02 May 2012 17:15:29 +0530
Suresh Jayaraman <sjayaraman@suse.com> wrote:

> On 04/28/2012 09:03 AM, Steve French wrote:
> > The fix makes sense, but it is fairly recent and I haven't had a
> > chance to try it,
> > so unless a new release is imminent, I would prefer to put in the next merge
> > request (I have at least one more fix likely as well) next week.
> 
> We need this fix for -stable as well, right? Please include
> 
>    Cc: stable@vger.kernel.org
> 
> 
> Suresh
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

The stable-kernel-rules.txt file says this:

 - It must fix a real bug that bothers people (not a, "This could be a
   problem..." type thing).

...and...

 - No "theoretical race condition" issues, unless an explanation of how the
   race can be exploited is also provided.

...and this seems to clearly violate both of those rules. I'd say we
just go with putting it in 3.4 for now, and keep it in mind if someone
comes back later and says that it's needed.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH] cifs - check S_AUTOMOUNT in revalidate

[Ian Kent]

On Wed, 2012-05-02 at 13:50 -0400, Jeff Layton wrote:
> On Wed, 02 May 2012 17:15:29 +0530
> Suresh Jayaraman <sjayaraman@suse.com> wrote:
> 
> > On 04/28/2012 09:03 AM, Steve French wrote:
> > > The fix makes sense, but it is fairly recent and I haven't had a
> > > chance to try it,
> > > so unless a new release is imminent, I would prefer to put in the next merge
> > > request (I have at least one more fix likely as well) next week.
> > 
> > We need this fix for -stable as well, right? Please include
> > 
> >    Cc: stable@vger.kernel.org
> > 
> > 
> > Suresh
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> The stable-kernel-rules.txt file says this:
> 
>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).
> 
> ...and...
> 
>  - No "theoretical race condition" issues, unless an explanation of how the
>    race can be exploited is also provided.
> 
> ...and this seems to clearly violate both of those rules. I'd say we
> just go with putting it in 3.4 for now, and keep it in mind if someone
> comes back later and says that it's needed.
> 

I have to agree with Jeff.

I just happened to notice it when trying to work out why DFS automounts
weren't happening.

I eventually found that the kernel in use didn't have the d_invalidate()
patch, then realized the potential problem. It would require fairly
heavy demand to trigger so it isn't urgent, especially since no-one has
actually reported it.

Another reason I should have sent this to Steve, not Linus, sorry for
the noise Linus.

Ian

Re: [PATCH] cifs - check S_AUTOMOUNT in revalidate

[Suresh Jayaraman]

On 05/02/2012 11:20 PM, Jeff Layton wrote:
> On Wed, 02 May 2012 17:15:29 +0530
> Suresh Jayaraman <sjayaraman@suse.com> wrote:
> 
>> On 04/28/2012 09:03 AM, Steve French wrote:
>>> The fix makes sense, but it is fairly recent and I haven't had a
>>> chance to try it,
>>> so unless a new release is imminent, I would prefer to put in the next merge
>>> request (I have at least one more fix likely as well) next week.
>>
>> We need this fix for -stable as well, right? Please include
>>
>>    Cc: stable@vger.kernel.org
> 
> The stable-kernel-rules.txt file says this:
> 
>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).
> 

Doh, looks like I had assumed (wrongly) that this fixes the problem
reported here

   http://marc.info/?l=linux-cifs&m=133460168320472&w=2

(mapping to a dfsroot and cd to a reflink returning "Object is remote"
or "Not a directory" error). I didn't have a setup to verify it too.

I notice that Steve has already merged this patch adding Cc to stable
and the Cc has to be removed before sending the pull request.


Suresh