| Message ID | 20220527133428.2291945-1-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | xfs: fix xfs_ifree() error handling to not leak perag ref | expand |
On Fri, May 27, 2022 at 09:34:28AM -0400, Brian Foster wrote: > For some reason commit 9a5280b312e2e ("xfs: reorder iunlink remove > operation in xfs_ifree") replaced a jump to the exit path in the > event of an xfs_difree() error with a direct return, which skips > releasing the perag reference acquired at the top of the function. > Restore the original code to drop the reference on error. > > Fixes: 9a5280b312e2e ("xfs: reorder iunlink remove operation in xfs_ifree") > Signed-off-by: Brian Foster <bfoster@redhat.com> Doh. Good catch! Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index b2879870a17e..52d6f2c7d58b 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -2622,7 +2622,7 @@ xfs_ifree( > */ > error = xfs_difree(tp, pag, ip->i_ino, &xic); > if (error) > - return error; > + goto out; > > error = xfs_iunlink_remove(tp, pag, ip); > if (error) > -- > 2.34.1 >
On Fri, May 27, 2022 at 09:34:28AM -0400, Brian Foster wrote: > For some reason commit 9a5280b312e2e ("xfs: reorder iunlink remove > operation in xfs_ifree") replaced a jump to the exit path in the > event of an xfs_difree() error with a direct return, which skips Reason: the patchset that the fix was promoted from had moved all the pag handling out of xfs_ifree to the caller, so direct return was, in fact, the correct behaviour in the code it originated from. The patch applied to upstream with no errors or fuzz, and I had completely forgotten that somewhere in the ~50 patches in the stack before that fix had completely reworked unlink pag handling... How did you find this? I haven't noticed any specific increase in unmount perag accounting leaks as a result of this, so I'm curious as to how you noticed it and isolated it to this specific bug. > Restore the original code to drop the reference on error. > > Fixes: 9a5280b312e2e ("xfs: reorder iunlink remove operation in xfs_ifree") > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/xfs_inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > index b2879870a17e..52d6f2c7d58b 100644 > --- a/fs/xfs/xfs_inode.c > +++ b/fs/xfs/xfs_inode.c > @@ -2622,7 +2622,7 @@ xfs_ifree( > */ > error = xfs_difree(tp, pag, ip->i_ino, &xic); > if (error) > - return error; > + goto out; Looks good, Reviewed-by: Dave Chinner <dchinner@redhat.com>
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c index b2879870a17e..52d6f2c7d58b 100644 --- a/fs/xfs/xfs_inode.c +++ b/fs/xfs/xfs_inode.c @@ -2622,7 +2622,7 @@ xfs_ifree( */ error = xfs_difree(tp, pag, ip->i_ino, &xic); if (error) - return error; + goto out; error = xfs_iunlink_remove(tp, pag, ip); if (error)
For some reason commit 9a5280b312e2e ("xfs: reorder iunlink remove operation in xfs_ifree") replaced a jump to the exit path in the event of an xfs_difree() error with a direct return, which skips releasing the perag reference acquired at the top of the function. Restore the original code to drop the reference on error. Fixes: 9a5280b312e2e ("xfs: reorder iunlink remove operation in xfs_ifree") Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/xfs/xfs_inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)