search

fishinabarrel / linux-kernel-module-rust

Framework for writing Linux kernel modules in safe Rust
GNU General Public License v2.0
1.33k stars 119 forks source link

Filesystem mount #211

Closed luisgerhorst closed 4 years ago

luisgerhorst commented 4 years ago

Still WIP but I thought it's better if you see this coming. If you don't mind I'll leave this open and post/force push when it's complete.

alex commented 4 years ago

Nope, we don't mind at all! Please ping us when it's ready for review or if you have a question -- we're always excited for more contributors!

luisgerhorst commented 4 years ago

I indeed have a question: I'm currently trying to create a flexible yet safe interface to the super block's fs_info field. In C, the callback fill_super can store some private data into this field which can be used in subsequent filesystem callbacks. In put_super, the filesystem then has to free this data again when it has stored something here.

The current solution only ensures at runtime that after put_super does return, the fs_info field has been reset to None again. This has the downside that the user does only realise at runtime that he forgot to clean up fs_info and it also adds an additional check.

I've thought about maybe creating a separate UnfilledSuperBlock type and passing that to fill_super and making him return a FilledSuperBlock<Fsinfo> (which he can only get by calling some method on UnfilledSuperBlock that consumes it with his fs_info as argument), but I figured that this will not work if I only pass him a reference to the super block.

I think you have a little more Rust experience, do you guys have an idea?

luisgerhorst commented 4 years ago

Another idea which is a little related to the FilledSuperBlock idea: Encode the capabilities of the callback into the wrapper type of the super block. For example some callbacks would get SuperBlockRef and some SuperBlockRefMut passed by value (and not a SuperBlock reference which in turn contains a reference to the super_block which is maybe inefficient). Then only the latter exposes methods to modify the underlying super_block. Any ideas on that?

alex commented 4 years ago

I think the simplest implementation is to make the code in linux-kernel-module-rust responsible for clearing out fs_info in put_super, and not make the user responsible for it. This is similar to how we handle file.private_data in chrdev.rs.

luisgerhorst commented 4 years ago

Hmm I see. The reason I thought that this is not feasible is that then the callback no longer has the option to decide when/if he wants to clean up fs_info. We already introduced the requirement that fs_info is allocated on the heap (something that is not assumed anywhere in the kernel I think). The latter however can be lifted by creating IntoRaw / FromRaw traits I believe.

There's kind of a trade-off between introducing few assumptions and creating simple code. Did you already discuss this somewhere or decided on a policy? I figured it's maybe better to create wrappers that allow the Rust callbacks to do everything the C callbacks could do (however I'm not sure yet whether this is always possible).

alex commented 4 years ago

We don't have a firm-fixed policy. Thus far our focus has been on exposing functionality safely, rather than in making sure we allow every single detail that the C APIs allow.

On Thu, Nov 28, 2019 at 9:32 AM Luis Gerhorst notifications@github.com wrote:

Hmm I see. The reason I thought that this is not feasible is that then the callback no longer has the option to decide when/if he wants to clean up fs_info. We already introduced the requirement that fs_info is allocated on the heap (something that is not assumed anywhere in the kernel I think). The latter however can be lifted by creating IntoRaw / FromRaw traits I believe.

There's kind of a trade-off between introducing few assumptions and creating simple code. Did you already discuss this somewhere or decided on a policy? I figured it's maybe better to create wrappers that allow the Rust callbacks to do everything the C callbacks could do (however I'm not sure yet whether this is always possible).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/fishinabarrel/linux-kernel-module-rust/pull/211?email_source=notifications&email_token=AAAAGBGXVHJ33W3Q2TXRJ4DQV7QBZA5CNFSM4JRPFPI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFM5NBY#issuecomment-559535751, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAGBDGZGTMZ3AKIHS6KTTQV7QBZANCNFSM4JRPFPIQ .

-- All that is necessary for evil to succeed is for good people to do nothing.

luisgerhorst commented 4 years ago

That's reasonable I think. I'll try to simplify it a bit (and squash it into nice commits) so that it only introduces code needed for mounting (the other stuff can be dealt with when support for other callbacks besides put_super is added) and then it should be ready for review.