Re: [PATCH v2 14/14] nfsd: use do_lookup_open() for non-creating open requests too.

[Chuck Lever <cel@kernel.org>]

Hi Neil,

Thanks for jumping on the Sashiko findings. Codex review identified
three correctness concerns which I've confirmed with Claude are not
false positives:

1. do_lookup_open() takes write access unconditionally.

	error = mnt_want_write(parent->mnt);
	if (error)
		return ERR_PTR(error);

   Fine for a create. But a read-only OPEN with no create now returns
   -EROFS on anything mounted read-only at the VFS layer: squashfs,
   iso9660, a read-only bind mount, a snapshot. The old path didn't ask
   for write until nfsd_open() actually opened the file, and for
   O_RDONLY it never asked at all, so these opens worked.

   Worth stressing that this isn't the "ro" export option. That keeps
   the underlying mount writable and is enforced in nfsd, so it's fine.
   Only genuinely read-only mounts break, which is probably why it
   slipped past testing. Can we make the mnt_want_write() conditional on
   O_CREAT? The read-only-open setattr path already re-takes write for
   itself, so it shouldn't need the blanket grab.

2. We open the target before checking it's a regular file.

   do_lookup_open() runs dentry_open() on any positive dentry, and
   nfsd_check_obj_isreg() doesn't run until nfsd4_open_file() returns.
   There's no O_NONBLOCK on that open, so an OPEN of an existing FIFO
   sits in fifo_open() waiting for a peer, and it waits with the parent
   directory's i_rwsem still held from start_creating(). One client can
   pin an nfsd thread and the parent directory that way; a handful of
   them and the pool is spent. A device node does the same thing through
   its ->open. We used to know the type before we opened anything, so
   could we check d_is_reg() ahead of the dentry_open()?

   In fairness this isn't new to 14/14. "always open file in
   nfsd4_create_file()" already opened existing files for the UNCHECKED
   case; this patch just extends it to plain reads.

3. Mount crossing and the target export check are gone.

   nfsd_lookup_dentry() went through nfsd_mountpoint() and
   nfsd_cross_mnt() and revalidated the export it crossed into.
   start_creating() is a plain one-component lookup, and we fh_compose()
   against the parent export. Bind-mount one regular file over another
   inside an export and the new path hands back the covered file instead
   of the mounted one, with no check_nfsd_access() on the real target.
   Narrow, since only file-over-file mounts reach it (a directory trips
   the isreg check), but the behavior did change.

Actually some of the suspect logic is added in earlier patches, only to
be exposed by this final switchover patch, so I'll wait for a re-roll.


--
Chuck Lever