[BUG] Regression in mfsymlink handling introduced by "cifs: only set ops for inodes in I_NEW state"

[BUG] Regression in mfsymlink handling introduced by "cifs: only set ops for inodes in I_NEW state"

[David McBride]

The following commit included in 3.10, and copied to some table-series
kernels:

 commit: c2b93e0699723700f886ce17bb65ffd771195a6d
 cifs: only set ops for inodes in I_NEW state

... causes a regression in mfsymlink handling under some circumstances.

Specifically, this patch modified cifs_fattr_to_inode() to only invoke
cifs_set_ops() if that inode still had the I_NEW flag set --- preventing
an inode's ops-set from from being changed after it had been
initialized, preventing an oops if another thread of execution was
already chasing that pointer.

However, mfsymlinks are also affected by this commit.  In the cold-cache
case, a user invoking stat() on an mfsymlink will still see the correct
results.  But, if the dentry cache is instead populated via
cifs_readdir/filldir, then inode property population operates in a two
stage mode:

 - First, the inode is initialized as a regular file, with the
   CIFS_FATTR_NEED_REVAL flag set.

 - Later, when the file is stat()'d, then correct operation depends on
   the (re)validation steps from being able to update the inode's
   properties --- including the set of file operations permitted.

(A comment explains:  "trying to get the type and mode can be slow, so
 just call those regular files for now, and mark for reval")

With the above commit, this second revalidation step never updates the
operations field, resulting in symlinks not having the readlink(), etc.
functions correctly hooked up.

A naïve fix-up is to modify the conditional to also permit (re)setting
ops on S_IFLNK-mode files; however, I suspect a correct fix lies elsewhere?

Cheers,
David
-- 
David McBride <dwm37-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
Unix Specialist, University Computing Service

Re: [BUG] Regression in mfsymlink handling introduced by "cifs: only set ops for inodes in I_NEW state"

[Stefan (metze) Metzmacher]

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

Hi David,

> The following commit included in 3.10, and copied to some table-series
> kernels:
> 
>  commit: c2b93e0699723700f886ce17bb65ffd771195a6d
>  cifs: only set ops for inodes in I_NEW state
> 
> ... causes a regression in mfsymlink handling under some circumstances.
> 
> Specifically, this patch modified cifs_fattr_to_inode() to only invoke
> cifs_set_ops() if that inode still had the I_NEW flag set --- preventing
> an inode's ops-set from from being changed after it had been
> initialized, preventing an oops if another thread of execution was
> already chasing that pointer.
> 
> However, mfsymlinks are also affected by this commit.  In the cold-cache
> case, a user invoking stat() on an mfsymlink will still see the correct
> results.  But, if the dentry cache is instead populated via
> cifs_readdir/filldir, then inode property population operates in a two
> stage mode:
> 
>  - First, the inode is initialized as a regular file, with the
>    CIFS_FATTR_NEED_REVAL flag set.
> 
>  - Later, when the file is stat()'d, then correct operation depends on
>    the (re)validation steps from being able to update the inode's
>    properties --- including the set of file operations permitted.
> 
> (A comment explains:  "trying to get the type and mode can be slow, so
>  just call those regular files for now, and mark for reval")
> 
> With the above commit, this second revalidation step never updates the
> operations field, resulting in symlinks not having the readlink(), etc.
> functions correctly hooked up.

> A naïve fix-up is to modify the conditional to also permit (re)setting
> ops on S_IFLNK-mode files; however, I suspect a correct fix lies elsewhere?

I guess 'if (inode->i_state & I_NEW)' should be changed to
'if ((inode->i_state & I_NEW) || (fattr->cf_flags & CIFS_FATTR_NEED_REVAL))'

Would that solve the problem?

Jeff/Steve, any comment on this?

metze


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 261 bytes --]

Re: [BUG] Regression in mfsymlink handling introduced by "cifs: only set ops for inodes in I_NEW state"

[Jeff Layton]

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

On Tue, 06 Aug 2013 16:30:52 +0200
"Stefan (metze) Metzmacher" <metze-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:

> Hi David,
> 
> > The following commit included in 3.10, and copied to some table-series
> > kernels:
> > 
> >  commit: c2b93e0699723700f886ce17bb65ffd771195a6d
> >  cifs: only set ops for inodes in I_NEW state
> > 
> > ... causes a regression in mfsymlink handling under some circumstances.
> > 
> > Specifically, this patch modified cifs_fattr_to_inode() to only invoke
> > cifs_set_ops() if that inode still had the I_NEW flag set --- preventing
> > an inode's ops-set from from being changed after it had been
> > initialized, preventing an oops if another thread of execution was
> > already chasing that pointer.
> > 
> > However, mfsymlinks are also affected by this commit.  In the cold-cache
> > case, a user invoking stat() on an mfsymlink will still see the correct
> > results.  But, if the dentry cache is instead populated via
> > cifs_readdir/filldir, then inode property population operates in a two
> > stage mode:
> > 
> >  - First, the inode is initialized as a regular file, with the
> >    CIFS_FATTR_NEED_REVAL flag set.
> > 
> >  - Later, when the file is stat()'d, then correct operation depends on
> >    the (re)validation steps from being able to update the inode's
> >    properties --- including the set of file operations permitted.
> > 
> > (A comment explains:  "trying to get the type and mode can be slow, so
> >  just call those regular files for now, and mark for reval")
> > 
> > With the above commit, this second revalidation step never updates the
> > operations field, resulting in symlinks not having the readlink(), etc.
> > functions correctly hooked up.
> 
> > A naïve fix-up is to modify the conditional to also permit (re)setting
> > ops on S_IFLNK-mode files; however, I suspect a correct fix lies elsewhere?
> 
> I guess 'if (inode->i_state & I_NEW)' should be changed to
> 'if ((inode->i_state & I_NEW) || (fattr->cf_flags & CIFS_FATTR_NEED_REVAL))'
> 
> Would that solve the problem?
> 
> Jeff/Steve, any comment on this?
> 
> metze
> 

I don't think so...

Once an inode's operations are set, you really don't want to go
changing them. That opens the door for a malicious server to change the
file type out from under you and cause oopses. The bug report that
Sachin worked on originally had such an oops. In that case it was
caused by something else, but a malicious server could wreak similar
havoc...

The right solution is probably to ensure that the dentry fails
d_revalidate if there's a chance it could turn out to be a mfsymlink.
I think that the easiest way to ensure that would probably be to set the
dentry->d_time to 0 in the readdir code.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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

Re: [BUG] Regression in mfsymlink handling introduced by "cifs: only set ops for inodes in I_NEW state"

[Jeff Layton]

On Tue, 6 Aug 2013 10:56:23 -0400
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Tue, 06 Aug 2013 16:30:52 +0200
> "Stefan (metze) Metzmacher" <metze-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> 
> > Hi David,
> > 
> > > The following commit included in 3.10, and copied to some table-series
> > > kernels:
> > > 
> > >  commit: c2b93e0699723700f886ce17bb65ffd771195a6d
> > >  cifs: only set ops for inodes in I_NEW state
> > > 
> > > ... causes a regression in mfsymlink handling under some circumstances.
> > > 
> > > Specifically, this patch modified cifs_fattr_to_inode() to only invoke
> > > cifs_set_ops() if that inode still had the I_NEW flag set --- preventing
> > > an inode's ops-set from from being changed after it had been
> > > initialized, preventing an oops if another thread of execution was
> > > already chasing that pointer.
> > > 
> > > However, mfsymlinks are also affected by this commit.  In the cold-cache
> > > case, a user invoking stat() on an mfsymlink will still see the correct
> > > results.  But, if the dentry cache is instead populated via
> > > cifs_readdir/filldir, then inode property population operates in a two
> > > stage mode:
> > > 
> > >  - First, the inode is initialized as a regular file, with the
> > >    CIFS_FATTR_NEED_REVAL flag set.
> > > 
> > >  - Later, when the file is stat()'d, then correct operation depends on
> > >    the (re)validation steps from being able to update the inode's
> > >    properties --- including the set of file operations permitted.
> > > 
> > > (A comment explains:  "trying to get the type and mode can be slow, so
> > >  just call those regular files for now, and mark for reval")
> > > 
> > > With the above commit, this second revalidation step never updates the
> > > operations field, resulting in symlinks not having the readlink(), etc.
> > > functions correctly hooked up.
> > 
> > > A naïve fix-up is to modify the conditional to also permit (re)setting
> > > ops on S_IFLNK-mode files; however, I suspect a correct fix lies elsewhere?
> > 
> > I guess 'if (inode->i_state & I_NEW)' should be changed to
> > 'if ((inode->i_state & I_NEW) || (fattr->cf_flags & CIFS_FATTR_NEED_REVAL))'
> > 
> > Would that solve the problem?
> > 
> > Jeff/Steve, any comment on this?
> > 
> > metze
> > 
> 
> I don't think so...
> 
> Once an inode's operations are set, you really don't want to go
> changing them. That opens the door for a malicious server to change the
> file type out from under you and cause oopses. The bug report that
> Sachin worked on originally had such an oops. In that case it was
> caused by something else, but a malicious server could wreak similar
> havoc...
> 
> The right solution is probably to ensure that the dentry fails
> d_revalidate if there's a chance it could turn out to be a mfsymlink.
> I think that the easiest way to ensure that would probably be to set the
> dentry->d_time to 0 in the readdir code.
> 

Specifically, I think the logic in cifs_d_revalidate is just plain
wrong. Currently, the dentry->d_time check and the check for
lookupCacheEnabled is only done for negative dentries. Shouldn't that
be affecting positive dentries as well?

If so, then you could simply set the dentries that are possible
mfsymlinks with a d_time of 0. They'd fail revalidation at that
step and get recreated when you stat them.

Sachin, Steve, thoughts?
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH] cifs: don't instantiate new dentries in readdir for inodes that need to be revalidated immediately

[Jeff Layton]

David reported that commit c2b93e06 (cifs: only set ops for inodes in
I_NEW state) caused a regression with mfsymlinks. If a mfsymlink dentry
was instantiated at readdir time, then it would not be properly tossed
out of the cache and reinstantiated when a stat or readlink call came
about.

This patch addresses this by simply skipping instantiating dentries
when we *know* that they will need to be immediately revalidated. The
next attempt to use that dentry will cause a new lookup to occur
(which is basically what we want to happen anyway).

Reported-by: David McBride <dwm37-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/readdir.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
index ab87784..69d2c82 100644
--- a/fs/cifs/readdir.c
+++ b/fs/cifs/readdir.c
@@ -111,6 +111,14 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
 			return;
 	}
 
+	/*
+	 * If we know that the inode will need to be revalidated immediately,
+	 * then don't create a new dentry for it. We'll end up doing an on
+	 * the wire call either way and this spares us an invalidation.
+	 */
+	if (fattr->cf_flags & CIFS_FATTR_NEED_REVAL)
+		return;
+
 	dentry = d_alloc(parent, name);
 	if (!dentry)
 		return;
-- 
1.8.3.1

Re: [PATCH] cifs: don't instantiate new dentries in readdir for inodes that need to be revalidated immediately

[Jeff Layton]

On Wed,  7 Aug 2013 09:04:07 -0400
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> David reported that commit c2b93e06 (cifs: only set ops for inodes in
> I_NEW state) caused a regression with mfsymlinks. If a mfsymlink dentry
> was instantiated at readdir time, then it would not be properly tossed
> out of the cache and reinstantiated when a stat or readlink call came
> about.
> 
> This patch addresses this by simply skipping instantiating dentries
> when we *know* that they will need to be immediately revalidated. The
> next attempt to use that dentry will cause a new lookup to occur
> (which is basically what we want to happen anyway).
> 
> Reported-by: David McBride <dwm37-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/readdir.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/fs/cifs/readdir.c b/fs/cifs/readdir.c
> index ab87784..69d2c82 100644
> --- a/fs/cifs/readdir.c
> +++ b/fs/cifs/readdir.c
> @@ -111,6 +111,14 @@ cifs_prime_dcache(struct dentry *parent, struct qstr *name,
>  			return;
>  	}
>  
> +	/*
> +	 * If we know that the inode will need to be revalidated immediately,
> +	 * then don't create a new dentry for it. We'll end up doing an on
> +	 * the wire call either way and this spares us an invalidation.
> +	 */
> +	if (fattr->cf_flags & CIFS_FATTR_NEED_REVAL)
> +		return;
> +
>  	dentry = d_alloc(parent, name);
>  	if (!dentry)
>  		return;


This patch is untested (other than for compilation), but I think it'll
do the right thing. Metze, David could you test it and let us know if
it helps?

Thanks,
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: don't instantiate new dentries in readdir for inodes that need to be revalidated immediately

[David McBride]

On 07/08/13 14:06, Jeff Layton wrote:

> This patch is untested (other than for compilation), but I think it'll
> do the right thing. Metze, David could you test it and let us know if
> it helps?

Hi Jeff,

Seems to work nicely; many thanks!

(Caveat: I'm testing this on a backported version of the 3.10 driver on
top of Ubuntu's 3.8 kernel.)

Tested-By: David McBride <dwm37-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>

Cheers,
David
-- 
David McBride <dwm37-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
Unix Specialist, University Computing Service

Re: [PATCH] cifs: don't instantiate new dentries in readdir for inodes that need to be revalidated immediately

[Marcus Moeller]

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

Am 07.08.2013 15:42, schrieb David McBride:
> On 07/08/13 14:06, Jeff Layton wrote:
>
>> This patch is untested (other than for compilation), but I think it'll
>> do the right thing. Metze, David could you test it and let us know if
>> it helps?

Just to verify: could it be that mfsymlink is already broken in 
3.9.6-301.fc19? At least links created with fedora 18 are no longer 
readable there. Newly created links seems to work fine (at least till now).

Greets
Marcus




[-- Attachment #2: S/MIME Kryptografische Unterschrift --]
[-- Type: application/pkcs7-signature, Size: 2460 bytes --]

Re: [PATCH] cifs: don't instantiate new dentries in readdir for inodes that need to be revalidated immediately

[Jeff Layton]

On Thu, 08 Aug 2013 16:05:44 +0200
Marcus Moeller <marcus.moeller-OI3hZJvNYWs@public.gmane.org> wrote:

> Am 07.08.2013 15:42, schrieb David McBride:
> > On 07/08/13 14:06, Jeff Layton wrote:
> >
> >> This patch is untested (other than for compilation), but I think it'll
> >> do the right thing. Metze, David could you test it and let us know if
> >> it helps?
> 
> Just to verify: could it be that mfsymlink is already broken in 
> 3.9.6-301.fc19? At least links created with fedora 18 are no longer 
> readable there. Newly created links seems to work fine (at least till now).
> 
> Greets
> Marcus
> 

Yes, 3.9.6 has the patch that broke it.

What matters is how the (in memory) inode is created after mounting. If
you first run across an mfsymlink with a readdir() call, then you'll
get an inode that's "stuck" as a regular file. If you instead do
something like stat() it before ever running a readdir over the
containing directory, it should work just fine.

This patch probably is probably reasonable for stable, given that the
original one also went there.

Steve, want to mark as such?

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: don't instantiate new dentries in readdir for inodes that need to be revalidated immediately

[Steve French]

On Thu, Aug 8, 2013 at 9:22 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Thu, 08 Aug 2013 16:05:44 +0200
> Marcus Moeller <marcus.moeller-OI3hZJvNYWs@public.gmane.org> wrote:
>
>> Am 07.08.2013 15:42, schrieb David McBride:
>> > On 07/08/13 14:06, Jeff Layton wrote:
>> >
>> >> This patch is untested (other than for compilation), but I think it'll
>> >> do the right thing. Metze, David could you test it and let us know if
>> >> it helps?
>>
>> Just to verify: could it be that mfsymlink is already broken in
>> 3.9.6-301.fc19? At least links created with fedora 18 are no longer
>> readable there. Newly created links seems to work fine (at least till now).
>>
>> Greets
>> Marcus
>>
>
> Yes, 3.9.6 has the patch that broke it.
>
> What matters is how the (in memory) inode is created after mounting. If
> you first run across an mfsymlink with a readdir() call, then you'll
> get an inode that's "stuck" as a regular file. If you instead do
> something like stat() it before ever running a readdir over the
> containing directory, it should work just fine.
>
> This patch probably is probably reasonable for stable, given that the
> original one also went there.
>
> Steve, want to mark as such?

It is already marked for stable


-- 
Thanks,

Steve

Re: [PATCH] cifs: don't instantiate new dentries in readdir for inodes that need to be revalidated immediately

[Marcus Moeller]

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

Am 08.08.2013 17:33, schrieb Steve French:
> On Thu, Aug 8, 2013 at 9:22 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On Thu, 08 Aug 2013 16:05:44 +0200
>> Marcus Moeller <marcus.moeller-OI3hZJvNYWs@public.gmane.org> wrote:
>>
>>> Am 07.08.2013 15:42, schrieb David McBride:
>>>> On 07/08/13 14:06, Jeff Layton wrote:
>>>>
>>>>> This patch is untested (other than for compilation), but I think it'll
>>>>> do the right thing. Metze, David could you test it and let us know if
>>>>> it helps?
>>>
>>> Just to verify: could it be that mfsymlink is already broken in
>>> 3.9.6-301.fc19? At least links created with fedora 18 are no longer
>>> readable there. Newly created links seems to work fine (at least till now).
>>>
>>> Greets
>>> Marcus
>>>
>>
>> Yes, 3.9.6 has the patch that broke it.
>>
>> What matters is how the (in memory) inode is created after mounting. If
>> you first run across an mfsymlink with a readdir() call, then you'll
>> get an inode that's "stuck" as a regular file. If you instead do
>> something like stat() it before ever running a readdir over the
>> containing directory, it should work just fine.
>>
>> This patch probably is probably reasonable for stable, given that the
>> original one also went there.
>>
>> Steve, want to mark as such?

@Jeff, could you please do me a favor and let me know, when this is 
stable/made it to Fedora, as we are in need of that patch.

Thanks
Marcus



[-- Attachment #2: S/MIME Kryptografische Unterschrift --]
[-- Type: application/pkcs7-signature, Size: 2460 bytes --]