Re: [PATCH] exportfs: Make sure pass all valid export flags to nfsd

["J. Bruce Fields" <bfields@fieldses.org>]

On Sun, Jan 15, 2017 at 03:43:21PM +0800, Kinglong Mee wrote:
> On 1/11/2017 22:16, J. Bruce Fields wrote:
> > On Sat, Jan 07, 2017 at 07:57:28PM +0800, Kinglong Mee wrote:
> >> On 1/7/2017 05:05, J. Bruce Fields wrote:
> >>> On Sat, Dec 31, 2016 at 09:05:11PM +0800, Kinglong Mee wrote:
> >>>> test_export pass a export flags only marks NFSEXP_FSID,
> >>>> nfsd may want other flags for export checking.
> >>>
> >>> Why?  What problem does this fix?
> >>
> >> When testing the patch "NFSD: Only support readonly export for
> >> !fsync and readonly filesystem", I found exportfs don't pass 
> >> all valid export flags to nfsd. So, make this patch.
> > 
> > This function is meant to test whether a filesystem supports nfs export
> > or not.  It doesn't need the full set of export flags.  Off the top of
> > my head I can't see reason this would cause problems, but I'm not
> > convinced it's safe, either.  (New nfs-utils against old kernels might
> > be a case to check, e.g. to see how unsupported flags are handled.)
> 
> There are two cases that passing the flags to nfsd, 
> 1, exportfs checks the export entry, only NFSEXP_FSID without this patch,
> 2, mountd pass the validate export entry, all validate export flags.
> 
> When the new nfs-utils against old kernels,
> user specifies an unsupported flags(Not NFSEXP_FSID),
> exportfs doesn't warning that unsupported flags (without test),
> mountd doesn't get the export entry for the flags.
> 
> If exportfs warning the unsupported flags, user can modify it and
> do the right export.
> Do I have an exact understanding?

OK, so when somebody specifies unsupported flags, the failures is silent
and client mounts just fail.  It would be better to warn when exportfs
runs.

I agree that that would be helpful.  Have you tested those cases?  (new
nfs-utils, old kernel).

It would be worth looking at validate_export() to see how it could
improve error messages in this case.  Currently it will say only:

	/example/export does not support NFS export

or

	/example/export requires fsid= for export

With your patch (forgive me if I misremember), I believe those are still
the only error messages, so it will be confusing if, for example, you
get

	/example/export does not support NFS export

when the real problem is that an export flag is unsupported.  (But maybe
you had kernel messages to help there, I don't remember.)

--b.