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