linux-kernel - Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path

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: <1238539789.28445.105.camel@heimdal.trondhjem.org>
Date:	Tue, 31 Mar 2009 18:49:49 -0400
From:	Trond Myklebust <Trond.Myklebust@...app.com>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org, viro@...iv.linux.org.uk,
	hch@...radead.org
Subject: Re: [PATCH 1/4] VFS: Add a VFS helper function
 vfs_remote_path_lookup()

On Tue, 2009-03-31 at 18:38 -0400, Trond Myklebust wrote:
> On Tue, 2009-03-31 at 14:40 -0700, Linus Torvalds wrote:
> > 
> > On Wed, 11 Mar 2009, Trond Myklebust wrote:
> > >
> > > This patch therefore defines a VFS helper function that sets up a temporary
> > > mount namespace to represent the server namespace, and has the current
> > > task pivot into that prior to doing the path lookup. Upon completion, it
> > > pivots back into the original namespace, and destroys the private one.
> > 
> > This is disgusting.
> > 
> > Why don't you just create the namespace once?
> > 
> > Also, why do you need that disgusting pivot thing, when we could instead 
> > trivially just add a 
> > 
> > 	struct filesystem *fs;
> > 
> > into the nameidata, and then we can initialize it to
> > 
> > 	nd->fs = current->fs
> > 
> > and make all the path walkers use that.
> > 
> > Or we could even try to clean up that horrid AT_FDCWD mess, and drop the 
> > whole "dfd" passing to "do_path_lookup()", and instead do
> > 
> > 	rwlock_t *lock;
> > 	struct path *root, *pwd;
> > 
> > and do
> > 
> > 	nd->lock = &current->fs->lock;
> > 	nd->root = &current->fs->root;
> > 	nd->pwd =  &current->fs->pwd;
> > 
> > to initialize things. Then drop dfd as an argument entirely from 
> > path_lookup_open() and do_path_lookup(), and just have the caller 
> > initialize the nameidata (the only caller that doesn't use fs->pwd 
> > currently is do_filp_open(), which takes that 'dfd' and could just 
> > initialize nd->pwd to the right thing.
> 
> I'd be fine with that.
> 
> > I dunno. This is very Al Viroish country, but I really hate how your patch 
> > looks. 99% of it is just working around the fact that you want some very 
> > _slight_ differences to how that special '/' thing is handled.
> 
> No. The main purpose is to able to look up and walk down an NFSv4 mount
> path, which is a path on the _server_'s file/directory namespace.
> In order to be able to follow any valid mount path that the user
> supplies, we need the ability to follow symlinks, cross mount points and
> even cross NFSv4 referrals (i.e. afs-like junctions that point to paths
> on a different server).
> 
> Under normal path walking we use the 'follow_link()' method of
> autogenerating mountpoints (see
> Documentation/filesystems/automount-support.txt) when we come across
> mount points or referrals on the server. If you are walking a path that
> is not supposed to be visible to user processes, you therefore need a
> private namespace.
> 
> > It's not worth doing these kinds of hacks for that.
> > 
> > And I think it's positively _wrong_ to have a function that creates and 
> > destroys the whole "struct fs_struct" and a namespace for just one call. 
> > Even if you don't think it's at all performance-critical, the interface is 
> > too damn ugly. Have separate "create/destroy context" functions, so that 
> > you _can_ do it just once, and have multiple calls in between.
> 
> That can probably be done, but the main reason for having the namespace
> was to be able, once the sys_mount() is complete, to garbage collect and
> get rid of those autogenerated mount points that are not user visible.

I meant to say "...the main reason for destroying the namespace"...

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@...app.com
www.netapp.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/