| Message ID | 1464152979-103988-3-git-send-email-loghyr@primarydata.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-05-24 at 22:09 -0700, Tom Haynes wrote: > Signed-off-by: Tom Haynes <loghyr@primarydata.com> > --- > fs/nfsd/blocklayout.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c > index 248adb6..9a195f1 100644 > --- a/fs/nfsd/blocklayout.c > +++ b/fs/nfsd/blocklayout.c > @@ -23,7 +23,7 @@ nfsd4_block_proc_layoutget(struct inode *inode, const struct svc_fh *fhp, > struct nfsd4_layout_seg *seg = &args->lg_seg; > struct super_block *sb = inode->i_sb; > u32 block_size = (1 << inode->i_blkbits); > - struct pnfs_block_extent *bex; > + struct pnfs_block_extent *bex = NULL; > struct iomap iomap; > u32 device_generation = 0; > int error; > @@ -105,9 +105,11 @@ nfsd4_block_proc_layoutget(struct inode *inode, const struct svc_fh *fhp, > return 0; > > out_error: > + kfree(bex); > seg->length = 0; > return nfserrno(error); > out_layoutunavailable: > + kfree(bex); > seg->length = 0; > return nfserr_layoutunavailable; > } Nice catch! Might be reasonable for stable? Reviewed-by: Jeff Layton <jlayton@poochiereds.net> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 24, 2016 at 10:09:37PM -0700, Tom Haynes wrote:
> Signed-off-by: Tom Haynes <loghyr@primarydata.com>
How was this reported?
Like other NFS procedures the private data should be freed by the
XDR encode callback (nfsd4_encode_layoutget in this case) even
in the error case. It could be that there is a bug somewhere,
but it probably shouldn't be fixed here.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
> On May 25, 2016, at 8:07 AM, Christoph Hellwig <hch@lst.de> wrote: > > On Tue, May 24, 2016 at 10:09:37PM -0700, Tom Haynes wrote: >> Signed-off-by: Tom Haynes <loghyr@primarydata.com> > > How was this reported? Code inspection. My guess is no one ever hit the error cases in there. > > Like other NFS procedures the private data should be freed by the > XDR encode callback (nfsd4_encode_layoutget in this case) even > in the error case. It could be that there is a bug somewhere, > but it probably shouldn't be fixed here. > No, it doesn’t do that on errors: nfsd4_layoutget(): nfserr = ops->proc_layoutget(d_inode(current_fh->fh_dentry), current_fh, lgp); if (nfserr) goto out_put_stid; nfserr = nfsd4_insert_layout(lgp, ls); out_put_stid: mutex_unlock(&ls->ls_mutex); nfs4_put_stid(&ls->ls_stid); out: return nfserr; } So on error we never do anything with the lgp and the memory would be dropped. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 25, 2016 at 06:12:25PM +0000, Thomas Haynes wrote: > > > On May 25, 2016, at 8:07 AM, Christoph Hellwig <hch@lst.de> wrote: > > > > On Tue, May 24, 2016 at 10:09:37PM -0700, Tom Haynes wrote: > >> Signed-off-by: Tom Haynes <loghyr@primarydata.com> > > > > How was this reported? > > Code inspection. My guess is no one ever hit the error cases > in there. > > > > > Like other NFS procedures the private data should be freed by the > > XDR encode callback (nfsd4_encode_layoutget in this case) even > > in the error case. It could be that there is a bug somewhere, > > but it probably shouldn't be fixed here. > > > > No, it doesn’t do that on errors: We have in nfsd4_block_proc_layoutget: bex = kzalloc(sizeof(*bex), GFP_KERNEL); if (!bex) goto out_error; args->lg_content = bex; and then in nfsd4_encode_layoutget: kfree(lgp->lg_content); So, I think we're OK as is? --b. > > nfsd4_layoutget(): > > nfserr = ops->proc_layoutget(d_inode(current_fh->fh_dentry), > current_fh, lgp); > if (nfserr) > goto out_put_stid; > > nfserr = nfsd4_insert_layout(lgp, ls); > > out_put_stid: > mutex_unlock(&ls->ls_mutex); > nfs4_put_stid(&ls->ls_stid); > out: > return nfserr; > } > > So on error we never do anything with the lgp and the memory would > be dropped. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c index 248adb6..9a195f1 100644 --- a/fs/nfsd/blocklayout.c +++ b/fs/nfsd/blocklayout.c @@ -23,7 +23,7 @@ nfsd4_block_proc_layoutget(struct inode *inode, const struct svc_fh *fhp, struct nfsd4_layout_seg *seg = &args->lg_seg; struct super_block *sb = inode->i_sb; u32 block_size = (1 << inode->i_blkbits); - struct pnfs_block_extent *bex; + struct pnfs_block_extent *bex = NULL; struct iomap iomap; u32 device_generation = 0; int error; @@ -105,9 +105,11 @@ nfsd4_block_proc_layoutget(struct inode *inode, const struct svc_fh *fhp, return 0; out_error: + kfree(bex); seg->length = 0; return nfserrno(error); out_layoutunavailable: + kfree(bex); seg->length = 0; return nfserr_layoutunavailable; }
Signed-off-by: Tom Haynes <loghyr@primarydata.com> --- fs/nfsd/blocklayout.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)