| Message ID | 20240823-nfsd-fixes-v1-0-fc99aa16f6a0@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | nfsd: CB_GETATTR fixes | expand |
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
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@suse.de> 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 - 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@suse.de> --- 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; }
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@suse.de> > 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@suse.de> > --- > 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.
On Mon, 2024-08-26 at 10:37 -0400, Jeff Layton wrote: > 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@suse.de> > > 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? > Doh! Nevermind -- I see it now. It looks like the break_lease tag is just in the wrong place. We should definitely fix that. In any case, your patch looks reasonable to me, but I couldn't get it to apply. Care to send a real PATCH instead? It's fine if you want to drop my patch and just replace it with yours. > > - 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@suse.de> > > --- > > 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.
On Mon, Aug 26, 2024 at 10:47:46AM -0400, Jeff Layton wrote: > On Mon, 2024-08-26 at 10:37 -0400, Jeff Layton wrote: > > 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@suse.de> > > > 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? > > > > Doh! Nevermind -- I see it now. It looks like the break_lease tag is > just in the wrong place. We should definitely fix that. > > In any case, your patch looks reasonable to me, but I couldn't get it > to apply. I applied Jeff's weekend CB_GETATTR patches to nfsd-fixes. If there's an additional bug fix carried in Neil's clean-up, I would like that to apply to that branch, as a small surgical fix, so it can go into v6.11-rc. Seems like these CB_GETATTR fixes need to be applicable to LTS kernels, so let's keep them narrow. > Care to send a real PATCH instead? It's fine if you want to > drop my patch and just replace it with yours. Neil, I'd prefer: 1) specific fixes to apply to the nfsd-fixes branch 2) larger clean-ups to apply to the nfsd-next branch Untangling nfsd4_deleg_getattr_conflict() is a sensible thing to do, IMO, but I'd bet that Linus would consider that development rather than an urgent bug fix.
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. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- Jeff Layton (2): nfsd: hold reference to delegation when updating it for cb_getattr nfsd: fix potential UAF in nfsd4_cb_getattr_release fs/nfsd/nfs4state.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) --- base-commit: a204501e1743d695ca2930ed25a2be9f8ced96d3 change-id: 20240823-nfsd-fixes-61f0c785d125 Best regards,