linux-kernel - Re: [PATCH 05/12] badblocks: return error if any badblock set fails

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: <70D2392E-4F75-43C6-8C34-498AACC78E0C@coly.li>
Date: Fri, 21 Feb 2025 18:12:14 +0800
From: Coly Li <i@...y.li>
To: Yu Kuai <yukuai1@...weicloud.com>
Cc: Zheng Qixing <zhengqixing@...weicloud.com>,
 axboe@...nel.dk,
 song@...nel.org,
 colyli@...nel.org,
 dan.j.williams@...el.com,
 vishal.l.verma@...el.com,
 dave.jiang@...el.com,
 ira.weiny@...el.com,
 dlemoal@...nel.org,
 yanjun.zhu@...ux.dev,
 kch@...dia.com,
 Hannes Reinecke <hare@...e.de>,
 zhengqixing@...wei.com,
 john.g.garry@...cle.com,
 geliang@...nel.org,
 xni@...hat.com,
 colyli@...e.de,
 linux-block@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 linux-raid@...r.kernel.org,
 nvdimm@...ts.linux.dev,
 yi.zhang@...wei.com,
 yangerkun@...wei.com,
 "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH 05/12] badblocks: return error if any badblock set fails



> 2025年2月21日 18:09，Yu Kuai <yukuai1@...weicloud.com> 写道：
> 
> Hi,
> 
> 在 2025/02/21 17:52, Coly Li 写道:
>> On Fri, Feb 21, 2025 at 04:11:02PM +0800, Zheng Qixing wrote:
>>> From: Li Nan <linan122@...wei.com>
>>> 
>>> _badblocks_set() returns success if at least one badblock is set
>>> successfully, even if others fail. This can lead to data inconsistencies
>>> in raid, where a failed badblock set should trigger the disk to be kicked
>>> out to prevent future reads from failed write areas.
>>> 
>>> _badblocks_set() should return error if any badblock set fails. Instead
>>> of relying on 'rv', directly returning 'sectors' for clearer logic. If all
>>> badblocks are successfully set, 'sectors' will be 0, otherwise it
>>> indicates the number of badblocks that have not been set yet, thus
>>> signaling failure.
>>> 
>>> By the way, it can also fix an issue: when a newly set unack badblock is
>>> included in an existing ack badblock, the setting will return an error.
>>> ···
>>>   echo "0 100" /sys/block/md0/md/dev-loop1/bad_blocks
>>>   echo "0 100" /sys/block/md0/md/dev-loop1/unacknowledged_bad_blocks
>>>   -bash: echo: write error: No space left on device
>>> ```
>>> After fix, it will return success.
>>> 
>>> Fixes: aa511ff8218b ("badblocks: switch to the improved badblock handling code")
>>> Signed-off-by: Li Nan <linan122@...wei.com>
>>> ---
>>>  block/badblocks.c | 16 ++++------------
>>>  1 file changed, 4 insertions(+), 12 deletions(-)
>>> 
>> NACK.   Such modification will break current API.
> 
> Take a look at current APIs:
> - for raid, error should be returned, otherwise data may be corrupted.
> - for nvdimm, there is only error message if fail, and it make sense as
> well if any badblocks set failed:
>        if (badblocks_set(bb, s, num, 1))
>                dev_info_once(bb->dev, "%s: failed for sector %llx\n",
>                                __func__, (u64) s);
> - for null_blk, I think it's fine as well.
> 
> Hence I think it's fine to return error if any badblocks set failed.
> There is no need to invent a new API and switch all callers to a new
> API.

So we don’t need to add a negative return value for partial success/failure?

Coly Li