File operations

File operations

[Wedson Almeida Filho]

Hello,

I'm looking to implement a FileOperations trait for a driver and am
finding it cumbersome. For example, to implement read, one needs to
declare the following (copied from example):

impl file_operations::FileOperations for CycleFile {
    const READ: file_operations::ReadFn<Self> = Some(
        |_this: &Self,
         _file: &file_operations::File,
         _buf: &mut user_ptr::UserSlicePtrWriter,
         _offset: u64|
         -> KernelResult<()> {
         ...
       },
    );
}

I couldn't find a way to simplify this to just implementing the
desired functions, but I can get it down to implementing just the
desired functions plus one annotation for the impl block, for example
(note the [#build_fileops] annotation at the top):

[#build_fileops]
impl file_operations::FileOperations for CycleFile {
    fn read(
        &self,
        _file: &file_operations::File,
        _buf: &mut user_ptr::UserSlicePtrWriter,
        _offset: u64,
    ) -> KernelResult() {
        ...
    }
}

How do you all feel about this? I think it is an improvement but would
like to hear what you think before spending more time on this.

Thanks,
-Wedson

Re: File operations

[Miguel Ojeda]

On Wed, Dec 9, 2020 at 10:49 PM Wedson Almeida Filho
<wedsonaf@google.com> wrote:
>
> How do you all feel about this? I think it is an improvement but would
> like to hear what you think before spending more time on this.

Not sure... On one hand the first snippet looks very cumbersome; on
the other hand, introducing a macro for just a line may not be over
the threshold of "is this sugar worth enough?".

The macro makes the fileops functions look more normal, so I think it
would be fine, but don't spend too much time on that if you feel it
gets too convoluted: we will likely know how much macro magic people
enjoy if we end up being successful in introducing Rust in the kernel
:-)

Nevertheless, however you do it, having actual non-trivial drivers
implemented is a *very* nice thing!

Cheers,
Miguel

Re: File operations

[Wedson Almeida Filho]

On Wed, 9 Dec 2020 at 23:18, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Not sure... On one hand the first snippet looks very cumbersome; on
> the other hand, introducing a macro for just a line may not be over
> the threshold of "is this sugar worth enough?".

It will be several lines considering that we will have several of
these functions, but as you yourself mention in the next paragraph, I
don't think this is the main point of the macro, it's a reduction in
the boilerplate and making functions exactly the same as any other.

> The macro makes the fileops functions look more normal, so I think it
> would be fine, but don't spend too much time on that if you feel it
> gets too convoluted: we will likely know how much macro magic people
> enjoy if we end up being successful in introducing Rust in the kernel
> :-)

The macro would get the names of all functions implemented in a given
`impl` block and add another `impl` block with the same names as
booleans set the true. Not too complicated, but it would be better f
we could find a simpler solution.

> Nevertheless, however you do it, having actual non-trivial drivers
> implemented is a *very* nice thing!

Yes, that's what I am working towards. If the non-trivial driver is
filled with function declarations like those though, wrapped in
`Option`s and closures, I think it may be tougher sell.

Cheers,
-Wedson

Re: File operations

[Miguel Ojeda]

On Sun, Dec 13, 2020 at 1:01 AM Wedson Almeida Filho
<wedsonaf@google.com> wrote:
>
> Yes, that's what I am working towards. If the non-trivial driver is
> filled with function declarations like those though, wrapped in
> `Option`s and closures, I think it may be tougher sell.

Perhaps we should rethink how the file ops are defined altogether, so
that they are easier for clients (i.e. rather than try to minimize the
boilerplate that we have now).

For instance, could we remove the `const`s in `FileOperations` and do
it another way? e.g. give `FileOperations` all the functions with
default empty implementations? Or a builder pattern, perhaps, where we
pass functions for each?

In the worst case, if we end up with macro magic, we can go directly
to a macro that implements file ops entirely assigning things to the
vtable as needed if they are found; rather than trying to simply the
boilerplate the current `file_operations.rs` gives us:

make_file_operations! {
    fn read() {
        ...
    }
}


Cheers,
Miguel

Re: File operations

[Wedson Almeida Filho]

On Sun, 13 Dec 2020 at 00:50, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Perhaps we should rethink how the file ops are defined altogether, so
> that they are easier for clients (i.e. rather than try to minimize the
> boilerplate that we have now).

That's exactly what I'm trying to do. We're on the same page. :)

> For instance, could we remove the `const`s in `FileOperations` and do
> it another way? e.g. give `FileOperations` all the functions with
> default empty implementations? Or a builder pattern, perhaps, where we
> pass functions for each?

Yes, that's what my proposal was about. In summary, FileOperations
would have a set of functions, all with default implementations that
just fail. Then clients would just implement the functions that they
care about.

With this solution, `struct file_operations` is filled with pointers
even for the functions that are not implemented by the client. The
problem with this is that for some of the functions, the kernel
behaviour changes depending on whether the function is null or
non-null, so ideally we should only have non-null pointers in `struct
file_operations` when clients actually implement them.

So I was in search of a way to determine, ideally at compile-time, if
a function implementation was provided by the type or if it was the
default one (provided by the trait itself). I couldn't find a way. So
I thought about the macro to extract this information for us: it gets
a list of functions that *are* implemented, then generates the
implementation of another trait that is used to generate the correct
`struct file_operations` at compile time.

For example, the client would write something like:

[#build_fileops]
impl FileOperations for MyFile {
  fn open(...) { ... }
  fn ioctl(...) { ... }
  fn read(...) {... }
}

Then this would be expanded by the macro to:

/* Nothing changes the implementation. */
impl FileOperations for MyFile {
  fn open(...) { ... }
  fn ioctl(...) { ... }
  fn read(...) {... }
}

/* This is added to the token stream. */
impl FileOperationBools for MyFile {
  const OPEN: bool = true;
  const IOCTL: bool = true;
  const READ: bool = true;
}

Then our vtable-generating code would use FileOperationsBools to
determine what should be null and non-null. (Similarly to how it uses
FileOperations with Some(_) to make the same determination today.)

If the client forgets to add [#build_fileops] to their implementation,
it will fail to compile because they won't implement
FileOperationBools.

Apart from the non-trivial macro, it seems like a good solution to me.
Do you know of a simpler way to figure out whether a function
implementation is actually provided for a given type or if it's using
the default one?

> In the worst case, if we end up with macro magic, we can go directly
> to a macro that implements file ops entirely assigning things to the
> vtable as needed if they are found; rather than trying to simply the
> boilerplate the current `file_operations.rs` gives us:
>
> make_file_operations! {
>     fn read() {
>         ...
>     }
> }

This also works. However, I think what I propose above seems less
magical because it has the "impl FileOperations for XX", so the client
knows that they're implementing a trait now, and with this knowledge
they can find the definition of the trait and figure out which
functions they are allowed to implement.

Cheers,
-Wedson

Re: File operations

[Miguel Ojeda]

On Sun, Dec 13, 2020 at 3:12 AM Wedson Almeida Filho
<wedsonaf@google.com> wrote:
>
> With this solution, `struct file_operations` is filled with pointers
> even for the functions that are not implemented by the client. The
> problem with this is that for some of the functions, the kernel
> behaviour changes depending on whether the function is null or
> non-null, so ideally we should only have non-null pointers in `struct
> file_operations` when clients actually implement them.

That is the part that sounds to me like we could go with a builder pattern.

> So I was in search of a way to determine, ideally at compile-time, if
> a function implementation was provided by the type or if it was the
> default one (provided by the trait itself). I couldn't find a way. So
> I thought about the macro to extract this information for us: it gets
> a list of functions that *are* implemented, then generates the
> implementation of another trait that is used to generate the correct
> `struct file_operations` at compile time.
>
> For example, the client would write something like:

Yeah, I was trying to understand if there could be a way around
generating that code to begin with, i.e. modifying how
`file_operations.rs` works.

For instance, could the user define a type with only the methods they
need and then construct the `file_operations` struct (perhaps
abstracted the builder pattern)? The codegen/performance should be OK,
since all the values would be known. The downside is that you need to
actually repeat your functions in the builder, but the positive side
is that the code is way more straightforward, no macros, etc.

> Then our vtable-generating code would use FileOperationsBools to
> determine what should be null and non-null. (Similarly to how it uses
> FileOperations with Some(_) to make the same determination today.)
>
> If the client forgets to add [#build_fileops] to their implementation,
> it will fail to compile because they won't implement
> FileOperationBools.

Sounds good to me!

> Apart from the non-trivial macro, it seems like a good solution to me.
> Do you know of a simpler way to figure out whether a function
> implementation is actually provided for a given type or if it's using
> the default one?

I don't think there is any either, but I'm not an expert.

> This also works. However, I think what I propose above seems less
> magical because it has the "impl FileOperations for XX", so the client
> knows that they're implementing a trait now, and with this knowledge
> they can find the definition of the trait and figure out which
> functions they are allowed to implement.

You're totally right, making users see the `impl` is better.

Cheers,
Miguel

Re: File operations

[Wedson Almeida Filho]

On Sun, 13 Dec 2020 at 03:33, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:

> For instance, could the user define a type with only the methods they
> need and then construct the `file_operations` struct (perhaps
> abstracted the builder pattern)? The codegen/performance should be OK,
> since all the values would be known. The downside is that you need to
> actually repeat your functions in the builder, but the positive side
> is that the code is way more straightforward, no macros, etc.

Hmm, I hadn't thought of using the builder pattern. I need to think
about it some more.

But one potential issue that comes immediately to mind is that with a
builder pattern, the value is created at run time. In our case, this
is undesirable because this table contains function pointers: if we
leave them in writable memory, they will become easy targets when
attackers have write-vulnerabilities that they want to elevate to
arbitrary execution; leaving the vtable in rdata (i.e., in read-only
pages) is much preferable. This means that we need const functions,
but we cannot have them in traits.

If we are ok with repeating function names, I have a solution with a
much simpler macro. Of course, it is simple enough that clients can
just ignore it and define the const themselves if they want to avoid
macros at all costs. What do you think of
https://github.com/Rust-for-Linux/linux/commit/13b2f2c47e62ad5bf824f7e15e14090c90a4ba9e
?

Cheers,
-Wedson

Re: File operations

[Miguel Ojeda]

On Mon, Dec 14, 2020 at 3:03 AM Wedson Almeida Filho
<wedsonaf@google.com> wrote:
>
> But one potential issue that comes immediately to mind is that with a
> builder pattern, the value is created at run time. In our case, this
> is undesirable because this table contains function pointers: if we
> leave them in writable memory, they will become easy targets when
> attackers have write-vulnerabilities that they want to elevate to
> arbitrary execution; leaving the vtable in rdata (i.e., in read-only
> pages) is much preferable. This means that we need const functions,
> but we cannot have them in traits.

That's true, I was assuming the optimizer would realize it is a
compile-time value anyway, so that we could do it that way even
without using `const` etc.

> If we are ok with repeating function names, I have a solution with a
> much simpler macro. Of course, it is simple enough that clients can
> just ignore it and define the const themselves if they want to avoid
> macros at all costs. What do you think of
> https://github.com/Rust-for-Linux/linux/commit/13b2f2c47e62ad5bf824f7e15e14090c90a4ba9e
> ?

That looks quite good to me! It seems easy to use, and the
implementation is also easier to understand.

Cheers,
Miguel