Re: [PATCH 26/79] block: rnull: add badblocks support

[Andreas Hindborg]

Alice Ryhl <aliceryhl@xxxxxxxxxx> writes:

> On Mon, Feb 16, 2026 at 12:35:13AM +0100, Andreas Hindborg wrote:
>> Add badblocks support to the rnull driver with a configfs interface for
>> managing bad sectors.
>> 
>> - Configfs attribute for adding/removing bad blocks via "+start-end" and
>>   "-start-end" syntax.
>> - Request handling that checks for bad blocks and returns IO errors.
>> - Updated request completion to handle error status properly.
>> 
>> The badblocks functionality is disabled by default and is enabled when
>> first bad block is added.
>> 
>> Signed-off-by: Andreas Hindborg <a.hindborg@xxxxxxxxxx>
>
>> +    fn store(this: &DeviceConfig, page: &[u8]) -> Result {
>> +        // This attribute can be set while device is powered.
>> +
>> +        for line in core::str::from_utf8(page)?.lines() {
>> +            let mut chars = line.chars();
>> +            match chars.next() {
>> +                Some(sign @ '+' | sign @ '-') => {
>> +                    if let Some((start, end)) = chars.as_str().split_once('-') {
>> +                        let start: u64 = start.parse().map_err(|_| EINVAL)?;
>> +                        let end: u64 = end.parse().map_err(|_| EINVAL)?;
>> +
>> +                        if start > end {
>> +                            return Err(EINVAL);
>> +                        }
>> +
>> +                        this.data.lock().bad_blocks.enable();
>> +
>> +                        if sign == '+' {
>> +                            this.data.lock().bad_blocks.set_bad(start..=end, true)?;
>> +                        } else {
>> +                            this.data.lock().bad_blocks.set_good(start..=end)?;
>
> Taking lock twice: TOCTOU.

I fixed all of these, thanks for reporting.

>
>> @@ -118,6 +125,7 @@ fn make_group(
>>                      home_node: bindings::NUMA_NO_NODE,
>>                      discard: false,
>>                      no_sched: false,
>> +                    bad_blocks: Arc::pin_init(BadBlocks::new(false), GFP_KERNEL)?,
>>                  }),
>>              }),
>>              core::iter::empty(),
>
> [..]
>
>> @@ -155,6 +160,7 @@ fn init(_module: &'static ThisModule) -> impl PinInit<Self, Error> {
>>                      home_node: *module_parameters::home_node.value(),
>>                      discard: *module_parameters::discard.value() != 0,
>>                      no_sched: *module_parameters::no_sched.value() != 0,
>> +                    bad_blocks: Arc::pin_init(BadBlocks::new(false), GFP_KERNEL)?,
>
> It seems weird to construct this Arc in two places. Is it shared or not?

In the case where the device is created via configfs, the `BadBlocks`
instance is shared. When the device is constructed via module
parameters, the `BadBlocks` instance is not shared. In this case it is
actually not used, as there is no way to enable the code path. But we
need to provide an instance anyway.

I did not want to use an Option, because I did not want the checks on
access.


Best regards,
Andreas Hindborg