linux-kernel - Re: [PATCH v4 3/3] block: prevent race condition on bi_status in __bio_chain

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: <20251203061403.GA16604@lst.de>
Date: Wed, 3 Dec 2025 07:14:03 +0100
From: Christoph Hellwig <hch@....de>
To: Stephen Zhang <starzhangzsd@...il.com>
Cc: Andreas Gruenbacher <agruenba@...hat.com>,
	Christoph Hellwig <hch@....de>, Johannes.Thumshirn@....com,
	ming.lei@...hat.com, hsiangkao@...ux.alibaba.com,
	csander@...estorage.com, colyli@...as.com,
	linux-block@...r.kernel.org, linux-bcache@...r.kernel.org,
	linux-kernel@...r.kernel.org, zhangshida@...inos.cn
Subject: Re: [PATCH v4 3/3] block: prevent race condition on bi_status in
 __bio_chain_endio

On Wed, Dec 03, 2025 at 11:09:36AM +0800, Stephen Zhang wrote:
> 
> I’ve been reconsidering the two approaches for the upcoming patch revision.
> Essentially, we’re comparing two methods:
> A:
>         if (bio->bi_status)
>                    parent->bi_status = bio->bi_status;
> B:
>         if (bio->bi_status)
>                 cmpxchg(&parent->bi_status, 0, bio->bi_status);
> 
> Both appear correct, but B seems a little bit redundant here.

A is not correct.  You at least needs READ_ONCE/WRITE_ONCE here.

B solves all these issues.

> Upon further reflection, I’ve noticed a subtle difference:
> A unconditionally writes to parent->bi_status when bio->bi_status is non-zero,
> regardless of the current value of parent->bi_status.
> B uses cmpxchg to only update parent->bi_status if it is still zero.
> 
> Thus, B could avoid unnecessary writes in cases where parent->bi_status has
> already been set to a non-zero value.

The unnecessary writes don't really matter, we're in an error slow path
here.