On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
> Keith Busch <[email protected]> writes:
>
> > On Sat, Jun 01, 2024 at 03:40:04PM +0200, Andreas Hindborg wrote:
> >> +impl kernel::Module for NullBlkModule {
> >> + fn init(_module: &'static ThisModule) -> Result<Self> {
> >> + pr_info!("Rust null_blk loaded\n");
> >> + let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
> >> +
> >> + let disk = {
> >> + let block_size: u16 = 4096;
> >> + if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
> >> + return Err(kernel::error::code::EINVAL);
> >> + }
> >
> > You've set block_size to the literal 4096, then validate its value
> > immediately after? Am I missing some way this could ever be invalid?
>
> Good catch. It is because I have a patch in the outbound queue that allows setting
> the block size via a module parameter. The module parameter patch is not
> upstream yet. Once I have that up, I will send the patch with the block
> size config.
>
> Do you think it is OK to have this redundancy? It would only be for a
> few cycles.
It's fine, just wondering why it's there. But it also allows values like
1536 and 3584, which are not valid block sizes, so I think you want the
check to be:
if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)
Keith Busch <[email protected]> writes:
> On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
>> Keith Busch <[email protected]> writes:
>>
>> > On Sat, Jun 01, 2024 at 03:40:04PM +0200, Andreas Hindborg wrote:
>> >> +impl kernel::Module for NullBlkModule {
>> >> + fn init(_module: &'static ThisModule) -> Result<Self> {
>> >> + pr_info!("Rust null_blk loaded\n");
>> >> + let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
>> >> +
>> >> + let disk = {
>> >> + let block_size: u16 = 4096;
>> >> + if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
>> >> + return Err(kernel::error::code::EINVAL);
>> >> + }
>> >
>> > You've set block_size to the literal 4096, then validate its value
>> > immediately after? Am I missing some way this could ever be invalid?
>>
>> Good catch. It is because I have a patch in the outbound queue that allows setting
>> the block size via a module parameter. The module parameter patch is not
>> upstream yet. Once I have that up, I will send the patch with the block
>> size config.
>>
>> Do you think it is OK to have this redundancy? It would only be for a
>> few cycles.
>
> It's fine, just wondering why it's there. But it also allows values like
> 1536 and 3584, which are not valid block sizes, so I think you want the
> check to be:
>
> if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)
Right, that makes sense. I modeled it after the C null_blk validation
code in `null_validate_conf`. It contains this:
dev->blocksize = round_down(dev->blocksize, 512);
dev->blocksize = clamp_t(unsigned int, dev->blocksize, 512, 4096);
That would have the same semantics, right? I guess I'll try to make a
device with a 1536 block size and see what happens.
BR Andreas
Andreas Hindborg <[email protected]> writes:
> Keith Busch <[email protected]> writes:
>
>> On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
>>> Keith Busch <[email protected]> writes:
>>>
>>> > On Sat, Jun 01, 2024 at 03:40:04PM +0200, Andreas Hindborg wrote:
>>> >> +impl kernel::Module for NullBlkModule {
>>> >> + fn init(_module: &'static ThisModule) -> Result<Self> {
>>> >> + pr_info!("Rust null_blk loaded\n");
>>> >> + let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
>>> >> +
>>> >> + let disk = {
>>> >> + let block_size: u16 = 4096;
>>> >> + if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
>>> >> + return Err(kernel::error::code::EINVAL);
>>> >> + }
>>> >
>>> > You've set block_size to the literal 4096, then validate its value
>>> > immediately after? Am I missing some way this could ever be invalid?
>>>
>>> Good catch. It is because I have a patch in the outbound queue that allows setting
>>> the block size via a module parameter. The module parameter patch is not
>>> upstream yet. Once I have that up, I will send the patch with the block
>>> size config.
>>>
>>> Do you think it is OK to have this redundancy? It would only be for a
>>> few cycles.
>>
>> It's fine, just wondering why it's there. But it also allows values like
>> 1536 and 3584, which are not valid block sizes, so I think you want the
>> check to be:
>>
>> if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)
>
> Right, that makes sense. I modeled it after the C null_blk validation
> code in `null_validate_conf`. It contains this:
>
> dev->blocksize = round_down(dev->blocksize, 512);
> dev->blocksize = clamp_t(unsigned int, dev->blocksize, 512, 4096);
>
> That would have the same semantics, right? I guess I'll try to make a
> device with a 1536 block size and see what happens.
This happens:
root@debian:~# insmod /mnt/linux-build/drivers/block/null_blk/null_blk.ko bs=1536
BUG: kernel NULL pointer dereference, address: 0000000000000000
#PF: supervisor write access in kernel mode
#PF: error_code(0x0002) - not-present page
PGD 0 P4D 0
Oops: Oops: 0002 [#1] SMP
CPU: 2 PID: 291 Comm: insmod Not tainted 6.10.0-rc1+ #839
Probably a good idea with a better check.
BR Andreas
On Sat, Jun 01, 2024 at 10:01:40AM -0600, Keith Busch wrote:
> It's fine, just wondering why it's there. But it also allows values like
> 1536 and 3584, which are not valid block sizes, so I think you want the
> check to be:
>
> if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)
I'd drop the range check. We're pretty close to landing the bs>PS
patches, so just
if block_size & block_size - 1 != 0
should be enough of a validation. Is it considered "good style" in
Rust to omit the brackets here?
Matthew Wilcox <[email protected]> writes:
> On Sat, Jun 01, 2024 at 10:01:40AM -0600, Keith Busch wrote:
>> It's fine, just wondering why it's there. But it also allows values like
>> 1536 and 3584, which are not valid block sizes, so I think you want the
>> check to be:
>>
>> if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)
>
> I'd drop the range check. We're pretty close to landing the bs>PS
> patches, so just
>
> if block_size & block_size - 1 != 0
>
> should be enough of a validation.
Is it safe to do so already? Otherwise we just remove it when it is
safe, no biggie.
> Is it considered "good style" in
> Rust to omit the brackets here?
Yes, the compiler will complain if you add parenthesis here.
```rust
fn main() {
if (true) {
return;
}
}
```
Building this will give you:
```text
warning: unnecessary parentheses around `if` condition
--> src/main.rs:2:8
|
2 | if (true) {
| ^ ^
|
= note: `#[warn(unused_parens)]` on by default
help: remove these parentheses
|
2 - if (true) {
2 + if true {
|
warning: `playground` (bin "playground") generated 1 warning (run `cargo fix --bin "playground"` to apply 1 suggestion)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.64s
Running `target/debug/playground`
```
If you omit the `{}` block after the `if` it is a syntax error.
Best regards,
Andreas
On 6/1/24 18:01, Keith Busch wrote:
> On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
>> Keith Busch <[email protected]> writes:
>>
>>> On Sat, Jun 01, 2024 at 03:40:04PM +0200, Andreas Hindborg wrote:
>>>> +impl kernel::Module for NullBlkModule {
>>>> + fn init(_module: &'static ThisModule) -> Result<Self> {
>>>> + pr_info!("Rust null_blk loaded\n");
>>>> + let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
>>>> +
>>>> + let disk = {
>>>> + let block_size: u16 = 4096;
>>>> + if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
>>>> + return Err(kernel::error::code::EINVAL);
>>>> + }
>>>
>>> You've set block_size to the literal 4096, then validate its value
>>> immediately after? Am I missing some way this could ever be invalid?
>>
>> Good catch. It is because I have a patch in the outbound queue that allows setting
>> the block size via a module parameter. The module parameter patch is not
>> upstream yet. Once I have that up, I will send the patch with the block
>> size config.
>>
>> Do you think it is OK to have this redundancy? It would only be for a
>> few cycles.
>
> It's fine, just wondering why it's there. But it also allows values like
> 1536 and 3584, which are not valid block sizes, so I think you want the
> check to be:
>
> if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)
>
Can't we overload .contains() to check only power-of-2 values?
Cheers,
Hannes
On Mon, Jun 3, 2024 at 11:05 AM Hannes Reinecke <[email protected]> wrote:
>
> On 6/1/24 18:01, Keith Busch wrote:
> > On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
> >> Keith Busch <[email protected]> writes:
> >>
> >>> On Sat, Jun 01, 2024 at 03:40:04PM +0200, Andreas Hindborg wrote:
> >>>> +impl kernel::Module for NullBlkModule {
> >>>> + fn init(_module: &'static ThisModule) -> Result<Self> {
> >>>> + pr_info!("Rust null_blk loaded\n");
> >>>> + let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
> >>>> +
> >>>> + let disk = {
> >>>> + let block_size: u16 = 4096;
> >>>> + if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
> >>>> + return Err(kernel::error::code::EINVAL);
> >>>> + }
> >>>
> >>> You've set block_size to the literal 4096, then validate its value
> >>> immediately after? Am I missing some way this could ever be invalid?
> >>
> >> Good catch. It is because I have a patch in the outbound queue that allows setting
> >> the block size via a module parameter. The module parameter patch is not
> >> upstream yet. Once I have that up, I will send the patch with the block
> >> size config.
> >>
> >> Do you think it is OK to have this redundancy? It would only be for a
> >> few cycles.
> >
> > It's fine, just wondering why it's there. But it also allows values like
> > 1536 and 3584, which are not valid block sizes, so I think you want the
> > check to be:
> >
> > if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)
> >
> Can't we overload .contains() to check only power-of-2 values?
Rust integers have a method called is_power_of_two. If you need to
assert that it's a power of two, you can use that.
Alice
Hannes Reinecke <[email protected]> writes:
> On 6/1/24 18:01, Keith Busch wrote:
>> On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
>>> Keith Busch <[email protected]> writes:
>>>
>>>> On Sat, Jun 01, 2024 at 03:40:04PM +0200, Andreas Hindborg wrote:
>>>>> +impl kernel::Module for NullBlkModule {
>>>>> + fn init(_module: &'static ThisModule) -> Result<Self> {
>>>>> + pr_info!("Rust null_blk loaded\n");
>>>>> + let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
>>>>> +
>>>>> + let disk = {
>>>>> + let block_size: u16 = 4096;
>>>>> + if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
>>>>> + return Err(kernel::error::code::EINVAL);
>>>>> + }
>>>>
>>>> You've set block_size to the literal 4096, then validate its value
>>>> immediately after? Am I missing some way this could ever be invalid?
>>>
>>> Good catch. It is because I have a patch in the outbound queue that allows setting
>>> the block size via a module parameter. The module parameter patch is not
>>> upstream yet. Once I have that up, I will send the patch with the block
>>> size config.
>>>
>>> Do you think it is OK to have this redundancy? It would only be for a
>>> few cycles.
>> It's fine, just wondering why it's there. But it also allows values like
>> 1536 and 3584, which are not valid block sizes, so I think you want the
>> check to be:
>> if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1))
>> != 0)
>>
> Can't we overload .contains() to check only power-of-2 values?
I think `contains` just compiles down to a simple bounds check. We have
to do both the bounds check and the power-of-2 check either way.
BR Andreas
Alice Ryhl <[email protected]> writes:
> On Mon, Jun 3, 2024 at 11:05 AM Hannes Reinecke <[email protected]> wrote:
>>
>> On 6/1/24 18:01, Keith Busch wrote:
>> > On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
>> >> Keith Busch <[email protected]> writes:
>> >>
>> >>> On Sat, Jun 01, 2024 at 03:40:04PM +0200, Andreas Hindborg wrote:
>> >>>> +impl kernel::Module for NullBlkModule {
>> >>>> + fn init(_module: &'static ThisModule) -> Result<Self> {
>> >>>> + pr_info!("Rust null_blk loaded\n");
>> >>>> + let tagset = Arc::pin_init(TagSet::try_new(1, 256, 1), flags::GFP_KERNEL)?;
>> >>>> +
>> >>>> + let disk = {
>> >>>> + let block_size: u16 = 4096;
>> >>>> + if block_size % 512 != 0 || !(512..=4096).contains(&block_size) {
>> >>>> + return Err(kernel::error::code::EINVAL);
>> >>>> + }
>> >>>
>> >>> You've set block_size to the literal 4096, then validate its value
>> >>> immediately after? Am I missing some way this could ever be invalid?
>> >>
>> >> Good catch. It is because I have a patch in the outbound queue that allows setting
>> >> the block size via a module parameter. The module parameter patch is not
>> >> upstream yet. Once I have that up, I will send the patch with the block
>> >> size config.
>> >>
>> >> Do you think it is OK to have this redundancy? It would only be for a
>> >> few cycles.
>> >
>> > It's fine, just wondering why it's there. But it also allows values like
>> > 1536 and 3584, which are not valid block sizes, so I think you want the
>> > check to be:
>> >
>> > if !(512..=4096).contains(&block_size) || ((block_size & (block_size - 1)) != 0)
>> >
>> Can't we overload .contains() to check only power-of-2 values?
>
> Rust integers have a method called is_power_of_two. If you need to
> assert that it's a power of two, you can use that.
Thanks Alice, that is much easier to read ????
BR Andreas