linux-kernel - Re: [PATCH] test 305230142ae0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Sun, 12 Nov 2023 07:18:16 +1030
From:   Qu Wenruo <quwenruo.btrfs@....com>
To:     Edward Adam Davis <eadavis@...com>, willy@...radead.org
Cc:     boris@....io, clm@...com, dsterba@...e.com, josef@...icpanda.com,
        linux-btrfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        syzbot+4d81015bc10889fd12ea@...kaller.appspotmail.com,
        syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] test 305230142ae0



On 2023/11/11 18:43, Edward Adam Davis wrote:
> On Sat, 11 Nov 2023 06:20:59 +0000, Matthew Wilcox wrote:
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -4931,7 +4931,8 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
>>>   		goto out;
>>>   	}
>>>
>>> -	*objectid = root->free_objectid++;
>>> +	while (find_qgroup_rb(root->fs_info, root->free_objectid++));
>>> +	*objectid = root->free_objectid;
>>
>> This looks buggy to me.  Let's say that free_objectid is currently 3.
>>
>> Before, it would assign 3 to *objectid, and increment free_objectid to
>> 4.  After (assuming the loop terminates on first iteration), it will
>> increment free_objectid to 4, then assign 4 to *objectid.
>>
>> I think you meant to write:
>>
>> 	while (find_qgroup_rb(root->fs_info, root->free_objectid))
>> 		root->free_objectid++;
>> 	*objectid = root->free_objectid++;
> Yes, your guess is correct.
>>
>> And the lesson here is that more compact code is not necessarily more
>> correct code.
>>
>> (I'm not making any judgement about whether this is the correct fix;
>> I don't understand btrfs well enough to have an opinion.  Just that
>> this is not an equivalent transformation)
> I don't have much knowledge about btrfs too, but one thing is clear: the qgroupid
> taken by create_snapshot() is calculated from btrfs_get_free_ojectid().
> At the same time, when calculating the new value in btrfs_get_free_ojectid(),
> it is clearly unreasonable to not determine whether the new value exists in the
> qgroup_tree tree.

Nope, it's totally wrong.

Qgroupid is bound to subvolumeid, thus getting a different id for
qgroupid is going to screw the whole thing up.

> Perhaps there are other methods to obtain a new qgroupid, but before obtaining
> a new value, it is necessary to perform a duplicate value judgment on qgroup_tree,
> otherwise similar problems may still occur.

If you don't really understand the context, the fix is never going to be
correct.

Thanks,
Qu

>
> edward
>
>