On Mon, 2024-08-26 at 09:22 +1000, NeilBrown wrote:
> On Sun, 25 Aug 2024, Chuck Lever wrote:
> > On Fri, Aug 23, 2024 at 06:27:37PM -0400, Jeff Layton wrote:
> > > Fixes for a couple of CB_GETATTR bugs I found while working on the
> > > delstid set. Mostly this just ensures that we hold references to the
> > > delegation while working with it.
> > >
> > >
> >
> > Applied to nfsd-fixes for v6.11-rc, thanks!
> >
> > [1/2] nfsd: hold reference to delegation when updating it for cb_getattr
> > commit: 8fceb5f6636bbbf803fe29fff59f138206559964
> > [2/2] nfsd: fix potential UAF in nfsd4_cb_getattr_release
> > commit: 8bc97f9b84c8852fcc56be2382f5115c518de785
> >
> > --
> > Chuck Lever
> >
>
> Maybe the following can tidy up that code. I can split this into
> a few separate patches if you like.
> Thoughts?
>
> Note that the patch is easier to review if you apply it then use "git
> diff -b".
>
> NeilBrown
>
>
> From: NeilBrown <neilb@xxxxxxx>
> Subject: [PATCH] nfsd: untangle code in nfsd4_deleg_getattr_conflict()
>
> The code in nfsd4_deleg_getattr_conflict() is convoluted and buggy.
>
> With this patch we:
> - properly handle non-nfsd leases. We must not assume flc_owner is a
> delegation unless fl_lmops == &nfsd_lease_mng_ops
AFAICT, non-nfsd leases are already properly handled (though I do agree
that the "flow" of this code is awkward). What case do you see that's
wrong?
> - move the main code out of the for loop
> - have a single exit which calls nfs4_put_stid()
> (and other exits which don't need to call that)
>
> Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> fs/nfsd/nfs4state.c | 130 ++++++++++++++++++++++----------------------
> 1 file changed, 65 insertions(+), 65 deletions(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 2c4b9a22b2bb..7672fa7a70f3 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -8837,6 +8837,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
> struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> struct inode *inode = d_inode(dentry);
> struct file_lock_context *ctx;
> + struct nfs4_delegation *dp = NULL;
> struct nfs4_cb_fattr *ncf;
> struct file_lease *fl;
> struct iattr attrs;
> @@ -8845,77 +8846,76 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct dentry *dentry,
> ctx = locks_inode_context(inode);
> if (!ctx)
> return 0;
> +
> +#define NON_NFSD_LEASE ((void*)1)
> +
> spin_lock(&ctx->flc_lock);
> for_each_file_lock(fl, &ctx->flc_lease) {
> - unsigned char type = fl->c.flc_type;
> -
> if (fl->c.flc_flags == FL_LAYOUT)
> continue;
> - if (fl->fl_lmops != &nfsd_lease_mng_ops) {
> - /*
> - * non-nfs lease, if it's a lease with F_RDLCK then
> - * we are done; there isn't any write delegation
> - * on this inode
> - */
> - if (type == F_RDLCK)
> - break;
> - goto break_lease;
> - }
> - if (type == F_WRLCK) {
> - struct nfs4_delegation *dp = fl->c.flc_owner;
> -
> - if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> - spin_unlock(&ctx->flc_lock);
> - return 0;
> - }
> -break_lease:
> - nfsd_stats_wdeleg_getattr_inc(nn);
> - dp = fl->c.flc_owner;
> - refcount_inc(&dp->dl_stid.sc_count);
> - ncf = &dp->dl_cb_fattr;
> - nfs4_cb_getattr(&dp->dl_cb_fattr);
> - spin_unlock(&ctx->flc_lock);
> - wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
> - TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
> - if (ncf->ncf_cb_status) {
> - /* Recall delegation only if client didn't respond */
> - status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> - if (status != nfserr_jukebox ||
> - !nfsd_wait_for_delegreturn(rqstp, inode)) {
> - nfs4_put_stid(&dp->dl_stid);
> - return status;
> - }
> - }
> - if (!ncf->ncf_file_modified &&
> - (ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> - ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
> - ncf->ncf_file_modified = true;
> - if (ncf->ncf_file_modified) {
> - int err;
> -
> - /*
> - * Per section 10.4.3 of RFC 8881, the server would
> - * not update the file's metadata with the client's
> - * modified size
> - */
> - attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
> - attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
> - inode_lock(inode);
> - err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
> - inode_unlock(inode);
> - if (err) {
> - nfs4_put_stid(&dp->dl_stid);
> - return nfserrno(err);
> - }
> - ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
> - *size = ncf->ncf_cur_fsize;
> - *modified = true;
> - }
> - nfs4_put_stid(&dp->dl_stid);
> - return 0;
> + if (fl->c.flc_type == F_WRLCK) {
> + if (fl->fl_lmops == &nfsd_lease_mng_ops)
> + dp = fl->c.flc_owner;
> + else
> + dp = NON_NFSD_LEASE;
> }
> break;
> }
> + if (dp == NULL || dp == NON_NFSD_LEASE ||
> + dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> + spin_unlock(&ctx->flc_lock);
> + if (dp == NON_NFSD_LEASE) {
> + status = nfserrno(nfsd_open_break_lease(inode,
> + NFSD_MAY_READ));
> + if (status != nfserr_jukebox ||
> + !nfsd_wait_for_delegreturn(rqstp, inode))
> + return status;
> + }
> + return 0;
> + }
> +
> + nfsd_stats_wdeleg_getattr_inc(nn);
> + refcount_inc(&dp->dl_stid.sc_count);
> + ncf = &dp->dl_cb_fattr;
> + nfs4_cb_getattr(&dp->dl_cb_fattr);
> spin_unlock(&ctx->flc_lock);
> - return 0;
> +
> + wait_on_bit_timeout(&ncf->ncf_cb_flags, CB_GETATTR_BUSY,
> + TASK_INTERRUPTIBLE, NFSD_CB_GETATTR_TIMEOUT);
> + if (ncf->ncf_cb_status) {
> + /* Recall delegation only if client didn't respond */
> + status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> + if (status != nfserr_jukebox ||
> + !nfsd_wait_for_delegreturn(rqstp, inode))
> + goto out_status;
> + }
> + if (!ncf->ncf_file_modified &&
> + (ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> + ncf->ncf_cur_fsize != ncf->ncf_cb_fsize))
> + ncf->ncf_file_modified = true;
> + if (ncf->ncf_file_modified) {
> + int err;
> +
> + /*
> + * Per section 10.4.3 of RFC 8881, the server would
> + * not update the file's metadata with the client's
> + * modified size
> + */
> + attrs.ia_mtime = attrs.ia_ctime = current_time(inode);
> + attrs.ia_valid = ATTR_MTIME | ATTR_CTIME | ATTR_DELEG;
> + inode_lock(inode);
> + err = notify_change(&nop_mnt_idmap, dentry, &attrs, NULL);
> + inode_unlock(inode);
> + if (err) {
> + status = nfserrno(err);
> + goto out_status;
> + }
> + ncf->ncf_cur_fsize = ncf->ncf_cb_fsize;
> + *size = ncf->ncf_cur_fsize;
> + *modified = true;
> + }
> + status = 0;
> +out_status:
> + nfs4_put_stid(&dp->dl_stid);
> + return status;
> }
Patch looks like a nice cleanup, but I don't think the Fixes tag is
appropriate here.
--
Jeff Layton <jlayton@xxxxxxxxxx>
