linux-kernel - Re: [RFC PATCH] badblocks: Improvement badblocks

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: <e58ff56f-4995-b2f6-fb0d-7b1ef8d8deb8@suse.de>
Date:   Sun, 20 Dec 2020 17:50:10 +0800
From:   Coly Li <colyli@...e.de>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     Jens Axboe <axboe@...nel.dk>,
        Vishal L Verma <vishal.l.verma@...el.com>,
        linux-block@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-raid <linux-raid@...r.kernel.org>,
        linux-nvdimm <linux-nvdimm@...ts.01.org>,
        NeilBrown <neilb@...e.de>
Subject: Re: [RFC PATCH] badblocks: Improvement badblocks_set() for handling
 multiple ranges

On 12/18/20 11:25 AM, Dan Williams wrote:
> [ add Neil, original gooodguy who wrote badblocks ]
> 
> 
> On Thu, Dec 3, 2020 at 9:16 AM Coly Li <colyli@...e.de> wrote:
>>
>> Recently I received a bug report that current badblocks code does not
>> properly handle multiple ranges. For example,
>>         badblocks_set(bb, 32, 1, true);
>>         badblocks_set(bb, 34, 1, true);
>>         badblocks_set(bb, 36, 1, true);
>>         badblocks_set(bb, 32, 12, true);
>> Then indeed badblocks_show() reports,
>>         32 3
>>         36 1
>> But the expected bad blocks table should be,
>>         32 12
>> Obviously only the first 2 ranges are merged and badblocks_set() returns
>> and ignores the rest setting range.
>>
>> This behavior is improper, if the caller of badblocks_set() wants to set
>> a range of blocks into bad blocks table, all of the blocks in the range
>> should be handled even the previous part encountering failure.
>>
>> The desired way to set bad blocks range by badblocks_set() is,
>> - Set as many as blocks in the setting range into bad blocks table.
>> - Merge the bad blocks ranges and occupy as less as slots in the bad
>>   blocks table.
>> - Fast.
>>
>> Indeed the above proposal is complicated, especially with the following
>> restrictions,
>> - The setting bad blocks range can be ackknowledged or not acknowledged.


Hi Dan,

> 
> s/ackknowledged/acknowledged/
> 
> I'd run checkpatch --codespell for future versions...

Thanks for the hint. I will do it next time.


> 
>> - The bad blocks table size is limited.
>> - Memory allocation should be avoided.
>>
>> This patch is an initial effort to improve badblocks_set() for setting
>> bad blocks range when it covers multiple already set bad ranges in the
>> bad blocks table, and to do it as fast as possible.
>>
>> The basic idea of the patch is to categorize all possible bad blocks
>> range setting combinationsinto to much less simplified and more less
>> special conditions. Inside badblocks_set() there is an implicit loop
>> composed by jumping between labels 're_insert' and 'update_sectors'. No
>> matter how large the setting bad blocks range is, in every loop just a
>> minimized range from the head is handled by a pre-defined behavior from
>> one of the categorized conditions. The logic is simple and code flow is
>> manageable.
>>
>> This patch is unfinished yet, it only improves badblocks_set() and not
>> touch badblocks_clear() and badblocks_show() yet. I post it earlier
>> because this patch will be large (more then 1000 lines of change), I
>> want more people to give me comments earlier before I go too far away.
>>
> 
> I wonder if this isn't indication that the base data structure should
> be replaced... but I have not had a chance to devote deeper thought to
> this.
> 

No existing data structure changed. Even the in-memory badblocks table I
don't change it at all. I just fix the report issue by handle more
corner cases, on-disk and in-memory stuffs are untouched and consistent.


Coly Li

> 
>> The code logic is tested as user space programmer, this patch passes
>> compiling but not tested in kernel mode yet. Right now it is only for
>> RFC purpose. I will post tested patch in further versions.
>>
>> Thank you in advance for any review or comments on this patch.
>>

[snipped]