From: Andreas Hindborg <a.hindborg@samsung.com>
This patch adds an initial version of the Rust null block driver.
Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
---
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
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() {}
+}
--
2.45.1On 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?
Keith Busch <kbusch@kernel.org> 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
On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
> Keith Busch <kbusch@kernel.org> 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)
On 6/1/24 18:01, Keith Busch wrote:
> On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
>> Keith Busch <kbusch@kernel.org> 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
Hannes Reinecke <hare@suse.de> 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 <kbusch@kernel.org> 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
On Mon, Jun 3, 2024 at 11:05 AM Hannes Reinecke <hare@suse.de> 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 <kbusch@kernel.org> 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
Alice Ryhl <aliceryhl@google.com> writes:
> On Mon, Jun 3, 2024 at 11:05 AM Hannes Reinecke <hare@suse.de> 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 <kbusch@kernel.org> 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
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 <willy@infradead.org> 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
Keith Busch <kbusch@kernel.org> writes:
> On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
>> Keith Busch <kbusch@kernel.org> 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 <nmi@metaspace.dk> writes:
> Keith Busch <kbusch@kernel.org> writes:
>
>> On Sat, Jun 01, 2024 at 05:36:20PM +0200, Andreas Hindborg wrote:
>>> Keith Busch <kbusch@kernel.org> 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
© 2016 - 2026 Red Hat, Inc.