[PATCH 1/2] nfsd: return NFS4ERR_SERVERFAULT from PUTROOTFH when path in /etc/exports not found

[Benny Halevy]

On Jan. 21, 2008, 19:29 +0200, "J. Bruce Fields" <bfields at fieldses.org> wrote:
> On Mon, Jan 21, 2008 at 12:09:14PM +0200, Benny Halevy wrote:
>> On Jan. 20, 2008, 22:30 +0200, "J. Bruce Fields" <bfields at fieldses.org> wrote:
>>> On Sun, Jan 20, 2008 at 06:23:52PM +0200, Benny Halevy wrote:
>>>> Currently, when /etc/exports refers to a non-existing path, e.g.:
>>>> /foo     *(fsid=0,rw,sync,no_subtree_check,no_root_squash,insecure)
>>>>
>>>> nfsd4_putrootfh returns NFS4ERR_PERM which is not a valid nfsv4 error return
>>>> for PUTROOTFH (nor in nfsv4.1).
>>>>
>>>> NFSv4 allows the following errors for PUTROOTFH:
>>>>       NFS4ERR_RESOURCE
>>>>       NFS4ERR_SERVERFAULT
>>>>       NFS4ERR_WRONGSEC
>>>> amongst which, NFS4ERR_SERVERFAULT seems to make the most sense in this case.
>>> If the most helpful error to return on the client side is actually
>>> ENOENT, then returning NFS4ERR_SERVERFAULT and depending on the client
>>> to map that to ENOENT seems a roundabout way to do it.  Just returning
>>> NFS4ERR_ENOENT would seem more in the spirit of interoperability even if
>>> it violates the letter of the spec.
>> The problem I see with that is that indeed the server went mad :-)
>> Since there's no path in PUTROOTFH the server is supposed to always
>> be able to return the filehandle for the pseudo-root fs.
>> the translation later on to ENOENT is just to make the application
>> (mount in this case) feel warm and fuzzy.
>>
>>> But perhaps there's also more we should do in mountd or exportfs to
>>> avoid this situation in the first place.
>> Good point, maybe deny any access, even before PUTROOTFH, if the 
>> exported root is inaccessible.
>>
>> At any rate, returning nfserr_perm as we do today is both violating
>> the spec and not really identifying the root problem.
> 
> You may be right that NFS4ERR_SERVERFAULT is more correct but for now
> I'm most inclined to just delete the extra handling of this error and
> make a note of the problem.  How about the following?


Fine with me.

Benny

> 
> --b.
> 
>>From 5b1888332c2dff147e6ca196ba4785767ac10fce Mon Sep 17 00:00:00 2001
> From: J. Bruce Fields <bfields at citi.umich.edu>
> Date: Mon, 21 Jan 2008 12:20:45 -0500
> Subject: [PATCH] knfsd: don't bother mapping putrootfh enoent to eperm
> 
> Neither EPERM and ENOENT map to valid errors for PUTROOTFH according to
> rfc 3530, and, if anything, ENOENT is likely to be slightly more
> informative; so don't bother mapping ENOENT to EPERM.  (Probably this
> was originally done because one likely cause was that there is an fsid=0
> export but that it isn't permitted to this particular client.  Now that
> we allow WRONGSEC returns, this is somewhat less likely.)
> 
> In the long term we should work to make this situation less likely,
> perhaps by turning off nfsv4 service entirely in the absence of the
> pseudofs root, or constructing a pseudofilesystem root ourselves in the
> kernel as necessary.
> 
> Thanks to Benny Halevy <bhalevy at panasas.com> for pointing out this
> problem.
> 
> Signed-off-by: J. Bruce Fields <bfields at citi.umich.edu>
> Cc: Benny Halevy <bhalevy at panasas.com>
> ---
>  fs/nfsd/export.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/export.c b/fs/nfsd/export.c
> index cbbc594..79b4bf8 100644
> --- a/fs/nfsd/export.c
> +++ b/fs/nfsd/export.c
> @@ -1357,8 +1357,6 @@ exp_pseudoroot(struct svc_rqst *rqstp, struct svc_fh *fhp)
>  	mk_fsid(FSID_NUM, fsidv, 0, 0, 0, NULL);
>  
>  	exp = rqst_exp_find(rqstp, FSID_NUM, fsidv);
> -	if (PTR_ERR(exp) == -ENOENT)
> -		return nfserr_perm;
>  	if (IS_ERR(exp))
>  		return nfserrno(PTR_ERR(exp));
>  	rv = fh_compose(fhp, exp, exp->ex_dentry, NULL);