Re: [PATCH v3] SUNRPC: set desired file system root beforeconnecting local transports

[Myklebust, Trond]

On Wed, 2012-11-14 at 16:42 -0500, J. Bruce Fields wrote:
> On Wed, Nov 14, 2012 at 09:36:33PM +0000, Myklebust, Trond wrote:
> > On Wed, 2012-11-14 at 16:01 -0500, J. Bruce Fields wrote:
> > > On Mon, Nov 12, 2012 at 12:37:54PM +0400, Stanislav Kinsbursky wrote:
> > > > 07.11.2012 22:33, J. Bruce Fields ÐÐÑÐÑ:
> > > > >On Tue, Nov 06, 2012 at 08:36:05AM -0500, J. Bruce Fields wrote:
> > > > >>On Tue, Nov 06, 2012 at 08:10:18AM -0500, Christoph Hellwig wrote:
> > > > >>>On Tue, Nov 06, 2012 at 08:07:06AM -0500, J. Bruce Fields wrote:
> > > > >>>>So you're worried that a bug in the nfs code could modify the root and
> > > > >>>>then not restore it?
> > > > >>>
> > > > >>>At least the link you pointed to earlier never sets it back.
> > > > >>
> > > > >>This? http://thread.gmane.org/gmane.linux.kernel/1259986/focus=47687
> > > > >>
> > > > >>	+	get_fs_root(current->fs, &root);
> > > > >>	+	set_fs_root(current->fs, &transport->root);
> > > > >>	+
> > > > >>	 	status = xs_local_finish_connecting(xprt, sock);
> > > > >>	+
> > > > >>	+	set_fs_root(current->fs, &root);
> > > > >>	+	path_put(&root);
> > > > >>
> > > > >>>Instead
> > > > >>>of messing with it I'd rather have the sunrpc code use vfs_path_lookup
> > > > >>>and not care about current->fs->root at all.
> > > > >>
> > > > >>The annoyance is that the lookup happens somewhere lower down in the
> > > > >>networking code (net/unix/af_unix.c:unix_find_other, I think).  So we'd
> > > > >>need some new (internal) API.  We'd likely be the only user of that new
> > > > >>API.
> > > > >
> > > > >So, if the only drawback is really just the risk of introducing a bug
> > > > >that leaves the fs_root changed--the above seems simple enough for that
> > > > >not to be a great risk, right?
> > > > >
> > > > 
> > > > If we unshare rpciod fs struct (which is exported already), then we
> > > 
> > > I'm not sure what you mean by that.  Do workqueues actually have their
> > > own dedicated set of associated tasks?  I thought all workqueues shared
> > > a common pool of tasks these days.
> > > 
> > > > won't affect other kthreads by root swapping.
> > > > But would be great to hear Trond's opinion about this approach.
> > > > 
> > > > Trond, could you tell us your feeling about all this?
> > > 
> > > I think it's often easier to get people to comment on an actual patch,
> > > and this one would be quite short, so try that....
> > 
> > unshare() would break expectations for other users of workqueue threads
> > unless you "reshare()" afterwards. Either way that's going to be
> > seriously ugly.
> > 
> > OK, let's look at this again. Do we ever use AF_LOCAL connections for
> > anything other than synchronous rpc calls to the local rpcbind daemon in
> > order to register/unregister new services?
> 
> Simo's patches use them for upcalls to svcgssd.  Those will always be
> done from server threads.

Any reason why you can't set that up when you start nfsd?

> > If not, then let's just move
> > the AF_LOCAL connection back into the process context and out of rpciod.
> 
> Remind me how this helps?

rpciod shares the 'init' process net namespace and chroot properties.
If, however you call bind() from the (containerised) process that was
used to start nfsd, then you will be using filesystem root (and net
namespace) of that container.

> --b.
> 
> > 
> > That implies that the process needs to be privileged, but it needs
> > privileges in order to start RPC daemons anyway.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
N‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i