| Message ID | 20210519160635.333057-1-Anna.Schumaker@Netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSv4: Fix a NULL pointer dereference in pnfs_mark_matching_lsegs_return() | expand |
On Wed, 2021-05-19 at 12:06 -0400, schumaker.anna@gmail.com wrote: > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > Commit de144ff4234f changes _pnfs_return_layout() to call > pnfs_mark_matching_lsegs_return() passing NULL as the struct > pnfs_layout_range argument. Unfortunately, > pnfs_mark_matching_lsegs_return() doesn't check if we have a value > here > before dereferencing it, causing an oops. > > I'm able to hit this crash consistently when running connectathon > basic > tests on NFS v4.1/v4.2 against Ontap. > > Fixes: de144ff4234f ("NFSv4: Don't discard segments marked for return > in _pnfs_return_layout()") > Cc: stable@vger.kernel.org > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > --- > fs/nfs/pnfs.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > index 03e0b34c4a64..6d720afb7b70 100644 > --- a/fs/nfs/pnfs.c > +++ b/fs/nfs/pnfs.c > @@ -2484,12 +2484,12 @@ pnfs_mark_matching_lsegs_return(struct > pnfs_layout_hdr *lo, > set_bit(NFS_LSEG_LAYOUTRETURN, &lseg- > >pls_flags); > } > > - if (remaining) { > + if (remaining && return_range) { > pnfs_set_plh_return_info(lo, return_range->iomode, > seq); > return -EBUSY; > } > > - if (!list_empty(&lo->plh_return_segs)) { > + if (return_range && !list_empty(&lo->plh_return_segs)) { > pnfs_set_plh_return_info(lo, return_range->iomode, > seq); > return 0; This patch would mean we fail to mark the layout for return in situations where we clearly should be doing so. The lack of a return_range doesn't indicate that we don't want to return any layouts, but rather that we want to return all layouts. I suggest rather changing the caller, _pnfs_return_layout() to move that declaration of 'struct pnfs_layout_range range' out of the if statement so that it can be passed as an argument instead of NULL. > }
On Wed, May 19, 2021 at 12:43 PM Trond Myklebust <trondmy@hammerspace.com> wrote: > > On Wed, 2021-05-19 at 12:06 -0400, schumaker.anna@gmail.com wrote: > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > Commit de144ff4234f changes _pnfs_return_layout() to call > > pnfs_mark_matching_lsegs_return() passing NULL as the struct > > pnfs_layout_range argument. Unfortunately, > > pnfs_mark_matching_lsegs_return() doesn't check if we have a value > > here > > before dereferencing it, causing an oops. > > > > I'm able to hit this crash consistently when running connectathon > > basic > > tests on NFS v4.1/v4.2 against Ontap. > > > > Fixes: de144ff4234f ("NFSv4: Don't discard segments marked for return > > in _pnfs_return_layout()") > > Cc: stable@vger.kernel.org > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > --- > > fs/nfs/pnfs.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c > > index 03e0b34c4a64..6d720afb7b70 100644 > > --- a/fs/nfs/pnfs.c > > +++ b/fs/nfs/pnfs.c > > @@ -2484,12 +2484,12 @@ pnfs_mark_matching_lsegs_return(struct > > pnfs_layout_hdr *lo, > > set_bit(NFS_LSEG_LAYOUTRETURN, &lseg- > > >pls_flags); > > } > > > > - if (remaining) { > > + if (remaining && return_range) { > > pnfs_set_plh_return_info(lo, return_range->iomode, > > seq); > > return -EBUSY; > > } > > > > - if (!list_empty(&lo->plh_return_segs)) { > > + if (return_range && !list_empty(&lo->plh_return_segs)) { > > pnfs_set_plh_return_info(lo, return_range->iomode, > > seq); > > return 0; > > This patch would mean we fail to mark the layout for return in > situations where we clearly should be doing so. The lack of a > return_range doesn't indicate that we don't want to return any layouts, > but rather that we want to return all layouts. > > I suggest rather changing the caller, _pnfs_return_layout() to move > that declaration of 'struct pnfs_layout_range range' out of the if > statement so that it can be passed as an argument instead of NULL. Sure, I'll try that now. Thanks! Anna > > > } > > -- > Trond Myklebust > Linux NFS client maintainer, Hammerspace > trond.myklebust@hammerspace.com > >
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c index 03e0b34c4a64..6d720afb7b70 100644 --- a/fs/nfs/pnfs.c +++ b/fs/nfs/pnfs.c @@ -2484,12 +2484,12 @@ pnfs_mark_matching_lsegs_return(struct pnfs_layout_hdr *lo, set_bit(NFS_LSEG_LAYOUTRETURN, &lseg->pls_flags); } - if (remaining) { + if (remaining && return_range) { pnfs_set_plh_return_info(lo, return_range->iomode, seq); return -EBUSY; } - if (!list_empty(&lo->plh_return_segs)) { + if (return_range && !list_empty(&lo->plh_return_segs)) { pnfs_set_plh_return_info(lo, return_range->iomode, seq); return 0; }