linux-kernel - Re: [PATCH] CIFS: fix automount for DFS shares

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111023074653.352be663@tlielax.poochiereds.net>
Date:	Sun, 23 Oct 2011 07:46:53 -0400
From:	Jeff Layton <jlayton@...hat.com>
To:	Ian Kent <raven@...maw.net>
Cc:	Gerlando Falauto <gerlando.falauto@...mile.com>,
	linux-kernel@...r.kernel.org, David Howells <dhowells@...hat.com>
Subject: Re: [PATCH] CIFS: fix automount for DFS shares

On Sun, 23 Oct 2011 19:14:34 +0800
Ian Kent <raven@...maw.net> wrote:

> On Fri, 2011-10-07 at 13:09 +0200, Gerlando Falauto wrote:
> > Automounting directories are now invalidated by .d_revalidate()
> > so to be d_instantiate()d again with the right DCACHE_NEED_AUTOMOUNT
> > flag
> 
> But why doesn't CIFS know this is a DFS inode the first time around, it
> appears to do a truck load of work looking that stuff up?
> 

This area needs some work...

The readdir codepath in cifs uses the FIND_FIRST/NEXT calls on the
wire. Those return both filenames and attributes for the particular SMB
call infolevel. That in turn is used to instantiate dentries and inodes
for those entries.

Unfortunately though, we have not found a way to determine whether a
particular inode is a DFS referral from within this codepath. The only
way to know for sure (AFAIK) is to try an operation on a specific
pathname and then look for a NT_STATUS_PATH_NOT_COVERED return code.

> > 
> > Signed-off-by: Gerlando Falauto <gerlando.falauto@...mile.com>
> > ---
> >  fs/cifs/dir.c |    7 ++++++-
> >  1 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
> > index 9ea65cf..67f54d3 100644
> > --- a/fs/cifs/dir.c
> > +++ b/fs/cifs/dir.c
> > @@ -637,8 +637,13 @@ cifs_d_revalidate(struct dentry *direntry, struct nameidata *nd)
> >  	if (direntry->d_inode) {
> >  		if (cifs_revalidate_dentry(direntry))
> >  			return 0;
> > -		else
> > +		else {
> > +			/* We want automonting inodes to be
> > +			 * considered invalid or so */
> > +			if (IS_AUTOMOUNT(direntry->d_inode))
> > +				return 0;
> 
> I'd be inclined to set DCACHE_NEED_AUTOMOUNT here but are we certain
> that cifs_revalidate_dentry() will always return 0 for a DFS inode or at
> least ones that don't yet have DCACHE_NEED_AUTOMOUNT set and why?
> 

You mean you'd set that in the "return 1" case here? That doesn't sound
quite right if so. This inode could be entirely unrelated to DFS.

Currently, cifs_revalidate_dentry checks to see if the attributes on
the inode are "too old" (past the actimeo setting). If they are then it
will issue a QUERY_PATH_INFO call on the wire to try and update those
attributes. If it's a DFS inode then you'll get an error back and that
function should return 0.

It's possible however that the inode has already had its attributes
updated recently, and was found to be an automount point. That is, it
got S_AUTOMOUNT set but no referral chasing happened. At that point I'm
a little fuzzy as to what should happen...

The safest thing would seem to be to return 0 in that case, to force an
invalidation and lookup on this dentry again, but maybe there's a better
way to handle that?

> >  			return 1;
> > +		}
> >  	}
> >  
> >  	/*
> 
> 

-- 
Jeff Layton <jlayton@...hat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/