| Message ID | 20200716124833.93667-2-sgarzare@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | io_uring: add restrictions to support untrusted applications and guests | expand |
On 16/07/2020 15:48, Stefano Garzarella wrote: > The enumeration allows us to keep track of the last > io_uring_register(2) opcode available. > > Behaviour and opcodes names don't change. > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > include/uapi/linux/io_uring.h | 27 ++++++++++++++++----------- > 1 file changed, 16 insertions(+), 11 deletions(-) > > diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h > index 7843742b8b74..efc50bd0af34 100644 > --- a/include/uapi/linux/io_uring.h > +++ b/include/uapi/linux/io_uring.h > @@ -253,17 +253,22 @@ struct io_uring_params { > /* > * io_uring_register(2) opcodes and arguments > */ > -#define IORING_REGISTER_BUFFERS 0 > -#define IORING_UNREGISTER_BUFFERS 1 > -#define IORING_REGISTER_FILES 2 > -#define IORING_UNREGISTER_FILES 3 > -#define IORING_REGISTER_EVENTFD 4 > -#define IORING_UNREGISTER_EVENTFD 5 > -#define IORING_REGISTER_FILES_UPDATE 6 > -#define IORING_REGISTER_EVENTFD_ASYNC 7 > -#define IORING_REGISTER_PROBE 8 > -#define IORING_REGISTER_PERSONALITY 9 > -#define IORING_UNREGISTER_PERSONALITY 10 > +enum { > + IORING_REGISTER_BUFFERS, > + IORING_UNREGISTER_BUFFERS, > + IORING_REGISTER_FILES, > + IORING_UNREGISTER_FILES, > + IORING_REGISTER_EVENTFD, > + IORING_UNREGISTER_EVENTFD, > + IORING_REGISTER_FILES_UPDATE, > + IORING_REGISTER_EVENTFD_ASYNC, > + IORING_REGISTER_PROBE, > + IORING_REGISTER_PERSONALITY, > + IORING_UNREGISTER_PERSONALITY, > + > + /* this goes last */ > + IORING_REGISTER_LAST > +}; It breaks userspace API. E.g. #ifdef IORING_REGISTER_BUFFERS > > struct io_uring_files_update { > __u32 offset; >
On 7/16/20 2:16 PM, Pavel Begunkov wrote: > On 16/07/2020 15:48, Stefano Garzarella wrote: >> The enumeration allows us to keep track of the last >> io_uring_register(2) opcode available. >> >> Behaviour and opcodes names don't change. >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> include/uapi/linux/io_uring.h | 27 ++++++++++++++++----------- >> 1 file changed, 16 insertions(+), 11 deletions(-) >> >> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h >> index 7843742b8b74..efc50bd0af34 100644 >> --- a/include/uapi/linux/io_uring.h >> +++ b/include/uapi/linux/io_uring.h >> @@ -253,17 +253,22 @@ struct io_uring_params { >> /* >> * io_uring_register(2) opcodes and arguments >> */ >> -#define IORING_REGISTER_BUFFERS 0 >> -#define IORING_UNREGISTER_BUFFERS 1 >> -#define IORING_REGISTER_FILES 2 >> -#define IORING_UNREGISTER_FILES 3 >> -#define IORING_REGISTER_EVENTFD 4 >> -#define IORING_UNREGISTER_EVENTFD 5 >> -#define IORING_REGISTER_FILES_UPDATE 6 >> -#define IORING_REGISTER_EVENTFD_ASYNC 7 >> -#define IORING_REGISTER_PROBE 8 >> -#define IORING_REGISTER_PERSONALITY 9 >> -#define IORING_UNREGISTER_PERSONALITY 10 >> +enum { >> + IORING_REGISTER_BUFFERS, >> + IORING_UNREGISTER_BUFFERS, >> + IORING_REGISTER_FILES, >> + IORING_UNREGISTER_FILES, >> + IORING_REGISTER_EVENTFD, >> + IORING_UNREGISTER_EVENTFD, >> + IORING_REGISTER_FILES_UPDATE, >> + IORING_REGISTER_EVENTFD_ASYNC, >> + IORING_REGISTER_PROBE, >> + IORING_REGISTER_PERSONALITY, >> + IORING_UNREGISTER_PERSONALITY, >> + >> + /* this goes last */ >> + IORING_REGISTER_LAST >> +}; > > It breaks userspace API. E.g. > > #ifdef IORING_REGISTER_BUFFERS It can, yes, but we have done that in the past. In this one, for example: commit 9e3aa61ae3e01ce1ce6361a41ef725e1f4d1d2bf (tag: io_uring-5.5-20191212) Author: Jens Axboe <axboe@kernel.dk> Date: Wed Dec 11 15:55:43 2019 -0700 io_uring: ensure we return -EINVAL on unknown opcod But it would be safer/saner to do this like we have the done the IOSQE_ flags.
On 16/07/2020 23:42, Jens Axboe wrote: > On 7/16/20 2:16 PM, Pavel Begunkov wrote: >> On 16/07/2020 15:48, Stefano Garzarella wrote: >>> The enumeration allows us to keep track of the last >>> io_uring_register(2) opcode available. >>> >>> Behaviour and opcodes names don't change. >>> >>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>> --- >>> include/uapi/linux/io_uring.h | 27 ++++++++++++++++----------- >>> 1 file changed, 16 insertions(+), 11 deletions(-) >>> >>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h >>> index 7843742b8b74..efc50bd0af34 100644 >>> --- a/include/uapi/linux/io_uring.h >>> +++ b/include/uapi/linux/io_uring.h >>> @@ -253,17 +253,22 @@ struct io_uring_params { >>> /* >>> * io_uring_register(2) opcodes and arguments >>> */ >>> -#define IORING_REGISTER_BUFFERS 0 >>> -#define IORING_UNREGISTER_BUFFERS 1 >>> -#define IORING_REGISTER_FILES 2 >>> -#define IORING_UNREGISTER_FILES 3 >>> -#define IORING_REGISTER_EVENTFD 4 >>> -#define IORING_UNREGISTER_EVENTFD 5 >>> -#define IORING_REGISTER_FILES_UPDATE 6 >>> -#define IORING_REGISTER_EVENTFD_ASYNC 7 >>> -#define IORING_REGISTER_PROBE 8 >>> -#define IORING_REGISTER_PERSONALITY 9 >>> -#define IORING_UNREGISTER_PERSONALITY 10 >>> +enum { >>> + IORING_REGISTER_BUFFERS, >>> + IORING_UNREGISTER_BUFFERS, >>> + IORING_REGISTER_FILES, >>> + IORING_UNREGISTER_FILES, >>> + IORING_REGISTER_EVENTFD, >>> + IORING_UNREGISTER_EVENTFD, >>> + IORING_REGISTER_FILES_UPDATE, >>> + IORING_REGISTER_EVENTFD_ASYNC, >>> + IORING_REGISTER_PROBE, >>> + IORING_REGISTER_PERSONALITY, >>> + IORING_UNREGISTER_PERSONALITY, >>> + >>> + /* this goes last */ >>> + IORING_REGISTER_LAST >>> +}; >> >> It breaks userspace API. E.g. >> >> #ifdef IORING_REGISTER_BUFFERS > > It can, yes, but we have done that in the past. In this one, for Ok, if nobody on the userspace side cares, then better to do that sooner than later. > example: > > commit 9e3aa61ae3e01ce1ce6361a41ef725e1f4d1d2bf (tag: io_uring-5.5-20191212) > Author: Jens Axboe <axboe@kernel.dk> > Date: Wed Dec 11 15:55:43 2019 -0700 > > io_uring: ensure we return -EINVAL on unknown opcod > > But it would be safer/saner to do this like we have the done the IOSQE_ > flags. IOSQE_ are a bitmask, but this would look peculiar enum { __IORING_REGISTER_BUFFERS, ... }; define IORING_REGISTER_BUFFERS __IORING_REGISTER_BUFFERS
On 7/16/20 2:47 PM, Pavel Begunkov wrote: > On 16/07/2020 23:42, Jens Axboe wrote: >> On 7/16/20 2:16 PM, Pavel Begunkov wrote: >>> On 16/07/2020 15:48, Stefano Garzarella wrote: >>>> The enumeration allows us to keep track of the last >>>> io_uring_register(2) opcode available. >>>> >>>> Behaviour and opcodes names don't change. >>>> >>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>>> --- >>>> include/uapi/linux/io_uring.h | 27 ++++++++++++++++----------- >>>> 1 file changed, 16 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h >>>> index 7843742b8b74..efc50bd0af34 100644 >>>> --- a/include/uapi/linux/io_uring.h >>>> +++ b/include/uapi/linux/io_uring.h >>>> @@ -253,17 +253,22 @@ struct io_uring_params { >>>> /* >>>> * io_uring_register(2) opcodes and arguments >>>> */ >>>> -#define IORING_REGISTER_BUFFERS 0 >>>> -#define IORING_UNREGISTER_BUFFERS 1 >>>> -#define IORING_REGISTER_FILES 2 >>>> -#define IORING_UNREGISTER_FILES 3 >>>> -#define IORING_REGISTER_EVENTFD 4 >>>> -#define IORING_UNREGISTER_EVENTFD 5 >>>> -#define IORING_REGISTER_FILES_UPDATE 6 >>>> -#define IORING_REGISTER_EVENTFD_ASYNC 7 >>>> -#define IORING_REGISTER_PROBE 8 >>>> -#define IORING_REGISTER_PERSONALITY 9 >>>> -#define IORING_UNREGISTER_PERSONALITY 10 >>>> +enum { >>>> + IORING_REGISTER_BUFFERS, >>>> + IORING_UNREGISTER_BUFFERS, >>>> + IORING_REGISTER_FILES, >>>> + IORING_UNREGISTER_FILES, >>>> + IORING_REGISTER_EVENTFD, >>>> + IORING_UNREGISTER_EVENTFD, >>>> + IORING_REGISTER_FILES_UPDATE, >>>> + IORING_REGISTER_EVENTFD_ASYNC, >>>> + IORING_REGISTER_PROBE, >>>> + IORING_REGISTER_PERSONALITY, >>>> + IORING_UNREGISTER_PERSONALITY, >>>> + >>>> + /* this goes last */ >>>> + IORING_REGISTER_LAST >>>> +}; >>> >>> It breaks userspace API. E.g. >>> >>> #ifdef IORING_REGISTER_BUFFERS >> >> It can, yes, but we have done that in the past. In this one, for > > Ok, if nobody on the userspace side cares, then better to do that > sooner than later. > > >> example: >> >> commit 9e3aa61ae3e01ce1ce6361a41ef725e1f4d1d2bf (tag: io_uring-5.5-20191212) >> Author: Jens Axboe <axboe@kernel.dk> >> Date: Wed Dec 11 15:55:43 2019 -0700 >> >> io_uring: ensure we return -EINVAL on unknown opcod >> >> But it would be safer/saner to do this like we have the done the IOSQE_ >> flags. > > IOSQE_ are a bitmask, but this would look peculiar > > enum { > __IORING_REGISTER_BUFFERS, > ... > }; > define IORING_REGISTER_BUFFERS __IORING_REGISTER_BUFFERS Yeah true of course, that won't really work for this case at all. That said, I don't think it's a huge deal to turn it into an enum.
On 7/16/20 2:51 PM, Jens Axboe wrote: > On 7/16/20 2:47 PM, Pavel Begunkov wrote: >> On 16/07/2020 23:42, Jens Axboe wrote: >>> On 7/16/20 2:16 PM, Pavel Begunkov wrote: >>>> On 16/07/2020 15:48, Stefano Garzarella wrote: >>>>> The enumeration allows us to keep track of the last >>>>> io_uring_register(2) opcode available. >>>>> >>>>> Behaviour and opcodes names don't change. >>>>> >>>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >>>>> --- >>>>> include/uapi/linux/io_uring.h | 27 ++++++++++++++++----------- >>>>> 1 file changed, 16 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h >>>>> index 7843742b8b74..efc50bd0af34 100644 >>>>> --- a/include/uapi/linux/io_uring.h >>>>> +++ b/include/uapi/linux/io_uring.h >>>>> @@ -253,17 +253,22 @@ struct io_uring_params { >>>>> /* >>>>> * io_uring_register(2) opcodes and arguments >>>>> */ >>>>> -#define IORING_REGISTER_BUFFERS 0 >>>>> -#define IORING_UNREGISTER_BUFFERS 1 >>>>> -#define IORING_REGISTER_FILES 2 >>>>> -#define IORING_UNREGISTER_FILES 3 >>>>> -#define IORING_REGISTER_EVENTFD 4 >>>>> -#define IORING_UNREGISTER_EVENTFD 5 >>>>> -#define IORING_REGISTER_FILES_UPDATE 6 >>>>> -#define IORING_REGISTER_EVENTFD_ASYNC 7 >>>>> -#define IORING_REGISTER_PROBE 8 >>>>> -#define IORING_REGISTER_PERSONALITY 9 >>>>> -#define IORING_UNREGISTER_PERSONALITY 10 >>>>> +enum { >>>>> + IORING_REGISTER_BUFFERS, >>>>> + IORING_UNREGISTER_BUFFERS, >>>>> + IORING_REGISTER_FILES, >>>>> + IORING_UNREGISTER_FILES, >>>>> + IORING_REGISTER_EVENTFD, >>>>> + IORING_UNREGISTER_EVENTFD, >>>>> + IORING_REGISTER_FILES_UPDATE, >>>>> + IORING_REGISTER_EVENTFD_ASYNC, >>>>> + IORING_REGISTER_PROBE, >>>>> + IORING_REGISTER_PERSONALITY, >>>>> + IORING_UNREGISTER_PERSONALITY, >>>>> + >>>>> + /* this goes last */ >>>>> + IORING_REGISTER_LAST >>>>> +}; >>>> >>>> It breaks userspace API. E.g. >>>> >>>> #ifdef IORING_REGISTER_BUFFERS >>> >>> It can, yes, but we have done that in the past. In this one, for >> >> Ok, if nobody on the userspace side cares, then better to do that >> sooner than later. I actually don't think it's a huge issue. Normally if applications do this, it's because they are using it and need it. Ala: #ifndef IORING_REGISTER_SOMETHING #define IORING_REGISTER_SOMETHING fooval #endif and that'll still work just fine, even if an identical enum is there.
On Thu, Jul 16, 2020 at 03:20:53PM -0600, Jens Axboe wrote: > On 7/16/20 2:51 PM, Jens Axboe wrote: > > On 7/16/20 2:47 PM, Pavel Begunkov wrote: > >> On 16/07/2020 23:42, Jens Axboe wrote: > >>> On 7/16/20 2:16 PM, Pavel Begunkov wrote: > >>>> On 16/07/2020 15:48, Stefano Garzarella wrote: > >>>>> The enumeration allows us to keep track of the last > >>>>> io_uring_register(2) opcode available. > >>>>> > >>>>> Behaviour and opcodes names don't change. > >>>>> > >>>>> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > >>>>> --- > >>>>> include/uapi/linux/io_uring.h | 27 ++++++++++++++++----------- > >>>>> 1 file changed, 16 insertions(+), 11 deletions(-) > >>>>> > >>>>> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h > >>>>> index 7843742b8b74..efc50bd0af34 100644 > >>>>> --- a/include/uapi/linux/io_uring.h > >>>>> +++ b/include/uapi/linux/io_uring.h > >>>>> @@ -253,17 +253,22 @@ struct io_uring_params { > >>>>> /* > >>>>> * io_uring_register(2) opcodes and arguments > >>>>> */ > >>>>> -#define IORING_REGISTER_BUFFERS 0 > >>>>> -#define IORING_UNREGISTER_BUFFERS 1 > >>>>> -#define IORING_REGISTER_FILES 2 > >>>>> -#define IORING_UNREGISTER_FILES 3 > >>>>> -#define IORING_REGISTER_EVENTFD 4 > >>>>> -#define IORING_UNREGISTER_EVENTFD 5 > >>>>> -#define IORING_REGISTER_FILES_UPDATE 6 > >>>>> -#define IORING_REGISTER_EVENTFD_ASYNC 7 > >>>>> -#define IORING_REGISTER_PROBE 8 > >>>>> -#define IORING_REGISTER_PERSONALITY 9 > >>>>> -#define IORING_UNREGISTER_PERSONALITY 10 > >>>>> +enum { > >>>>> + IORING_REGISTER_BUFFERS, > >>>>> + IORING_UNREGISTER_BUFFERS, > >>>>> + IORING_REGISTER_FILES, > >>>>> + IORING_UNREGISTER_FILES, > >>>>> + IORING_REGISTER_EVENTFD, > >>>>> + IORING_UNREGISTER_EVENTFD, > >>>>> + IORING_REGISTER_FILES_UPDATE, > >>>>> + IORING_REGISTER_EVENTFD_ASYNC, > >>>>> + IORING_REGISTER_PROBE, > >>>>> + IORING_REGISTER_PERSONALITY, > >>>>> + IORING_UNREGISTER_PERSONALITY, > >>>>> + > >>>>> + /* this goes last */ > >>>>> + IORING_REGISTER_LAST > >>>>> +}; > >>>> > >>>> It breaks userspace API. E.g. > >>>> > >>>> #ifdef IORING_REGISTER_BUFFERS > >>> > >>> It can, yes, but we have done that in the past. In this one, for > >> > >> Ok, if nobody on the userspace side cares, then better to do that > >> sooner than later. > > I actually don't think it's a huge issue. Normally if applications > do this, it's because they are using it and need it. Ala: > > #ifndef IORING_REGISTER_SOMETHING > #define IORING_REGISTER_SOMETHING fooval > #endif > > and that'll still work just fine, even if an identical enum is there. > Thank you both for the review! Then if you agree, I'll leave this patch as it is by introducing the enum. Stefano
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 7843742b8b74..efc50bd0af34 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -253,17 +253,22 @@ struct io_uring_params { /* * io_uring_register(2) opcodes and arguments */ -#define IORING_REGISTER_BUFFERS 0 -#define IORING_UNREGISTER_BUFFERS 1 -#define IORING_REGISTER_FILES 2 -#define IORING_UNREGISTER_FILES 3 -#define IORING_REGISTER_EVENTFD 4 -#define IORING_UNREGISTER_EVENTFD 5 -#define IORING_REGISTER_FILES_UPDATE 6 -#define IORING_REGISTER_EVENTFD_ASYNC 7 -#define IORING_REGISTER_PROBE 8 -#define IORING_REGISTER_PERSONALITY 9 -#define IORING_UNREGISTER_PERSONALITY 10 +enum { + IORING_REGISTER_BUFFERS, + IORING_UNREGISTER_BUFFERS, + IORING_REGISTER_FILES, + IORING_UNREGISTER_FILES, + IORING_REGISTER_EVENTFD, + IORING_UNREGISTER_EVENTFD, + IORING_REGISTER_FILES_UPDATE, + IORING_REGISTER_EVENTFD_ASYNC, + IORING_REGISTER_PROBE, + IORING_REGISTER_PERSONALITY, + IORING_UNREGISTER_PERSONALITY, + + /* this goes last */ + IORING_REGISTER_LAST +}; struct io_uring_files_update { __u32 offset;
The enumeration allows us to keep track of the last io_uring_register(2) opcode available. Behaviour and opcodes names don't change. Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> --- include/uapi/linux/io_uring.h | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-)