Initialize field owner in file_operations (issue #214)

[Bristlefrost1]

Fix for the issue #214

Struct ThisModule in lib.rs seems to be the equivalent of THIS_MODULE and according to this StackOverflow the owner field should be initialized to THIS_MODULE.
Just point me to the right direction if this isn't the correct way to do it. I'm pretty new to contributing to the Linux kernel.

[ojeda]

Thanks!

Please note that this is taking the tuple constructor function and using its address as the owner. The Rust equivalent would be THIS_MODULE (a per-module value), rather than ThisModule (a type).

[wedsonaf]

Please note that this is taking the tuple constructor function and using its address as the owner. The Rust equivalent would be THIS_MODULE (a per-module value), rather than ThisModule (a type).

+1

Also note that crate::THIS_MODULE is the kernel one, which is not what we want. I think you'll need to take a module as argument. The challenge is to do it safely.

[Bristlefrost1]

Take module as an argument? It's a struct, not a function that can take arguments. What part of the code are you referring to?

[wedsonaf]

Take module as an argument? It's a struct, not a function that can take arguments. What part of the code are you referring to?

This struct is a private member of FileOperationsVtable. It is exposed externally via FileOperationsVtable::build.

[Bristlefrost1]

I can't take module: *mut bindings::module as an argument for FileOperationsVtable::build because there is no way to set Self::VTABLE.owner to that. I've been staring at Google search results for hours without an answer (which is probably something very simple).

[kloenk]

As far as I can see it, with the current implementation of FileOperationsVtable this is impossible. Const and const fn is evaluated at build time. But the address of THIS_MODULE can not be known then.

A working way could be a proc_macro like I build for the rtnl_link_ops in #208 which then creates a static (mut)

[adamrk]

Could we change build to return an owned FileOperationsVtable, then mutate the owner field and pin it before passing to the C side?

[wedsonaf]

I don't have a solution for this yet, that's why I opened this bug... But the following are desirable aspects of an eventual solution:

1. The vtable is const: this is desirable for security reasons, specifically to avoid exploits that go from arbitrary write to arbitrary execution by easily changing pointers in the vtable, then triggering a code path that uses the vtable. (I know there are other ways to do this, but I don't want rust vtables to be another way of doing it.)
2. The above may not be possible today in Rust, if that's the case we should bring it up with the Rust folks to see if they can enable this for us eventually.
3. If we go the route of having a mutable vtable, this should only apply to vtables in modules. That is, if the code is built into the kernel, then THIS_MODULE is 0 and we should have a const vtable.
4. Ideally the interface should be safe: that is, we need to prevent users from passing the wrong THIS_MODULE to any function that requires this (e.g., where the code is in module A but THIS_MODULE belongs to module B).
5. If we can't achieve no 4 above, we need to make the function unsafe and annotate the requirement that it's the caller's responsibility to ensure the requirement.

(This is not a super easy task, apologies for not making it clear earlier.)

[ojeda]

Ideally the interface should be safe: that is, we need to prevent users from passing the wrong THIS_MODULE to any function that requires this (e.g., where the code is in module A but THIS_MODULE belongs to module B).

Related: #212.

[Bristlefrost1]

Just pushed some code to the branch. build now returns bindings::file_operations & vtable is mutable.

The code obviously isn't done yet.

The vtable isn't a constant and I had to remove the const keyword from the build function, as you cannot use those Some(something_callback::<T>) in constants. As @wedsonaf pointed out, it's a security risk to have a mutable vtable.

[wedsonaf]

If the code isn't done yet, please mark it as a draft so that we only spend time reviewing it when it's done.

Also, isn't the file_operations returned by build immediately destroyed (leaving you with a dangling pointer in fops)? We need to be careful with our usage of unsafe.

[Bristlefrost1]

Oops - undone

(The PR was closed until I push stuff to the branch for some reason)