| Message ID | 20221116151726.129217-8-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: fix inode->i_flctx accesses | expand |
> On Nov 16, 2022, at 10:17 AM, Jeff Layton <jlayton@kernel.org> wrote: > > nfsd currently doesn't access i_flctx safely everywhere. This requires a > smp_load_acquire, as the pointer is set via cmpxchg (a release > operation). > > Cc: Chuck Lever <chuck.lever@oracle.com> > Signed-off-by: Jeff Layton <jlayton@kernel.org> Acked-by: Chuck Lever <chuck.lever@oracle.com> > --- > fs/nfsd/nfs4state.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 836bd825ca4a..da8d0ea66229 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -4758,7 +4758,7 @@ nfs4_share_conflict(struct svc_fh *current_fh, unsigned int deny_type) > > static bool nfsd4_deleg_present(const struct inode *inode) > { > - struct file_lock_context *ctx = smp_load_acquire(&inode->i_flctx); > + struct file_lock_context *ctx = locks_inode_context(inode); > > return ctx && !list_empty_careful(&ctx->flc_lease); > } > @@ -5897,7 +5897,7 @@ nfs4_lockowner_has_blockers(struct nfs4_lockowner *lo) > > list_for_each_entry(stp, &lo->lo_owner.so_stateids, st_perstateowner) { > nf = stp->st_stid.sc_file; > - ctx = nf->fi_inode->i_flctx; > + ctx = locks_inode_context(nf->fi_inode); > if (!ctx) > continue; > if (locks_owner_has_blockers(ctx, lo)) > @@ -7713,7 +7713,7 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner) > } > > inode = locks_inode(nf->nf_file); > - flctx = inode->i_flctx; > + flctx = locks_inode_context(inode); > > if (flctx && !list_empty_careful(&flctx->flc_posix)) { > spin_lock(&flctx->flc_lock); > -- > 2.38.1 > -- Chuck Lever
On Wed, 2022-11-16 at 15:21 +0000, Chuck Lever III wrote: > > > On Nov 16, 2022, at 10:17 AM, Jeff Layton <jlayton@kernel.org> wrote: > > > > nfsd currently doesn't access i_flctx safely everywhere. This requires a > > smp_load_acquire, as the pointer is set via cmpxchg (a release > > operation). > > > > Cc: Chuck Lever <chuck.lever@oracle.com> > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > Acked-by: Chuck Lever <chuck.lever@oracle.com> > > > > --- > > fs/nfsd/nfs4state.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 836bd825ca4a..da8d0ea66229 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -4758,7 +4758,7 @@ nfs4_share_conflict(struct svc_fh *current_fh, unsigned int deny_type) > > > > static bool nfsd4_deleg_present(const struct inode *inode) > > { > > - struct file_lock_context *ctx = smp_load_acquire(&inode->i_flctx); > > + struct file_lock_context *ctx = locks_inode_context(inode); > > > > return ctx && !list_empty_careful(&ctx->flc_lease); > > } > > @@ -5897,7 +5897,7 @@ nfs4_lockowner_has_blockers(struct nfs4_lockowner *lo) > > > > list_for_each_entry(stp, &lo->lo_owner.so_stateids, st_perstateowner) { > > nf = stp->st_stid.sc_file; > > - ctx = nf->fi_inode->i_flctx; > > + ctx = locks_inode_context(nf->fi_inode); Thanks Chuck. To be clear: I think the above access is probably OK. We wouldn't have a lock stateid unless we had a valid lock context in the inode. That said, doing it this way keeps everything consistent, so I'm inclined to leave the patch as-is. check_for_locks definitely needs this though. > > if (!ctx) > > continue; > > if (locks_owner_has_blockers(ctx, lo)) > > @@ -7713,7 +7713,7 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner) > > } > > > > inode = locks_inode(nf->nf_file); > > - flctx = inode->i_flctx; > > + flctx = locks_inode_context(inode); > > > > if (flctx && !list_empty_careful(&flctx->flc_posix)) { > > spin_lock(&flctx->flc_lock); > > -- > > 2.38.1 > > > > -- > Chuck Lever > > >
On Wed, Nov 16, 2022 at 10:17:26AM -0500, Jeff Layton wrote: > nfsd currently doesn't access i_flctx safely everywhere. This requires a > smp_load_acquire, as the pointer is set via cmpxchg (a release > operation). Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 836bd825ca4a..da8d0ea66229 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -4758,7 +4758,7 @@ nfs4_share_conflict(struct svc_fh *current_fh, unsigned int deny_type) static bool nfsd4_deleg_present(const struct inode *inode) { - struct file_lock_context *ctx = smp_load_acquire(&inode->i_flctx); + struct file_lock_context *ctx = locks_inode_context(inode); return ctx && !list_empty_careful(&ctx->flc_lease); } @@ -5897,7 +5897,7 @@ nfs4_lockowner_has_blockers(struct nfs4_lockowner *lo) list_for_each_entry(stp, &lo->lo_owner.so_stateids, st_perstateowner) { nf = stp->st_stid.sc_file; - ctx = nf->fi_inode->i_flctx; + ctx = locks_inode_context(nf->fi_inode); if (!ctx) continue; if (locks_owner_has_blockers(ctx, lo)) @@ -7713,7 +7713,7 @@ check_for_locks(struct nfs4_file *fp, struct nfs4_lockowner *lowner) } inode = locks_inode(nf->nf_file); - flctx = inode->i_flctx; + flctx = locks_inode_context(inode); if (flctx && !list_empty_careful(&flctx->flc_posix)) { spin_lock(&flctx->flc_lock);
nfsd currently doesn't access i_flctx safely everywhere. This requires a smp_load_acquire, as the pointer is set via cmpxchg (a release operation). Cc: Chuck Lever <chuck.lever@oracle.com> Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/nfsd/nfs4state.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)