diff mbox series

[3/3] /dev/null: add IORING_OP_URING_CMD support

Message ID 166120327984.369593.8371751426301540450.stgit@olly (mailing list archive)
State New
Headers show
Series LSM hooks for IORING_OP_URING_CMD | expand

Commit Message

Paul Moore Aug. 22, 2022, 9:21 p.m. UTC
This patch adds support for the io_uring command pass through, aka
IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
/dev/null functionality, the implementation is just a simple sink
where commands go to die, but it should be useful for developers who
need a simple IORING_OP_URING_CMD test device that doesn't require
any special hardware.

Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 drivers/char/mem.c |    6 ++++++
 1 file changed, 6 insertions(+)

Comments

Jens Axboe Aug. 22, 2022, 10:36 p.m. UTC | #1
On 8/22/22 3:21 PM, Paul Moore wrote:
> This patch adds support for the io_uring command pass through, aka
> IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
> /dev/null functionality, the implementation is just a simple sink
> where commands go to die, but it should be useful for developers who
> need a simple IORING_OP_URING_CMD test device that doesn't require
> any special hardware.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  drivers/char/mem.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 84ca98ed1dad..32a932a065a6 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -480,6 +480,11 @@ static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
>  	return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
>  }
>  
> +static int uring_cmd_null(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
> +{
> +	return 0;
> +}

This would be better as:

	return IOU_OK;

using the proper return values for the uring_cmd hook. With that:

Acked-by: Jens Axboe <axboe@kernel.dk>
Paul Moore Aug. 22, 2022, 11:09 p.m. UTC | #2
On Mon, Aug 22, 2022 at 6:36 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 8/22/22 3:21 PM, Paul Moore wrote:
> > This patch adds support for the io_uring command pass through, aka
> > IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
> > /dev/null functionality, the implementation is just a simple sink
> > where commands go to die, but it should be useful for developers who
> > need a simple IORING_OP_URING_CMD test device that doesn't require
> > any special hardware.
> >
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
> >  drivers/char/mem.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> > index 84ca98ed1dad..32a932a065a6 100644
> > --- a/drivers/char/mem.c
> > +++ b/drivers/char/mem.c
> > @@ -480,6 +480,11 @@ static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
> >       return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
> >  }
> >
> > +static int uring_cmd_null(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
> > +{
> > +     return 0;
> > +}
>
> This would be better as:
>
>         return IOU_OK;
>
> using the proper return values for the uring_cmd hook.

The only problem I see with that is that IOU_OK is defined under
io_uring/io_uring.h and not include/linux/io_uring.h so the #include
macro is kinda ugly:

  #include "../../io_uring/io_uring.h"

I'm not sure I want to submit that upstream looking like that.  Are
you okay with leaving the return code as 0 for now and changing it at
a later date?  I'm trying to keep this patchset relatively small since
we are in the -rcX stage, but if you're okay with a simple cut-n-paste
of the enum to linux/io_uring.h I can do that.

> With that:
>
> Acked-by: Jens Axboe <axboe@kernel.dk>
Jens Axboe Aug. 22, 2022, 11:13 p.m. UTC | #3
On 8/22/22 5:09 PM, Paul Moore wrote:
> On Mon, Aug 22, 2022 at 6:36 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 8/22/22 3:21 PM, Paul Moore wrote:
>>> This patch adds support for the io_uring command pass through, aka
>>> IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
>>> /dev/null functionality, the implementation is just a simple sink
>>> where commands go to die, but it should be useful for developers who
>>> need a simple IORING_OP_URING_CMD test device that doesn't require
>>> any special hardware.
>>>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>>> ---
>>>  drivers/char/mem.c |    6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
>>> index 84ca98ed1dad..32a932a065a6 100644
>>> --- a/drivers/char/mem.c
>>> +++ b/drivers/char/mem.c
>>> @@ -480,6 +480,11 @@ static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
>>>       return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
>>>  }
>>>
>>> +static int uring_cmd_null(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
>>> +{
>>> +     return 0;
>>> +}
>>
>> This would be better as:
>>
>>         return IOU_OK;
>>
>> using the proper return values for the uring_cmd hook.
> 
> The only problem I see with that is that IOU_OK is defined under
> io_uring/io_uring.h and not include/linux/io_uring.h so the #include
> macro is kinda ugly:
> 
>   #include "../../io_uring/io_uring.h"
> 
> I'm not sure I want to submit that upstream looking like that.  Are
> you okay with leaving the return code as 0 for now and changing it at
> a later date?  I'm trying to keep this patchset relatively small since
> we are in the -rcX stage, but if you're okay with a simple cut-n-paste
> of the enum to linux/io_uring.h I can do that.

Ugh yes, that should move into the general domain. Yeah I'm fine with it
as it is, we can fix that up (and them nvme as well) at a later point.
Paul Moore Aug. 22, 2022, 11:19 p.m. UTC | #4
On Mon, Aug 22, 2022 at 7:13 PM Jens Axboe <axboe@kernel.dk> wrote:
> On 8/22/22 5:09 PM, Paul Moore wrote:
> > On Mon, Aug 22, 2022 at 6:36 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 8/22/22 3:21 PM, Paul Moore wrote:
> >>> This patch adds support for the io_uring command pass through, aka
> >>> IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
> >>> /dev/null functionality, the implementation is just a simple sink
> >>> where commands go to die, but it should be useful for developers who
> >>> need a simple IORING_OP_URING_CMD test device that doesn't require
> >>> any special hardware.
> >>>
> >>> Cc: Arnd Bergmann <arnd@arndb.de>
> >>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>> Signed-off-by: Paul Moore <paul@paul-moore.com>
> >>> ---
> >>>  drivers/char/mem.c |    6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> >>> index 84ca98ed1dad..32a932a065a6 100644
> >>> --- a/drivers/char/mem.c
> >>> +++ b/drivers/char/mem.c
> >>> @@ -480,6 +480,11 @@ static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
> >>>       return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
> >>>  }
> >>>
> >>> +static int uring_cmd_null(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
> >>> +{
> >>> +     return 0;
> >>> +}
> >>
> >> This would be better as:
> >>
> >>         return IOU_OK;
> >>
> >> using the proper return values for the uring_cmd hook.
> >
> > The only problem I see with that is that IOU_OK is defined under
> > io_uring/io_uring.h and not include/linux/io_uring.h so the #include
> > macro is kinda ugly:
> >
> >   #include "../../io_uring/io_uring.h"
> >
> > I'm not sure I want to submit that upstream looking like that.  Are
> > you okay with leaving the return code as 0 for now and changing it at
> > a later date?  I'm trying to keep this patchset relatively small since
> > we are in the -rcX stage, but if you're okay with a simple cut-n-paste
> > of the enum to linux/io_uring.h I can do that.
>
> Ugh yes, that should move into the general domain. Yeah I'm fine with it
> as it is, we can fix that up (and them nvme as well) at a later point.

Okay, sounds good, I'll leave it as-is.  Is it okay to still add your ACK?
Jens Axboe Aug. 22, 2022, 11:25 p.m. UTC | #5
On 8/22/22 5:19 PM, Paul Moore wrote:
> On Mon, Aug 22, 2022 at 7:13 PM Jens Axboe <axboe@kernel.dk> wrote:
>> On 8/22/22 5:09 PM, Paul Moore wrote:
>>> On Mon, Aug 22, 2022 at 6:36 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> On 8/22/22 3:21 PM, Paul Moore wrote:
>>>>> This patch adds support for the io_uring command pass through, aka
>>>>> IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
>>>>> /dev/null functionality, the implementation is just a simple sink
>>>>> where commands go to die, but it should be useful for developers who
>>>>> need a simple IORING_OP_URING_CMD test device that doesn't require
>>>>> any special hardware.
>>>>>
>>>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>>>>> ---
>>>>>  drivers/char/mem.c |    6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
>>>>> index 84ca98ed1dad..32a932a065a6 100644
>>>>> --- a/drivers/char/mem.c
>>>>> +++ b/drivers/char/mem.c
>>>>> @@ -480,6 +480,11 @@ static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
>>>>>       return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
>>>>>  }
>>>>>
>>>>> +static int uring_cmd_null(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
>>>>> +{
>>>>> +     return 0;
>>>>> +}
>>>>
>>>> This would be better as:
>>>>
>>>>         return IOU_OK;
>>>>
>>>> using the proper return values for the uring_cmd hook.
>>>
>>> The only problem I see with that is that IOU_OK is defined under
>>> io_uring/io_uring.h and not include/linux/io_uring.h so the #include
>>> macro is kinda ugly:
>>>
>>>   #include "../../io_uring/io_uring.h"
>>>
>>> I'm not sure I want to submit that upstream looking like that.  Are
>>> you okay with leaving the return code as 0 for now and changing it at
>>> a later date?  I'm trying to keep this patchset relatively small since
>>> we are in the -rcX stage, but if you're okay with a simple cut-n-paste
>>> of the enum to linux/io_uring.h I can do that.
>>
>> Ugh yes, that should move into the general domain. Yeah I'm fine with it
>> as it is, we can fix that up (and them nvme as well) at a later point.
> 
> Okay, sounds good, I'll leave it as-is.  Is it okay to still add your ACK?

Yep, all things considered, for 6.0 I think that's the way to go.
Paul Moore Aug. 22, 2022, 11:37 p.m. UTC | #6
On Mon, Aug 22, 2022 at 7:25 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 8/22/22 5:19 PM, Paul Moore wrote:
> > On Mon, Aug 22, 2022 at 7:13 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> On 8/22/22 5:09 PM, Paul Moore wrote:
> >>> On Mon, Aug 22, 2022 at 6:36 PM Jens Axboe <axboe@kernel.dk> wrote:
> >>>> On 8/22/22 3:21 PM, Paul Moore wrote:
> >>>>> This patch adds support for the io_uring command pass through, aka
> >>>>> IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
> >>>>> /dev/null functionality, the implementation is just a simple sink
> >>>>> where commands go to die, but it should be useful for developers who
> >>>>> need a simple IORING_OP_URING_CMD test device that doesn't require
> >>>>> any special hardware.
> >>>>>
> >>>>> Cc: Arnd Bergmann <arnd@arndb.de>
> >>>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >>>>> Signed-off-by: Paul Moore <paul@paul-moore.com>
> >>>>> ---
> >>>>>  drivers/char/mem.c |    6 ++++++
> >>>>>  1 file changed, 6 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> >>>>> index 84ca98ed1dad..32a932a065a6 100644
> >>>>> --- a/drivers/char/mem.c
> >>>>> +++ b/drivers/char/mem.c
> >>>>> @@ -480,6 +480,11 @@ static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
> >>>>>       return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
> >>>>>  }
> >>>>>
> >>>>> +static int uring_cmd_null(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
> >>>>> +{
> >>>>> +     return 0;
> >>>>> +}
> >>>>
> >>>> This would be better as:
> >>>>
> >>>>         return IOU_OK;
> >>>>
> >>>> using the proper return values for the uring_cmd hook.
> >>>
> >>> The only problem I see with that is that IOU_OK is defined under
> >>> io_uring/io_uring.h and not include/linux/io_uring.h so the #include
> >>> macro is kinda ugly:
> >>>
> >>>   #include "../../io_uring/io_uring.h"
> >>>
> >>> I'm not sure I want to submit that upstream looking like that.  Are
> >>> you okay with leaving the return code as 0 for now and changing it at
> >>> a later date?  I'm trying to keep this patchset relatively small since
> >>> we are in the -rcX stage, but if you're okay with a simple cut-n-paste
> >>> of the enum to linux/io_uring.h I can do that.
> >>
> >> Ugh yes, that should move into the general domain. Yeah I'm fine with it
> >> as it is, we can fix that up (and them nvme as well) at a later point.
> >
> > Okay, sounds good, I'll leave it as-is.  Is it okay to still add your ACK?
>
> Yep, all things considered, for 6.0 I think that's the way to go.

Great, thanks.
Greg KH Aug. 23, 2022, 6:51 a.m. UTC | #7
On Mon, Aug 22, 2022 at 05:21:19PM -0400, Paul Moore wrote:
> This patch adds support for the io_uring command pass through, aka
> IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
> /dev/null functionality, the implementation is just a simple sink
> where commands go to die, but it should be useful for developers who
> need a simple IORING_OP_URING_CMD test device that doesn't require
> any special hardware.
> 
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  drivers/char/mem.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 84ca98ed1dad..32a932a065a6 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -480,6 +480,11 @@ static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
>  	return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
>  }
>  
> +static int uring_cmd_null(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
> +{
> +	return 0;

If a callback just returns 0, that implies it is not needed at all and
can be removed and then you are back at the original file before your
commit :)

thanks,

greg k-h
Greg KH Aug. 23, 2022, 6:52 a.m. UTC | #8
On Mon, Aug 22, 2022 at 05:21:19PM -0400, Paul Moore wrote:
> This patch adds support for the io_uring command pass through, aka
> IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
> /dev/null functionality, the implementation is just a simple sink
> where commands go to die, but it should be useful for developers who
> need a simple IORING_OP_URING_CMD test device that doesn't require
> any special hardware.

Also, shouldn't you document this somewhere?

At least in the code itself saying "this is here so that /dev/null works
as a io_uring sink" or something like that?  Otherwise it just looks
like it does nothing at all.

thanks,

greg k-h
Jens Axboe Aug. 23, 2022, 1:33 p.m. UTC | #9
On 8/23/22 12:51 AM, Greg Kroah-Hartman wrote:
> On Mon, Aug 22, 2022 at 05:21:19PM -0400, Paul Moore wrote:
>> This patch adds support for the io_uring command pass through, aka
>> IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
>> /dev/null functionality, the implementation is just a simple sink
>> where commands go to die, but it should be useful for developers who
>> need a simple IORING_OP_URING_CMD test device that doesn't require
>> any special hardware.
>>
>> Cc: Arnd Bergmann <arnd@arndb.de>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Paul Moore <paul@paul-moore.com>
>> ---
>>  drivers/char/mem.c |    6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
>> index 84ca98ed1dad..32a932a065a6 100644
>> --- a/drivers/char/mem.c
>> +++ b/drivers/char/mem.c
>> @@ -480,6 +480,11 @@ static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
>>  	return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
>>  }
>>  
>> +static int uring_cmd_null(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
>> +{
>> +	return 0;
> 
> If a callback just returns 0, that implies it is not needed at all and
> can be removed and then you are back at the original file before your
> commit :)

In theory you are correct, but the empty hook is needed so that
submitting an io_uring cmd to the file type is attempted. If not it's
just errored upfront.

Paul, is it strictly needed to test the selinux uring cmd policy? If the
operation would've been attempted but null doesn't support it, you'd get
-1/EOPNOTSUPP - and supposedly you'd get EACCES/EPERM or something if
it's filtered?
Paul Moore Aug. 23, 2022, 5:02 p.m. UTC | #10
On Tue, Aug 23, 2022 at 2:52 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Aug 22, 2022 at 05:21:19PM -0400, Paul Moore wrote:
> > This patch adds support for the io_uring command pass through, aka
> > IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
> > /dev/null functionality, the implementation is just a simple sink
> > where commands go to die, but it should be useful for developers who
> > need a simple IORING_OP_URING_CMD test device that doesn't require
> > any special hardware.
>
> Also, shouldn't you document this somewhere?
>
> At least in the code itself saying "this is here so that /dev/null works
> as a io_uring sink" or something like that?  Otherwise it just looks
> like it does nothing at all.

What about read_null() and write_null()?  I can definitely add a
comment (there is no /dev/null documentation in the kernel source tree
that I can see), but there is clearly precedence for /dev/null having
"do nothing" file_operations functions.  If nothing else, it's pretty
much in the *name*; we're adding the "uring_cmd_null()" member
function to the "null_fops" struct for the "null" device ... come on
Greg :)
Paul Moore Aug. 23, 2022, 5:02 p.m. UTC | #11
On Tue, Aug 23, 2022 at 9:33 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 8/23/22 12:51 AM, Greg Kroah-Hartman wrote:
> > On Mon, Aug 22, 2022 at 05:21:19PM -0400, Paul Moore wrote:
> >> This patch adds support for the io_uring command pass through, aka
> >> IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
> >> /dev/null functionality, the implementation is just a simple sink
> >> where commands go to die, but it should be useful for developers who
> >> need a simple IORING_OP_URING_CMD test device that doesn't require
> >> any special hardware.
> >>
> >> Cc: Arnd Bergmann <arnd@arndb.de>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Signed-off-by: Paul Moore <paul@paul-moore.com>
> >> ---
> >>  drivers/char/mem.c |    6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> >> index 84ca98ed1dad..32a932a065a6 100644
> >> --- a/drivers/char/mem.c
> >> +++ b/drivers/char/mem.c
> >> @@ -480,6 +480,11 @@ static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
> >>      return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
> >>  }
> >>
> >> +static int uring_cmd_null(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
> >> +{
> >> +    return 0;
> >
> > If a callback just returns 0, that implies it is not needed at all and
> > can be removed and then you are back at the original file before your
> > commit :)
>
> In theory you are correct, but the empty hook is needed so that
> submitting an io_uring cmd to the file type is attempted. If not it's
> just errored upfront.
>
> Paul, is it strictly needed to test the selinux uring cmd policy? If the
> operation would've been attempted but null doesn't support it, you'd get
> -1/EOPNOTSUPP - and supposedly you'd get EACCES/EPERM or something if
> it's filtered?

I haven't built a kernel without this patch to test, but yes, you are
probably correct that it wouldn't be strictly necessary, but
considering the extreme simplicity of this addition, what is the real
harm here?  Wouldn't it be nice to have a rather simple
IORING_OP_URING_CMD target?

Also, just so we are clear, I didn't mark this patch with the
stable/fixes tags because I don't believe this should go into -stable;
while I believe it is a nice addition, it is definitely not critical.
Greg KH Aug. 24, 2022, 6:10 a.m. UTC | #12
On Tue, Aug 23, 2022 at 01:02:08PM -0400, Paul Moore wrote:
> On Tue, Aug 23, 2022 at 2:52 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Aug 22, 2022 at 05:21:19PM -0400, Paul Moore wrote:
> > > This patch adds support for the io_uring command pass through, aka
> > > IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
> > > /dev/null functionality, the implementation is just a simple sink
> > > where commands go to die, but it should be useful for developers who
> > > need a simple IORING_OP_URING_CMD test device that doesn't require
> > > any special hardware.
> >
> > Also, shouldn't you document this somewhere?
> >
> > At least in the code itself saying "this is here so that /dev/null works
> > as a io_uring sink" or something like that?  Otherwise it just looks
> > like it does nothing at all.
> 
> What about read_null() and write_null()?  I can definitely add a
> comment (there is no /dev/null documentation in the kernel source tree
> that I can see), but there is clearly precedence for /dev/null having
> "do nothing" file_operations functions.

Yes, they should "do nothing".  write_null() does report that it
consumed everything, why doesn't this function have to also do that?

thanks,

greg k-h
Paul Moore Aug. 24, 2022, 2:06 p.m. UTC | #13
On Wed, Aug 24, 2022 at 2:10 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Aug 23, 2022 at 01:02:08PM -0400, Paul Moore wrote:
> > On Tue, Aug 23, 2022 at 2:52 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Mon, Aug 22, 2022 at 05:21:19PM -0400, Paul Moore wrote:
> > > > This patch adds support for the io_uring command pass through, aka
> > > > IORING_OP_URING_CMD, to the /dev/null driver.  As with all of the
> > > > /dev/null functionality, the implementation is just a simple sink
> > > > where commands go to die, but it should be useful for developers who
> > > > need a simple IORING_OP_URING_CMD test device that doesn't require
> > > > any special hardware.
> > >
> > > Also, shouldn't you document this somewhere?
> > >
> > > At least in the code itself saying "this is here so that /dev/null works
> > > as a io_uring sink" or something like that?  Otherwise it just looks
> > > like it does nothing at all.
> >
> > What about read_null() and write_null()?  I can definitely add a
> > comment (there is no /dev/null documentation in the kernel source tree
> > that I can see), but there is clearly precedence for /dev/null having
> > "do nothing" file_operations functions.
>
> Yes, they should "do nothing".

Right, I don't think anyone was disputing that.  You were asking for a
comment for the new function that effectively says "this function does
nothing", which seems a little silly given the simplicity of the
function, the name, and the context of it all.

> write_null() does report that it
> consumed everything, why doesn't this function have to also do that?

Because a file write (file_operations->write) and a
IORING_OP_URING_CMD (file_operations->uring_cmd) are fundamentally
different operations; uring_cmd_null() returns 0, which is the success
return code for this file op (not to mention a significant number of
kernel functions that return an int).
diff mbox series

Patch

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 84ca98ed1dad..32a932a065a6 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -480,6 +480,11 @@  static ssize_t splice_write_null(struct pipe_inode_info *pipe, struct file *out,
 	return splice_from_pipe(pipe, out, ppos, len, flags, pipe_to_null);
 }
 
+static int uring_cmd_null(struct io_uring_cmd *ioucmd, unsigned int issue_flags)
+{
+	return 0;
+}
+
 static ssize_t read_iter_zero(struct kiocb *iocb, struct iov_iter *iter)
 {
 	size_t written = 0;
@@ -663,6 +668,7 @@  static const struct file_operations null_fops = {
 	.read_iter	= read_iter_null,
 	.write_iter	= write_iter_null,
 	.splice_write	= splice_write_null,
+	.uring_cmd	= uring_cmd_null,
 };
 
 static const struct file_operations __maybe_unused port_fops = {