linux-kernel - Re: [PATCH] btrfs: fix waitqueue_active without memory barrier in btrfs

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>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 10 Oct 2015 05:03:29 +0000
From:	Kosuke Tatsukawa <tatsu@...jp.nec.com>
To:	"dsterba@...e.cz" <dsterba@...e.cz>
CC:	Chris Mason <clm@...com>, Josef Bacik <jbacik@...com>,
	David Sterba <dsterba@...e.com>,
	"linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] btrfs: fix waitqueue_active without memory barrier in
 btrfs 

David Sterba wrote:
> On Fri, Oct 09, 2015 at 12:35:48AM +0000, Kosuke Tatsukawa wrote:
>> This patch removes the call to waitqueue_active() leaving just wake_up()
>> behind.  This fixes the problem because the call to spin_lock_irqsave()
>> in wake_up() will be an ACQUIRE operation.
>
> Either we can switch it to wake_up or put the barrier before the check.
> Not all instances of waitqueue_active need the barrier though.
>
>> I found this issue when I was looking through the linux source code
>> for places calling waitqueue_active() before wake_up*(), but without
>> preceding memory barriers, after sending a patch to fix a similar
>> issue in drivers/tty/n_tty.c  (Details about the original issue can be
>> found here: https://lkml.org/lkml/2015/9/28/849).
>
> There are more in btrfs:
>
> https://www.mail-archive.com/linux-btrfs%40vger.kernel.org/msg41914.html

Thank you for the pointer.
Your patch seems better than mine.

I think the other places in btrfs that use waitqueue_active() before
wake_up are preceded by either a smp_mb or some kind of atomic
operation.

The latter still needs smp_mb__after_atomic() but it's light-weight
compared to smp_mb().


>> @@ -918,9 +918,7 @@ void btrfs_bio_counter_inc_noblocked(struct btrfs_fs_info *fs_info)
>>  void btrfs_bio_counter_sub(struct btrfs_fs_info *fs_info, s64 amount)
>>  {
>>  	percpu_counter_sub(&fs_info->bio_counter, amount);
>> -
>> -	if (waitqueue_active(&fs_info->replace_wait))
>> -		wake_up(&fs_info->replace_wait);
>> +	wake_up(&fs_info->replace_wait);
>
> Chris had a comment on that one in
> https://www.mail-archive.com/linux-btrfs%40vger.kernel.org/msg42551.html
> it's in performance critial context and the explicit wake_up is even
> worse than the barrier.
---
Kosuke TATSUKAWA  | 3rd IT Platform Department
                  | IT Platform Division, NEC Corporation
                  | tatsu@...jp.nec.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/