[v4,2/3] rust: block: add rnull, Rust null_blk implementation

Commit Message

Andreas Hindborg June 1, 2024, 1:40 p.m. UTC

From: Andreas Hindborg <[e-mail]>

This patch adds an initial version of the Rust null block driver.

Signed-off-by: Andreas Hindborg <[e-mail]>
---
 drivers/block/Kconfig  |  9 +++++
 drivers/block/Makefile |  3 ++
 drivers/block/rnull.rs | 81 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 drivers/block/rnull.rs

Comments

Keith Busch June 1, 2024, 2:24 p.m. UTC | #1

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?

Andreas Hindborg June 1, 2024, 3:36 p.m. UTC | #2

Keith Busch <[e-mail]> 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.

Best regards,
Andreas

Keith Busch June 1, 2024, 4:01 p.m. UTC | #3

On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
> Keith Busch <[e-mail]> 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)

Andreas Hindborg June 1, 2024, 4:59 p.m. UTC | #4

Keith Busch <[e-mail]> writes:

> On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
>> Keith Busch <[e-mail]> 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 June 1, 2024, 7:53 p.m. UTC | #5

Andreas Hindborg <[e-mail]> writes:

> Keith Busch <[e-mail]> writes:
>
>> On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
>>> Keith Busch <[e-mail]> 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

Matthew Wilcox June 2, 2024, 3:49 a.m. UTC | #6

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?

Andreas Hindborg June 2, 2024, 9:27 a.m. UTC | #7

Matthew Wilcox <[e-mail]> 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

Hannes Reinecke June 3, 2024, 9:05 a.m. UTC | #8

On 6/1/24 18:01, Keith Busch wrote:
> On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
>> Keith Busch <[e-mail]> 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

Alice Ryhl June 3, 2024, 9:06 a.m. UTC | #9

On Mon, Jun 3, 2024 at 11:05 AM Hannes Reinecke <[e-mail]> 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 <[e-mail]> 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

Andreas Hindborg June 3, 2024, 12:05 p.m. UTC | #10

Alice Ryhl <[e-mail]> writes:

> On Mon, Jun 3, 2024 at 11:05 AM Hannes Reinecke <[e-mail]> 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 <[e-mail]> 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 

Andreas Hindborg June 3, 2024, 12:07 p.m. UTC | #11

Hannes Reinecke <[e-mail]> 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 <[e-mail]> 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

Patch

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 5b9d4aaebb81..ed209f4f2798 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -354,6 +354,15 @@  config VIRTIO_BLK
 	  This is the virtual block driver for virtio.  It can be used with
           QEMU based VMMs (like KVM or Xen).  Say Y or M.
 
+config BLK_DEV_RUST_NULL
+	tristate "Rust null block driver (Experimental)"
+	depends on RUST
+	help
+	  This is the Rust implementation of the null block driver. For now it
+	  is only a minimal stub.
+
+	  If unsure, say N.
+
 config BLK_DEV_RBD
 	tristate "Rados block device (RBD)"
 	depends on INET && BLOCK
diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index 101612cba303..1105a2d4fdcb 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -9,6 +9,9 @@ 
 # needed for trace events
 ccflags-y				+= -I$(src)
 
+obj-$(CONFIG_BLK_DEV_RUST_NULL) += rnull_mod.o
+rnull_mod-y := rnull.o
+
 obj-$(CONFIG_MAC_FLOPPY)	+= swim3.o
 obj-$(CONFIG_BLK_DEV_SWIM)	+= swim_mod.o
 obj-$(CONFIG_BLK_DEV_FD)	+= floppy.o
diff --git a/drivers/block/rnull.rs b/drivers/block/rnull.rs
new file mode 100644
index 000000000000..f90808208936
--- /dev/null
+++ b/drivers/block/rnull.rs
@@ -0,0 +1,81 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! This is a Rust implementation of the C null block driver.
+//!
+//! Supported features:
+//!
+//! - blk-mq interface
+//! - direct completion
+//! - block size 4k
+//!
+//! The driver is not configurable.
+
+use kernel::{
+    alloc::flags,
+    block::mq::{
+        self,
+        gen_disk::{self, GenDisk},
+        Operations, TagSet,
+    },
+    error::Result,
+    new_mutex, pr_info,
+    prelude::*,
+    sync::{Arc, Mutex},
+    types::ARef,
+};
+
+module! {
+    type: NullBlkModule,
+    name: "rnull_mod",
+    author: "Andreas Hindborg",
+    license: "GPL v2",
+}
+
+struct NullBlkModule {
+    _disk: Pin<Box<Mutex<GenDisk<NullBlkDevice, gen_disk::Added>>>>,
+}
+
+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);
+            }
+
+            let mut disk = gen_disk::GenDisk::try_new(tagset)?;
+            disk.set_name(format_args!("rnullb{}", 0))?;
+            disk.set_capacity_sectors(4096 << 11);
+            disk.set_queue_logical_block_size(block_size.into());
+            disk.set_queue_physical_block_size(block_size.into());
+            disk.set_rotational(false);
+            disk.add()?
+        };
+
+        let disk = Box::pin_init(new_mutex!(disk, "nullb:disk"), flags::GFP_KERNEL)?;
+
+        Ok(Self { _disk: disk })
+    }
+}
+
+struct NullBlkDevice;
+
+#[vtable]
+impl Operations for NullBlkDevice {
+    #[inline(always)]
+    fn queue_rq(rq: ARef<mq::Request<Self>>, _is_last: bool) -> Result {
+        mq::Request::end_ok(rq)
+            .map_err(|_e| kernel::error::code::EIO)
+            // We take no refcounts on the request, so we expect to be able to
+            // end the request. The request reference must be uniqueue at this
+            // point, and so `end_ok` cannot fail.
+            .expect("Fatal error - expected to be able to end request");
+
+        Ok(())
+    }
+
+    fn commit_rqs() {}
+}