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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <miaox@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 <miaox@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 majordomo@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <miaox@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 <miaox@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html