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

[Myklebust, Trond]

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? If not, then let's just move
the AF_LOCAL connection back into the process context and out of rpciod.

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Ç+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—