* [PATCH 0/2] misc kernel leak patches
@ 2013-10-02 12:51 Mark Tinguely
2013-10-02 12:51 ` [PATCH 1/2] xfs: fix memory leak in xfs_dir2_node_removename Mark Tinguely
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Mark Tinguely @ 2013-10-02 12:51 UTC (permalink / raw)
To: xfs
Here are a couple patches to free leaked memory.
First patch is v2 of a Coverity found leak in xfs_dir2_node_removename().
Second patch arose from an eariler leak found by Coverity. In looking
at that patch, Eric noted that it appeared that the transaction's
item list is also leaked if recovery fails. I tested this patch by forcing
errors into the recovery code to verify that the items are now being released.
I will send patches for xfsprogs equivalent.
--Mark.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] xfs: fix memory leak in xfs_dir2_node_removename 2013-10-02 12:51 [PATCH 0/2] misc kernel leak patches Mark Tinguely @ 2013-10-02 12:51 ` Mark Tinguely 2013-10-02 20:32 ` Dave Chinner 2013-10-02 12:51 ` [PATCH 2/2] xfs free the list of recovery items on error Mark Tinguely 2013-10-02 20:54 ` [PATCH 0/2] misc kernel leak patches Eric Sandeen 2 siblings, 1 reply; 8+ messages in thread From: Mark Tinguely @ 2013-10-02 12:51 UTC (permalink / raw) To: xfs [-- Attachment #1: xfs-fix-leak-in-xfs_dir2_node_removename.patch --] [-- Type: text/plain, Size: 1924 bytes --] Fix the leak of kernel memory in xfs_dir2_node_removename() when xfs_dir2_leafn_remove() returns an error code. Found by Coverity in userspace same patch applies there also. Signed-off-by: Mark Tinguely <tinguely@sgi.com> --- v2 corrected bad return code as pointed out by Roger Willcocks. fs/xfs/xfs_dir2_node.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) Index: b/fs/xfs/xfs_dir2_node.c =================================================================== --- a/fs/xfs/xfs_dir2_node.c +++ b/fs/xfs/xfs_dir2_node.c @@ -2105,12 +2105,12 @@ xfs_dir2_node_lookup( */ int /* error */ xfs_dir2_node_removename( - xfs_da_args_t *args) /* operation arguments */ + struct xfs_da_args *args) /* operation arguments */ { - xfs_da_state_blk_t *blk; /* leaf block */ + struct xfs_da_state_blk *blk; /* leaf block */ int error; /* error return value */ int rval; /* operation return value */ - xfs_da_state_t *state; /* btree cursor */ + struct xfs_da_state *state; /* btree cursor */ trace_xfs_dir2_node_removename(args); @@ -2132,9 +2132,10 @@ xfs_dir2_node_removename( * Didn't find it, upper layer screwed up. */ if (rval != EEXIST) { - xfs_da_state_free(state); - return rval; + error = rval; + goto done; } + blk = &state->path.blk[state->path.active - 1]; ASSERT(blk->magic == XFS_DIR2_LEAFN_MAGIC); ASSERT(state->extravalid); @@ -2145,7 +2146,7 @@ xfs_dir2_node_removename( error = xfs_dir2_leafn_remove(args, blk->bp, blk->index, &state->extrablk, &rval); if (error) - return error; + goto done; /* * Fix the hash values up the btree. */ @@ -2160,6 +2161,7 @@ xfs_dir2_node_removename( */ if (!error) error = xfs_dir2_node_to_leaf(state); +done: xfs_da_state_free(state); return error; } _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: fix memory leak in xfs_dir2_node_removename 2013-10-02 12:51 ` [PATCH 1/2] xfs: fix memory leak in xfs_dir2_node_removename Mark Tinguely @ 2013-10-02 20:32 ` Dave Chinner 2013-10-02 21:04 ` Mark Tinguely 0 siblings, 1 reply; 8+ messages in thread From: Dave Chinner @ 2013-10-02 20:32 UTC (permalink / raw) To: Mark Tinguely; +Cc: xfs On Wed, Oct 02, 2013 at 07:51:11AM -0500, Mark Tinguely wrote: > Fix the leak of kernel memory in xfs_dir2_node_removename() > when xfs_dir2_leafn_remove() returns an error code. > > Found by Coverity in userspace same patch applies there also. > > Signed-off-by: Mark Tinguely <tinguely@sgi.com> > --- > v2 corrected bad return code as pointed out by Roger Willcocks. > > fs/xfs/xfs_dir2_node.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > Index: b/fs/xfs/xfs_dir2_node.c > =================================================================== > --- a/fs/xfs/xfs_dir2_node.c > +++ b/fs/xfs/xfs_dir2_node.c > @@ -2105,12 +2105,12 @@ xfs_dir2_node_lookup( > */ > int /* error */ > xfs_dir2_node_removename( > - xfs_da_args_t *args) /* operation arguments */ > + struct xfs_da_args *args) /* operation arguments */ > { > - xfs_da_state_blk_t *blk; /* leaf block */ > + struct xfs_da_state_blk *blk; /* leaf block */ > int error; /* error return value */ > int rval; /* operation return value */ > - xfs_da_state_t *state; /* btree cursor */ > + struct xfs_da_state *state; /* btree cursor */ > > trace_xfs_dir2_node_removename(args); > > @@ -2132,9 +2132,10 @@ xfs_dir2_node_removename( > * Didn't find it, upper layer screwed up. > */ > if (rval != EEXIST) { > - xfs_da_state_free(state); > - return rval; > + error = rval; > + goto done; Can you make this something like "out_free"? We tend to name jump labels according to the action that needs to be taken, like "out_unlock", "error_trans_cancel", etc... Also, this code is now "funky" in how it handles rval. it will do this: /* * Look up the entry we're deleting, set up the cursor. */ error = xfs_da3_node_lookup_int(state, &rval); if (error) rval = error; /* * Didn't find it, upper layer screwed up. */ if (rval != EEXIST) { error = rval; goto out_free; } That's kind funky with the reassignment of rval if there's an error. Better would be: /* Look up the entry we're deleting, set up the cursor. */ error = xfs_da3_node_lookup_int(state, &rval); if (error) goto out_free; /* Didn't find it, upper layer screwed up. */ if (rval != EEXIST) { error = rval; goto out_free; } Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: fix memory leak in xfs_dir2_node_removename 2013-10-02 20:32 ` Dave Chinner @ 2013-10-02 21:04 ` Mark Tinguely 0 siblings, 0 replies; 8+ messages in thread From: Mark Tinguely @ 2013-10-02 21:04 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs On 10/02/13 15:32, Dave Chinner wrote: > On Wed, Oct 02, 2013 at 07:51:11AM -0500, Mark Tinguely wrote: >> Fix the leak of kernel memory in xfs_dir2_node_removename() >> when xfs_dir2_leafn_remove() returns an error code. >> >> Found by Coverity in userspace same patch applies there also. >> >> Signed-off-by: Mark Tinguely<tinguely@sgi.com> >> --- >> v2 corrected bad return code as pointed out by Roger Willcocks. >> >> fs/xfs/xfs_dir2_node.c | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) >> >> Index: b/fs/xfs/xfs_dir2_node.c >> =================================================================== >> --- a/fs/xfs/xfs_dir2_node.c >> +++ b/fs/xfs/xfs_dir2_node.c >> @@ -2105,12 +2105,12 @@ xfs_dir2_node_lookup( >> */ >> int /* error */ >> xfs_dir2_node_removename( >> - xfs_da_args_t *args) /* operation arguments */ >> + struct xfs_da_args *args) /* operation arguments */ >> { >> - xfs_da_state_blk_t *blk; /* leaf block */ >> + struct xfs_da_state_blk *blk; /* leaf block */ >> int error; /* error return value */ >> int rval; /* operation return value */ >> - xfs_da_state_t *state; /* btree cursor */ >> + struct xfs_da_state *state; /* btree cursor */ >> >> trace_xfs_dir2_node_removename(args); >> >> @@ -2132,9 +2132,10 @@ xfs_dir2_node_removename( >> * Didn't find it, upper layer screwed up. >> */ >> if (rval != EEXIST) { >> - xfs_da_state_free(state); >> - return rval; >> + error = rval; >> + goto done; > > Can you make this something like "out_free"? We tend to name jump > labels according to the action that needs to be taken, like > "out_unlock", "error_trans_cancel", etc... > > Also, this code is now "funky" in how it handles rval. it will do > this: > > /* > * Look up the entry we're deleting, set up the cursor. > */ > error = xfs_da3_node_lookup_int(state,&rval); > if (error) > rval = error; > /* > * Didn't find it, upper layer screwed up. > */ > if (rval != EEXIST) { > error = rval; > goto out_free; > } > > That's kind funky with the reassignment of rval if there's an error. > Better would be: > > /* Look up the entry we're deleting, set up the cursor. */ > error = xfs_da3_node_lookup_int(state,&rval); > if (error) > goto out_free; > > /* Didn't find it, upper layer screwed up. */ > if (rval != EEXIST) { > error = rval; > goto out_free; > } > > Cheers, > > Dave. nod. thank --Mark. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] xfs free the list of recovery items on error. 2013-10-02 12:51 [PATCH 0/2] misc kernel leak patches Mark Tinguely 2013-10-02 12:51 ` [PATCH 1/2] xfs: fix memory leak in xfs_dir2_node_removename Mark Tinguely @ 2013-10-02 12:51 ` Mark Tinguely 2013-12-04 20:51 ` Ben Myers 2013-10-02 20:54 ` [PATCH 0/2] misc kernel leak patches Eric Sandeen 2 siblings, 1 reply; 8+ messages in thread From: Mark Tinguely @ 2013-10-02 12:51 UTC (permalink / raw) To: xfs [-- Attachment #1: xfs-fix-leaked-xlog_recover_items.patch --] [-- Type: text/plain, Size: 2006 bytes --] Recovery builds a list of items on the transaction's r_itemq head. Normally these items are committed and freed. But in the event of a recovery error, these allocations are leaked. If the error occurs during item reordering, then reconstruct the r_itemq list before deleting the list to avoid leaking the entries that were on one of the temporary lists. Signed-off-by: Mark Tinguely <tinguely@sgi.com> --- fs/xfs/xfs_log_recover.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) Index: b/fs/xfs/xfs_log_recover.c =================================================================== --- a/fs/xfs/xfs_log_recover.c +++ b/fs/xfs/xfs_log_recover.c @@ -1659,6 +1659,7 @@ xlog_recover_reorder_trans( int pass) { xlog_recover_item_t *item, *n; + int error = 0; LIST_HEAD(sort_list); LIST_HEAD(cancel_list); LIST_HEAD(buffer_list); @@ -1700,9 +1701,17 @@ xlog_recover_reorder_trans( "%s: unrecognized type of log operation", __func__); ASSERT(0); - return XFS_ERROR(EIO); + /* + * return the remaining items back to the transaction + * item list so they can be freed in caller. + */ + if (!list_empty(&sort_list)) + list_splice_init(&sort_list, &trans->r_itemq); + error = XFS_ERROR(EIO); + goto out; } } +out: ASSERT(list_empty(&sort_list)); if (!list_empty(&buffer_list)) list_splice(&buffer_list, &trans->r_itemq); @@ -1712,7 +1721,7 @@ xlog_recover_reorder_trans( list_splice_tail(&inode_buffer_list, &trans->r_itemq); if (!list_empty(&cancel_list)) list_splice_tail(&cancel_list, &trans->r_itemq); - return 0; + return error; } /* @@ -3743,8 +3752,10 @@ xlog_recover_process_data( error = XFS_ERROR(EIO); break; } - if (error) + if (error) { + xlog_recover_free_trans(trans); return error; + } } dp += be32_to_cpu(ohead->oh_len); num_logops--; _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs free the list of recovery items on error. 2013-10-02 12:51 ` [PATCH 2/2] xfs free the list of recovery items on error Mark Tinguely @ 2013-12-04 20:51 ` Ben Myers 2013-12-05 22:52 ` Ben Myers 0 siblings, 1 reply; 8+ messages in thread From: Ben Myers @ 2013-12-04 20:51 UTC (permalink / raw) To: Mark Tinguely; +Cc: xfs On Wed, Oct 02, 2013 at 07:51:12AM -0500, Mark Tinguely wrote: > Recovery builds a list of items on the transaction's > r_itemq head. Normally these items are committed and freed. > But in the event of a recovery error, these allocations > are leaked. > > If the error occurs during item reordering, then reconstruct > the r_itemq list before deleting the list to avoid leaking > the entries that were on one of the temporary lists. > > Signed-off-by: Mark Tinguely <tinguely@sgi.com> Looks good to me. Reviewed-by: Ben Myers <bpm@sgi.com> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs free the list of recovery items on error. 2013-12-04 20:51 ` Ben Myers @ 2013-12-05 22:52 ` Ben Myers 0 siblings, 0 replies; 8+ messages in thread From: Ben Myers @ 2013-12-05 22:52 UTC (permalink / raw) To: Mark Tinguely; +Cc: xfs On Wed, Dec 04, 2013 at 02:51:05PM -0600, Ben Myers wrote: > On Wed, Oct 02, 2013 at 07:51:12AM -0500, Mark Tinguely wrote: > > Recovery builds a list of items on the transaction's > > r_itemq head. Normally these items are committed and freed. > > But in the event of a recovery error, these allocations > > are leaked. > > > > If the error occurs during item reordering, then reconstruct > > the r_itemq list before deleting the list to avoid leaking > > the entries that were on one of the temporary lists. > > > > Signed-off-by: Mark Tinguely <tinguely@sgi.com> > > Looks good to me. > Reviewed-by: Ben Myers <bpm@sgi.com> Applied. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/2] misc kernel leak patches 2013-10-02 12:51 [PATCH 0/2] misc kernel leak patches Mark Tinguely 2013-10-02 12:51 ` [PATCH 1/2] xfs: fix memory leak in xfs_dir2_node_removename Mark Tinguely 2013-10-02 12:51 ` [PATCH 2/2] xfs free the list of recovery items on error Mark Tinguely @ 2013-10-02 20:54 ` Eric Sandeen 2 siblings, 0 replies; 8+ messages in thread From: Eric Sandeen @ 2013-10-02 20:54 UTC (permalink / raw) To: Mark Tinguely; +Cc: xfs On 10/2/13 7:51 AM, Mark Tinguely wrote: > Here are a couple patches to free leaked memory. > > First patch is v2 of a Coverity found leak in xfs_dir2_node_removename(). > > Second patch arose from an eariler leak found by Coverity. In looking > at that patch, Eric noted that it appeared that the transaction's > item list is also leaked if recovery fails. I tested this patch by forcing > errors into the recovery code to verify that the items are now being released. > > I will send patches for xfsprogs equivalent. Good plan w.r.t. kernel first, progs 2nd, I think. Makes it less likely to accidentally send a free(); or similar upstream. :) -Eric _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-12-05 22:52 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-02 12:51 [PATCH 0/2] misc kernel leak patches Mark Tinguely 2013-10-02 12:51 ` [PATCH 1/2] xfs: fix memory leak in xfs_dir2_node_removename Mark Tinguely 2013-10-02 20:32 ` Dave Chinner 2013-10-02 21:04 ` Mark Tinguely 2013-10-02 12:51 ` [PATCH 2/2] xfs free the list of recovery items on error Mark Tinguely 2013-12-04 20:51 ` Ben Myers 2013-12-05 22:52 ` Ben Myers 2013-10-02 20:54 ` [PATCH 0/2] misc kernel leak patches Eric Sandeen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox