linux-ext4 - Re: [PATCH] ext4/jbd2: drop jbd2_transaction

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: <8d85f75e-cf9e-b3da-766f-59d80d608203@huaweicloud.com>
Date: Mon, 20 May 2024 20:39:51 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
 ritesh.list@...il.com, yi.zhang@...wei.com, chengzhihao1@...wei.com,
 yukuai3@...wei.com
Subject: Re: [PATCH] ext4/jbd2: drop jbd2_transaction_committed()

On 2024/5/20 16:49, Jan Kara wrote:
> On Thu 16-05-24 16:27:25, Zhang Yi wrote:
>> On 2024/5/15 8:25, Jan Kara wrote:
>>> On Mon 13-05-24 15:21:19, Zhang Yi wrote:
>>> Also accessing j_commit_sequence without any
>>> lock is theoretically problematic wrt compiler optimization. You should have
>>> READ_ONCE() there and the places modifying j_commit_sequence need to use
>>> WRITE_ONCE().
>>>
>>
>> Thanks for pointing this out, but I'm not sure if we have to need READ_ONCE()
>> here. IIUC, if we add READ_ONCE(), we could make sure to get the latest
>> j_commit_sequence, if not, there is a window (it might becomes larger) that
>> we could get the old value and jbd2_transaction_committed() could return false
>> even if the given transaction was just committed, but I think the window is
>> always there, so it looks like it is not a big problem, is that right?
> 
> Well, all accesses to any memory should use READ_ONCE(), be protected by a
> lock, or use types that handle atomicity on assembly level (like atomic_t,
> or atomic bit operations and similar). Otherwise the compiler is free to
> assume the underlying memory cannot change and generate potentionally
> invalid code. In this case, I don't think realistically any compiler will
> do it but still it is a good practice and also it saves us from KCSAN
> warnings. If you want to know more details about possible problems, see
> 
>   tools/memory-model/Documentation/explanation.txt
> 
> chapter "PLAIN ACCESSES AND DATA RACES".
> 

Sure, this document is really helpful, I'll add READ_ONCE() and
WRITE_ONCE() here, thanks a lot.

Yi.