Re: [PATCH v2] btrfs: relocation: Fix KASAN reports caused by extended reloc tree lifespan

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Josef Bacik <josef@toxicpanda.com>,
	Nikolay Borisov <nborisov@suse.com>, Qu Wenruo <wqu@suse.com>,
	linux-btrfs@vger.kernel.org
Cc: Zygo Blaxell <ce3g8jdj@umail.furryterror.org>,
	David Sterba <dsterba@suse.com>
Subject: Re: [PATCH v2] btrfs: relocation: Fix KASAN reports caused by extended reloc tree lifespan
Date: Thu, 9 Jan 2020 08:11:20 +0800	[thread overview]
Message-ID: <f5d2693b-9b30-e067-1ed5-a40255e8991b@gmx.com> (raw)
In-Reply-To: <3b6e5dc3-c1fc-c619-9939-16ffc0f1eacb@toxicpanda.com>

On 2020/1/8 下午11:19, Josef Bacik wrote:
> On 1/8/20 10:03 AM, Nikolay Borisov wrote:
[...]
>>>> + */
>>>> +static bool have_reloc_root(struct btrfs_root *root)
>>>> +{
>>>> +    if (test_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state))
>>>> +        return false;
>>>
>>> You still need a smp_mb__after_atomic() here, test_bit is unordered.
>>
>> Nope, that won't do anything, since smp_mb__(After|before)_atomic only
>> orders RMW operations and test_bit is not an RMW operation. From
>> atomic_bitops.txt:
>>
>>
>> Non-RMW ops:
>>
>>
>>
>>    test_bit()
>>
>> Furthermore from atomic_t.txt:
>>
>> The barriers:
>>
>>
>>
>>    smp_mb__{before,after}_atomic()
>>
>>
>>
>> only apply to the RMW atomic ops and can be used to augment/upgrade the
>>
>> ordering inherent to the op.
>
> Right but the document says it's unordered.  The problem we're trying to
> address here is making sure _either_ we see BTRFS_ROOT_DEAD_RELOC_TREE
> or we see !root->reloc_root.  Which means we don't want the test_bit
> being re-ordered WRT the clear_bit on the other side.  So the other side
> does
>
> reloc_root = NULL;
> smp_mb__before_atomic();
> clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE);

Yes, that's correct.

>
> now say on the other side we get re-ordered and we see reloc_root !=
> NULL and we also see !test_bit(BTRFS_ROOT_DEAD_RELOC_TREE), and now
> we're screwed.

That's not possible. With above mb, there are only several possible
results on the reader side:
A: Reloc_root == PTR, and DEAD_RELOC_TREE : Before NULL assignment
B: Reloc_root == NULL, and DEAD_RELOC_TREE: after NULL assignment
C: Reloc_root == NULL, no DEAD_RELOC_TREE: after clear_bit().

That's what mb() is doing, killing the unwanted case:
D: Reloc_root == PTR, no DEAD_RELOC_TREE.

On the reader side, even with the mb, the test_bit() can happen whatever
they want, mb makes no sense for *single* memory access.

>
> The smp_mb__after_atomic() guarantees that the re-ordering doesn't
> happen, correct?

That smp_mb() has no effect, as it's not defining any extra order, since
there is no extra memory access to start with.

And definitely has nothing to do with reader side, as reader can really
happen whenever they like.

From what I learnt recently, mb is only needed between at least *two*
memory access where the order is really important (to say, kill certain
re-order possibility).

If there are no two critical accesses at both side, then a mb makes no
sense.

Hopes this would help.

Thanks,
Qu

>
> I realize that this is mostly a moot point on real architectures, but
> I'm looking at things like ARM where test_bit() uses the generic asm
> helper, which could definitely be re-ordered as it's nothing special. 
> Thanks,
>
> Josef
>