Leaking btrfs_qgroup_inherit on snapshot creation?

2013-02-06 Thread Alex Lyakas 
Hi Jan, Arne,
I see this code in create_snapshot:

if (inherit) {
pending_snapshot-inherit = *inherit;
*inherit = NULL;/* take responsibility to free it */
}

So, first thing I think it should be:
if (*inherit)
because in btrfs_ioctl_snap_create_v2() we have:
struct btrfs_qgroup_inherit *inherit = NULL;
...
btrfs_ioctl_snap_create_transid(..., inherit)

so the current check is very unlikely to be NULL.

Second, I don't see anybody freeing pending_snapshot-inherit. I guess
it should be freed after callin btrfs_qgroup_inherit() and also in
btrfs_destroy_pending_snapshots().

Thanks,
Alex.
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Leaking btrfs_qgroup_inherit on snapshot creation?

2013-02-06 Thread Arne Jansen 
Hi Alex,

On 02/06/13 12:18, Alex Lyakas wrote:
 Hi Jan, Arne,
 I see this code in create_snapshot:
 
   if (inherit) {
   pending_snapshot-inherit = *inherit;
   *inherit = NULL;/* take responsibility to free it */
   }
 
 So, first thing I think it should be:
 if (*inherit)
 because in btrfs_ioctl_snap_create_v2() we have:
 struct btrfs_qgroup_inherit *inherit = NULL;
 ...
 btrfs_ioctl_snap_create_transid(..., inherit)
 
 so the current check is very unlikely to be NULL.

But in btrfs_ioctl_snap_create it is called with NULL, so *inherit would
dereference a NULL pointer.

 
 Second, I don't see anybody freeing pending_snapshot-inherit. I guess
 it should be freed after callin btrfs_qgroup_inherit() and also in
 btrfs_destroy_pending_snapshots().

You're right. In our original version (6f72c7e20dbaea5) it was still
there, in transaction.c. It has been removed in 6fa9700e734:

commit 6fa9700e734275de2acbcb0e99414bd7ddfc60f1
Author: Miao Xie mi...@cn.fujitsu.com
Date:   Thu Sep 6 04:00:32 2012 -0600

Btrfs: fix error path in create_pending_snapshot()

This patch fixes the following problem:
- If we failed to deal with the delayed dir items, we should abort
transaction,
  just as its comment said. Fix it.
- If root reference or root back reference insertion failed, we should
  abort transaction. Fix it.
- Fix the double free problem of pending-inherit.
- Do not restore the trans-rsv if we doesn't change it.
- make the error path more clearly.

Signed-off-by: Miao Xie mi...@cn.fujitsu.com

Miao, can you please explain where you see a double free?

-Arne


 Thanks,
 Alex.
 --
 To unsubscribe from this list: send the line unsubscribe linux-btrfs in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 

--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Leaking btrfs_qgroup_inherit on snapshot creation?

2013-02-06 Thread Miao Xie 
On wed, 06 Feb 2013 13:14:23 +0100, Arne Jansen wrote:
 Hi Alex,
 
 On 02/06/13 12:18, Alex Lyakas wrote:
 Hi Jan, Arne,
 I see this code in create_snapshot:

  if (inherit) {
  pending_snapshot-inherit = *inherit;
  *inherit = NULL;/* take responsibility to free it */
  }

 So, first thing I think it should be:
 if (*inherit)
 because in btrfs_ioctl_snap_create_v2() we have:
 struct btrfs_qgroup_inherit *inherit = NULL;
 ...
 btrfs_ioctl_snap_create_transid(..., inherit)

 so the current check is very unlikely to be NULL.
 
 But in btrfs_ioctl_snap_create it is called with NULL, so *inherit would
 dereference a NULL pointer.
 

 Second, I don't see anybody freeing pending_snapshot-inherit. I guess
 it should be freed after callin btrfs_qgroup_inherit() and also in
 btrfs_destroy_pending_snapshots().
 
 You're right. In our original version (6f72c7e20dbaea5) it was still
 there, in transaction.c. It has been removed in 6fa9700e734:
 
 commit 6fa9700e734275de2acbcb0e99414bd7ddfc60f1
 Author: Miao Xie mi...@cn.fujitsu.com
 Date:   Thu Sep 6 04:00:32 2012 -0600
 
 Btrfs: fix error path in create_pending_snapshot()
 
 This patch fixes the following problem:
 - If we failed to deal with the delayed dir items, we should abort
 transaction,
   just as its comment said. Fix it.
 - If root reference or root back reference insertion failed, we should
   abort transaction. Fix it.
 - Fix the double free problem of pending-inherit.
 - Do not restore the trans-rsv if we doesn't change it.
 - make the error path more clearly.
 
 Signed-off-by: Miao Xie mi...@cn.fujitsu.com
 
 Miao, can you please explain where you see a double free?

Sorry, I misread the code，I didn't notice that the pointer had been assigned
to NULL.

But I think we can make the code more readable and be easy to maintain, we can
free the memory in the caller(btrfs_ioctl_snap_create_v2()) since we are sure
the snapshot creation is done after btrfs_ioctl_snap_create_transid() completes.

Thanks
Miao

--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html