linux-kernel - Re: [PATCH v4] btrfs: add fs state details to error messages.

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

Message-ID: <20f14d85-6a07-e66d-4711-c16c6930c2a3@dorminy.me>
Date:   Thu, 24 Feb 2022 15:09:08 -0500
From:   Sweet Tea Dorminy <sweettea-kernel@...miny.me>
To:     dsterba@...e.cz, Chris Mason <clm@...com>,
        Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-team@...com
Subject: Re: [PATCH v4] btrfs: add fs state details to error messages.

>> All the other interactions with info->fs_state are test/set/clear_bit,
>> which treat the argument as volatile and are therefore safe to do from
>> multiple threads. Without the READ_ONCE (reading it as a volatile),
>> the compiler or cpu could turn the reads of info->fs_state in
>> for_each_set_bit() into writes of random stuff into info->fs_state,
>> potentially clearing the state bits or filling them with garbage.
> I'm not sure I'm missing something, but I find the above hard to
> believe. Concurrent access to a variable from multiple threads may not
> produce consistent results, but random writes should not happen when
> we're just reading.

Maybe I've been reading too many articles about the things compilers are 
technically allowed to do. But as per the following link, the C standard 
does permit compilers inventing writes except to atomics and volatiles: 
https://lwn.net/Articles/793253/#Invented%20Stores

>
>> Even if this is right, it'd be rare, but it would be exceeding weird
>> for a message to be logged listing an error and then future messages
>> be logged without any such state, or with a random collection of
>> garbage states.
> How would that happen? The volatile keyword is only a compiler hint not
> to do optimizations on the variable, what actually happens on the CPU
> level depends if the instruction is locked or not, so different threads
> may read different bits.
> You seem to imply that once a variable is not used with volatile
> semantics, even just for read, the result could lead to random writes
> because it's otherwise undefined.

Pretty much; once a variable is read without READ_ONCE, it's unsafe to 
write a new value on another thread that depends on the old value. 
Imagine a compiler which invents stores; then if you are both reading 
and setting a variable 'a' on different threads, the following could happen:

thread 1 (reads)       thread 2 (modifies)

reads a into tmp

stores junk into a

                                 reads junk from a

stores tmp into a

                                 writes junk | 2 to a


Now a contains junk indefinitely.


But if it's too theoretical, I'm happy to drop it and amend my paranoia 
level.


(Thanks for fixing the !CONFIG_PRINTK warning that btrfs_state_to_string 
was unused; sorry I missed it.)


Sweet Tea