On Mon, 2025-02-24 at 17:10 -0800, Dai Ngo wrote:
> On 2/24/25 7:48 AM, Jeff Layton wrote:
> > On Fri, 2025-02-21 at 15:42 -0800, Dai Ngo wrote:
> > > Allow READ using write delegation stateid granted on OPENs with
> > > OPEN4_SHARE_ACCESS_WRITE only, to accommodate clients whose WRITE
> > > implementation may unavoidably do (e.g., due to buffer cache
> > > constraints).
> > >
> > > When the server offers a write delegation for an OPEN with
> > > OPEN4_SHARE_ACCESS_WRITE, the file access mode, the nfs4_file
> > > and nfs4_ol_stateid are upgraded as if the OPEN was sent with
> > > OPEN4_SHARE_ACCESS_BOTH.
> > >
> > > When this delegation is returned or revoked, the corresponding open
> > > stateid is looked up and if it's found then the file access mode,
> > > the nfs4_file and nfs4_ol_stateid are downgraded to remove the read
> > > access.
> > >
> > > Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
> > > ---
> > > fs/nfsd/nfs4state.c | 62 +++++++++++++++++++++++++++++++++++++++++++++
> > > fs/nfsd/state.h | 2 ++
> > > 2 files changed, 64 insertions(+)
> > >
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index b533225e57cf..0c14f902c54c 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -6126,6 +6126,51 @@ nfs4_delegation_stat(struct nfs4_delegation *dp, struct svc_fh *currentfh,
> > > return rc == 0;
> > > }
> > >
> > > +/*
> > > + * Upgrade file access mode to include FMODE_READ. This is called only when
> > > + * a write delegation is granted for an OPEN with OPEN4_SHARE_ACCESS_WRITE.
> > > + */
> > > +static void
> > > +nfs4_upgrade_rdwr_file_access(struct nfs4_ol_stateid *stp)
> > > +{
> > > + struct nfs4_file *fp = stp->st_stid.sc_file;
> > > + struct nfsd_file *nflp;
> > > + struct file *file;
> > > +
> > > + spin_lock(&fp->fi_lock);
> > > + nflp = fp->fi_fds[O_WRONLY];
> > > + file = nflp->nf_file;
> > > + file->f_mode |= FMODE_READ;
> > You can't just do this. Open upgrade/downgrade doesn't exist at the VFS
> > layer currently. It might work with most local filesystems, but more
> > complex filesystems have significant context attached to a file. Just
> > because you've changed it here, doesn't mean that you will _actually_
> > be able to do reads using it.
>
> I think allowing read using a write delegation from a OPEN with
> WRONLY is an optional feature and is not a requirement from the
> spec. So if there are filesystems that do not allow this feature
> to work then it is ok; we did not introduce any new problems with
> this feature.
>
This patch is swapping the O_RDWR fd in the nfsd4_file with an O_WRONLY
one that has had FMODE_READ added. That will break on filesystems that
don't actually allow you to do reads on a filp that was opened
O_WRONLY.
> >
> > This might even be actively unsafe, as you're bypassing permissions
> > checks here. You never checked whether the file is readable. What if
> > it's write only? Same clients will do an ACCESS check before allowing
> > it, but a hostile actor might be able to exploit this.
>
> Apparently the NFS server relies on the NFS client to do permission
> check at time the file is opened. Once the permission check passes and
> the OPEN is successful, then there is no permission check on READ/WRITE.
>
> I wrote this pynfs test to verify:
>
> def testReadOnWrOnlyFile(t, env):
> """Test read using open stateid with OPEN4_SHARE_ACCESS_WRITE
> on file with permission 0222
>
> FLAGS: writedelegations deleg
> CODE: DELEG28
> """
>
> # create a file with write-only mode (0222)
> sess = env.c1.new_client_session(b"%s_2" % env.testname(t))
> filename = env.testname(t)
> res = open_create_file(sess, filename, open_create=OPEN4_CREATE,
> attrs={FATTR4_MODE: 0o222}, access=OPEN4_SHARE_ACCESS_WRITE, want_deleg=False)
> check(res)
>
> # write file content
> fh = res.resarray[-1].object
> stateid = res.resarray[-2].stateid
> data = b"write test data"
> res = write_file(sess, fh, data, 0, stateid)
> check(res)
>
> # close the file
> res = close_file(sess, fh, stateid)
> check(res)
>
> # OPEN file with OPEN4_SHARE_ACCESS_WRITE
> access = OPEN4_SHARE_ACCESS_WRITE|OPEN4_SHARE_ACCESS_WANT_NO_DELEG
> res = open_file(sess, filename, access = access, want_deleg = True)
nit: shouldn't want_deleg be False there?
> check(res)
>
> # READ file using open stateid
> # Linux NFS server allows READ using open stateid from OPEN4_SHARE_ACCESS_WRITE.
> # However, the file permission mode 0222 then the READ should fail!
> stateid = res.resarray[-2].stateid
> res = sess.compound([op.putfh(fh), op.read(stateid, 0, 10)])
> check(res, NFS4ERR_ACCESS)
> res = close_file(sess, fh, stateid)
> check(res)
>
> and the test failed with:
> "OP_READ should return NFS4ERR_ACCESS, instead got NFS4_OK"
>
> Am i missing something?
>
nfsd actually does check permissions on every READ operation via:
nfs4_preprocess_stateid_op
nfs4_check_file
nfsd_permission
You may be bypassing it via the NFSD_MAY_OWNER_OVERRIDE flag. Does the
above test also fail if the file is owned by a different user than the
one running the test?
>
> >
> > I think you need to acquire a R/W open from the get-go instead when you
> > intend to grant a delegation, and just fall back to doing a O_WRONLY
> > open if that fails (and don't grant one).
> >
> > > + swap(fp->fi_fds[O_RDWR], fp->fi_fds[O_WRONLY]);
> > > + clear_access(NFS4_SHARE_ACCESS_WRITE, stp);
> > > + set_access(NFS4_SHARE_ACCESS_BOTH, stp);
> > > + __nfs4_file_get_access(fp, NFS4_SHARE_ACCESS_READ); /* incr fi_access[O_RDONLY] */
> > > + spin_unlock(&fp->fi_lock);
> > > +}
> > > +
> > > +/*
> > > + * Downgrade file access mode to remove FMODE_READ. This is called when
> > > + * a write delegation, granted for an OPEN with OPEN4_SHARE_ACCESS_WRITE,
> > > + * is returned.
> > > + */
> > > +static void
> > > +nfs4_downgrade_wronly_file_access(struct nfs4_ol_stateid *stp)
> > > +{
> > > + struct nfs4_file *fp = stp->st_stid.sc_file;
> > > + struct nfsd_file *nflp;
> > > + struct file *file;
> > > +
> > > + spin_lock(&fp->fi_lock);
> > > + nflp = fp->fi_fds[O_RDWR];
> > > + file = nflp->nf_file;
> > > + file->f_mode &= ~FMODE_READ;
> > > + swap(fp->fi_fds[O_WRONLY], fp->fi_fds[O_RDWR]);
> > > + clear_access(NFS4_SHARE_ACCESS_BOTH, stp);
> > > + set_access(NFS4_SHARE_ACCESS_WRITE, stp);
> > > + spin_unlock(&fp->fi_lock);
> > > + nfs4_file_put_access(fp, NFS4_SHARE_ACCESS_READ); /* decr. fi_access[O_RDONLY] */
> > > +}
> > > +
> > > /*
> > > * The Linux NFS server does not offer write delegations to NFSv4.0
> > > * clients in order to avoid conflicts between write delegations and
> > > @@ -6207,6 +6252,11 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> > > dp->dl_cb_fattr.ncf_cur_fsize = stat.size;
> > > dp->dl_cb_fattr.ncf_initial_cinfo = nfsd4_change_attribute(&stat);
> > > trace_nfsd_deleg_write(&dp->dl_stid.sc_stateid);
> > > +
> > > + if ((open->op_share_access & NFS4_SHARE_ACCESS_BOTH) == NFS4_SHARE_ACCESS_WRITE) {
> > > + dp->dl_stateid = stp->st_stid.sc_stateid;
> > > + nfs4_upgrade_rdwr_file_access(stp);
> > > + }
> > > } else {
> > > open->op_delegate_type = deleg_ts ? OPEN_DELEGATE_READ_ATTRS_DELEG :
> > > OPEN_DELEGATE_READ;
> > > @@ -7710,6 +7760,8 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > struct nfs4_stid *s;
> > > __be32 status;
> > > struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> > > + struct nfs4_ol_stateid *stp;
> > > + struct nfs4_stid *stid;
> > >
> > > if ((status = fh_verify(rqstp, &cstate->current_fh, S_IFREG, 0)))
> > > return status;
> > > @@ -7724,6 +7776,16 @@ nfsd4_delegreturn(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > >
> > > trace_nfsd_deleg_return(stateid);
> > > destroy_delegation(dp);
> > > +
> > > + if (dp->dl_stateid.si_generation && dp->dl_stateid.si_opaque.so_id) {
> > > + if (!nfsd4_lookup_stateid(cstate, &dp->dl_stateid,
> > > + SC_TYPE_OPEN, 0, &stid, nn)) {
> > > + stp = openlockstateid(stid);
> > > + nfs4_downgrade_wronly_file_access(stp);
> > > + nfs4_put_stid(stid);
> > > + }
> > > + }
> > > +
> > > smp_mb__after_atomic();
> > > wake_up_var(d_inode(cstate->current_fh.fh_dentry));
> > > put_stateid:
> > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > > index 74d2d7b42676..3f2f1b92db66 100644
> > > --- a/fs/nfsd/state.h
> > > +++ b/fs/nfsd/state.h
> > > @@ -207,6 +207,8 @@ struct nfs4_delegation {
> > >
> > > /* for CB_GETATTR */
> > > struct nfs4_cb_fattr dl_cb_fattr;
> > > +
> > > + stateid_t dl_stateid; /* open stateid */
> > > };
> > >
> > > static inline bool deleg_is_read(u32 dl_type)
--
Jeff Layton <jlayton@xxxxxxxxxx>
