From: "J. Bruce Fields" Subject: Re: [PATCH] exportfs: fix incorrect EACCES in reconnect_path() Date: Fri, 2 May 2008 11:56:17 -0400 Message-ID: <20080502155617.GD18401@fieldses.org> References: <20080404102449.GA10209@janus> <20080407184346.GF3305@fieldses.org> <20080409133639.GA9588@lst.de> <18454.45086.254692.412079@notabene.brown> <20080429163554.GE20420@fieldses.org> <20080429174004.GA28719@janus> <20080430174736.GB20377@fieldses.org> <20080502151646.GA5515@janus> <20080502153439.GC7376@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Frank van Maarseveen , Neil Brown , Christoph Hellwig , Linux NFS mailing list To: Christoph Hellwig Return-path: Received: from mail.fieldses.org ([66.93.2.214]:42999 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965450AbYEBP4Y (ORCPT ); Fri, 2 May 2008 11:56:24 -0400 In-Reply-To: <20080502153439.GC7376@infradead.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Fri, May 02, 2008 at 11:34:39AM -0400, Christoph Hellwig wrote: > On Fri, May 02, 2008 at 05:16:46PM +0200, Frank van Maarseveen wrote: > > A privileged process on an NFS client which drops privileges after using > > them to change the current working directory, will experience incorrect > > EACCES after an NFS server reboot. This problem can also occur after > > memory pressure on the server, particularly when the client side is > > quiet for some time. > > > > This patch removes the x-bit check during dentry tree reconstruction at > > the server by exportfs on behalf of nfsd. > > I'm still against adding this crap, The only statements I've seen against the change so far have been of the form "you should not do that", without explaining why not. It's entirely possible that you're right, but I need some argument. > and even when I get overruled that > doesn't make the comments on lookup_one_noperm any less true, We do need to at least update it to reflect the addition of a new caller. > not does it give you a permit to break the kerneldoc generation. Oops; here's a version that should make kerneldoc happy. It also adds a little more explanation, and leaves alone the editorializing on sysfs (on which I have no opinion). --b. commit ccdfe77dc49a07c298bb9e2107290267492f16b3 Author: Frank van Maarseveen Date: Fri May 2 17:16:46 2008 +0200 exportfs: fix incorrect EACCES in reconnect_path() A privileged process on an NFS client which drops privileges after using them to change the current working directory, will experience incorrect EACCES after an NFS server reboot. This problem can also occur after memory pressure on the server, particularly when the client side is quiet for some time. This patch removes the x-bit check during dentry tree reconstruction at the server by exportfs on behalf of nfsd. Signed-off-by: Frank van Maarseveen Signed-off-by: J. Bruce Fields diff --git a/fs/exportfs/expfs.c b/fs/exportfs/expfs.c index 109ab5e..89dc7ae 100644 --- a/fs/exportfs/expfs.c +++ b/fs/exportfs/expfs.c @@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, struct dentry *target_dir) } dprintk("%s: found name: %s\n", __FUNCTION__, nbuf); mutex_lock(&ppd->d_inode->i_mutex); - npd = lookup_one_len(nbuf, ppd, strlen(nbuf)); + npd = lookup_one_noperm(nbuf, ppd); mutex_unlock(&ppd->d_inode->i_mutex); if (IS_ERR(npd)) { err = PTR_ERR(npd); @@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid, err = exportfs_get_name(mnt, target_dir, nbuf, result); if (!err) { mutex_lock(&target_dir->d_inode->i_mutex); - nresult = lookup_one_len(nbuf, target_dir, - strlen(nbuf)); + nresult = lookup_one_noperm(nbuf, target_dir); mutex_unlock(&target_dir->d_inode->i_mutex); if (!IS_ERR(nresult)) { if (nresult->d_inode) { diff --git a/fs/namei.c b/fs/namei.c index e179f71..c00150c 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -1391,7 +1391,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len) } /** - * lookup_one_noperm - bad hack for sysfs + * lookup_one_noperm - bad hack for sysfs and nfsd * @name: pathname component to lookup * @base: base directory to lookup from * @@ -1399,7 +1399,12 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len) * checks. It's a horrible hack to work around the braindead sysfs * architecture and should not be used anywhere else. * - * DON'T USE THIS FUNCTION EVER, thanks. + * It is also used by nfsd via exports to reconstruct the dentry tree + * for directory handles (e.g. when a client requests a directory by + * filehandle after a server reboot has cleared the dentry cache of that + * directory's parents). + * + * DON'T USE THIS FUNCTION ANYWHERE ELSE, thanks. */ struct dentry *lookup_one_noperm(const char *name, struct dentry *base) {