linux-api.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* io_uring vs in_compat_syscall()
@ 2020-07-20  6:10 Christoph Hellwig
  2020-07-20 16:36 ` Jens Axboe
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2020-07-20  6:10 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-arch, linux-api, linux-kernel, io-uring

Hi Jens,

I just found a (so far theoretical) issue with the io_uring submission
offloading to workqueues or threads.  We have lots of places using
in_compat_syscall() to check if a syscall needs compat treatmenet.
While the biggest users is iocttl(), we also have a fair amount of
places using in_compat_task() in read and write methods, and these
will not do the wrong thing when used with io_uring under certain
conditions.  I'm not sure how to best fix this, except for making sure
in_compat_syscall() returns true one way or another for these cases.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: io_uring vs in_compat_syscall()
  2020-07-20  6:10 io_uring vs in_compat_syscall() Christoph Hellwig
@ 2020-07-20 16:36 ` Jens Axboe
  2020-07-20 16:58   ` Andy Lutomirski
  0 siblings, 1 reply; 11+ messages in thread
From: Jens Axboe @ 2020-07-20 16:36 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-arch, linux-api, linux-kernel, io-uring

On 7/20/20 12:10 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> I just found a (so far theoretical) issue with the io_uring submission
> offloading to workqueues or threads.  We have lots of places using
> in_compat_syscall() to check if a syscall needs compat treatmenet.
> While the biggest users is iocttl(), we also have a fair amount of
> places using in_compat_task() in read and write methods, and these
> will not do the wrong thing when used with io_uring under certain
> conditions.  I'm not sure how to best fix this, except for making sure
> in_compat_syscall() returns true one way or another for these cases.

We can probably propagate this information in the io_kiocb via a flag,
and have the io-wq worker set TS_COMPAT if that's the case.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: io_uring vs in_compat_syscall()
  2020-07-20 16:36 ` Jens Axboe
@ 2020-07-20 16:58   ` Andy Lutomirski
  2020-07-20 17:02     ` Jens Axboe
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Lutomirski @ 2020-07-20 16:58 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Christoph Hellwig, linux-arch, linux-api, linux-kernel, io-uring


> On Jul 20, 2020, at 9:37 AM, Jens Axboe <axboe@kernel.dk> wrote:
> 
> On 7/20/20 12:10 AM, Christoph Hellwig wrote:
>> Hi Jens,
>> 
>> I just found a (so far theoretical) issue with the io_uring submission
>> offloading to workqueues or threads.  We have lots of places using
>> in_compat_syscall() to check if a syscall needs compat treatmenet.
>> While the biggest users is iocttl(), we also have a fair amount of
>> places using in_compat_task() in read and write methods, and these
>> will not do the wrong thing when used with io_uring under certain
>> conditions.  I'm not sure how to best fix this, except for making sure
>> in_compat_syscall() returns true one way or another for these cases.
> 
> We can probably propagate this information in the io_kiocb via a flag,
> and have the io-wq worker set TS_COMPAT if that's the case.
> 

Is TS_COMPAT actually a cross-arch concept for which this is safe?  Having a real arch helper for “set the current syscall arch for the current kernel thread” seems more sensible to me. 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: io_uring vs in_compat_syscall()
  2020-07-20 16:58   ` Andy Lutomirski
@ 2020-07-20 17:02     ` Jens Axboe
  2020-07-20 17:28       ` Andy Lutomirski
  0 siblings, 1 reply; 11+ messages in thread
From: Jens Axboe @ 2020-07-20 17:02 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Christoph Hellwig, linux-arch, linux-api, linux-kernel, io-uring

On 7/20/20 10:58 AM, Andy Lutomirski wrote:
> 
>> On Jul 20, 2020, at 9:37 AM, Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 7/20/20 12:10 AM, Christoph Hellwig wrote:
>>> Hi Jens,
>>>
>>> I just found a (so far theoretical) issue with the io_uring submission
>>> offloading to workqueues or threads.  We have lots of places using
>>> in_compat_syscall() to check if a syscall needs compat treatmenet.
>>> While the biggest users is iocttl(), we also have a fair amount of
>>> places using in_compat_task() in read and write methods, and these
>>> will not do the wrong thing when used with io_uring under certain
>>> conditions.  I'm not sure how to best fix this, except for making sure
>>> in_compat_syscall() returns true one way or another for these cases.
>>
>> We can probably propagate this information in the io_kiocb via a flag,
>> and have the io-wq worker set TS_COMPAT if that's the case.
>>
> 
> Is TS_COMPAT actually a cross-arch concept for which this is safe?
> Having a real arch helper for “set the current syscall arch for the
> current kernel thread” seems more sensible to me. 

Sure, I'd consider that implementation detail for the actual patch(es)
for this issue.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: io_uring vs in_compat_syscall()
  2020-07-20 17:02     ` Jens Axboe
@ 2020-07-20 17:28       ` Andy Lutomirski
  2020-07-21  7:07         ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Lutomirski @ 2020-07-20 17:28 UTC (permalink / raw)
  To: Jens Axboe
  Cc: Christoph Hellwig, linux-arch, linux-api, linux-kernel, io-uring



> On Jul 20, 2020, at 10:02 AM, Jens Axboe <axboe@kernel.dk> wrote:
> 
> On 7/20/20 10:58 AM, Andy Lutomirski wrote:
>> 
>>>> On Jul 20, 2020, at 9:37 AM, Jens Axboe <axboe@kernel.dk> wrote:
>>> 
>>> On 7/20/20 12:10 AM, Christoph Hellwig wrote:
>>>> Hi Jens,
>>>> 
>>>> I just found a (so far theoretical) issue with the io_uring submission
>>>> offloading to workqueues or threads.  We have lots of places using
>>>> in_compat_syscall() to check if a syscall needs compat treatmenet.
>>>> While the biggest users is iocttl(), we also have a fair amount of
>>>> places using in_compat_task() in read and write methods, and these
>>>> will not do the wrong thing when used with io_uring under certain
>>>> conditions.  I'm not sure how to best fix this, except for making sure
>>>> in_compat_syscall() returns true one way or another for these cases.
>>> 
>>> We can probably propagate this information in the io_kiocb via a flag,
>>> and have the io-wq worker set TS_COMPAT if that's the case.
>>> 
>> 
>> Is TS_COMPAT actually a cross-arch concept for which this is safe?
>> Having a real arch helper for “set the current syscall arch for the
>> current kernel thread” seems more sensible to me. 
> 
> Sure, I'd consider that implementation detail for the actual patch(es)
> for this issue.

There’s a corner case, though: doesn’t io_uring submission frequently do the work synchronously in the context of the calling thread?  If so, can a thread do a 64-bit submit with 32-bit work or vice versa?

Sometimes I think that in_compat_syscall() should have a mode in which calling it warns (e.g. not actually in a syscall when doing things in io_uring).  And the relevant operations should be properly wired up to avoid global state like this.

> 
> -- 
> Jens Axboe
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: io_uring vs in_compat_syscall()
  2020-07-20 17:28       ` Andy Lutomirski
@ 2020-07-21  7:07         ` Christoph Hellwig
  2020-07-21 14:31           ` Andy Lutomirski
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2020-07-21  7:07 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Jens Axboe, Christoph Hellwig, linux-arch, linux-api,
	linux-kernel, io-uring

On Mon, Jul 20, 2020 at 10:28:55AM -0700, Andy Lutomirski wrote:
> > Sure, I'd consider that implementation detail for the actual patch(es)
> > for this issue.
> 
> There’s a corner case, though: doesn’t io_uring submission frequently do the work synchronously in the context of the calling thread?

Yes.

> If so, can a thread do a 64-bit submit with 32-bit work or vice versa?

In theory you could share an fd created in a 32-bit thread to a 64-bit
thread or vice versa, but I think at that point you absolutely are in
"you get to keep the pieces" land.

> Sometimes I think that in_compat_syscall() should have a mode in which calling it warns (e.g. not actually in a syscall when doing things in io_uring).  And the relevant operations should be properly wired up to avoid global state like this.

What do you mean with "properly wired up".  Do you really want to spread
->compat_foo methods everywhere, including read and write?  I found
in_compat_syscall() a lot small and easier to maintain than all the
separate compat cruft.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: io_uring vs in_compat_syscall()
  2020-07-21  7:07         ` Christoph Hellwig
@ 2020-07-21 14:31           ` Andy Lutomirski
  2020-07-21 14:34             ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Lutomirski @ 2020-07-21 14:31 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Jens Axboe, linux-arch, Linux API, LKML, io-uring

On Tue, Jul 21, 2020 at 12:07 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Jul 20, 2020 at 10:28:55AM -0700, Andy Lutomirski wrote:
> > > Sure, I'd consider that implementation detail for the actual patch(es)
> > > for this issue.
> >
> > There’s a corner case, though: doesn’t io_uring submission frequently do the work synchronously in the context of the calling thread?
>
> Yes.
>
> > If so, can a thread do a 64-bit submit with 32-bit work or vice versa?
>
> In theory you could share an fd created in a 32-bit thread to a 64-bit
> thread or vice versa, but I think at that point you absolutely are in
> "you get to keep the pieces" land.

That seems potentially okay as long as these are pieces of userspace
and not pieces of the kernel.  If the kernel freaks out, we have a
problem.

>
> > Sometimes I think that in_compat_syscall() should have a mode in which calling it warns (e.g. not actually in a syscall when doing things in io_uring).  And the relevant operations should be properly wired up to avoid global state like this.
>
> What do you mean with "properly wired up".  Do you really want to spread
> ->compat_foo methods everywhere, including read and write?  I found
> in_compat_syscall() a lot small and easier to maintain than all the
> separate compat cruft.

I was imagining using a flag.  Some of the net code uses
MSG_CMSG_COMPAT for this purpose.

--Andy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: io_uring vs in_compat_syscall()
  2020-07-21 14:31           ` Andy Lutomirski
@ 2020-07-21 14:34             ` Christoph Hellwig
  2020-07-21 17:25               ` Andy Lutomirski
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2020-07-21 14:34 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Christoph Hellwig, Jens Axboe, linux-arch, Linux API, LKML, io-uring

On Tue, Jul 21, 2020 at 07:31:02AM -0700, Andy Lutomirski wrote:
> > What do you mean with "properly wired up".  Do you really want to spread
> > ->compat_foo methods everywhere, including read and write?  I found
> > in_compat_syscall() a lot small and easier to maintain than all the
> > separate compat cruft.
> 
> I was imagining using a flag.  Some of the net code uses
> MSG_CMSG_COMPAT for this purpose.

Killing that nightmarish monster is what actually got me into looking
io_uring and starting this thread.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: io_uring vs in_compat_syscall()
  2020-07-21 14:34             ` Christoph Hellwig
@ 2020-07-21 17:25               ` Andy Lutomirski
  2020-07-22  6:30                 ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Lutomirski @ 2020-07-21 17:25 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Andy Lutomirski, Jens Axboe, linux-arch, Linux API, LKML, io-uring

On Tue, Jul 21, 2020 at 7:34 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Jul 21, 2020 at 07:31:02AM -0700, Andy Lutomirski wrote:
> > > What do you mean with "properly wired up".  Do you really want to spread
> > > ->compat_foo methods everywhere, including read and write?  I found
> > > in_compat_syscall() a lot small and easier to maintain than all the
> > > separate compat cruft.
> >
> > I was imagining using a flag.  Some of the net code uses
> > MSG_CMSG_COMPAT for this purpose.
>
> Killing that nightmarish monster is what actually got me into looking
> io_uring and starting this thread.

I agree that MSG_CMSG_COMPAT is nasty, but I think the concept is
sound -- rather than tracking whether we're compat by using a
different function or a per-thread variable, actually explicitly
tracking the mode seems sensible.

If we're going to play in_compat_syscall() games, let's please make
io_uring_enter() return -EINVAL if in_compat_syscall() != ctx->compat.

--Andy

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: io_uring vs in_compat_syscall()
  2020-07-21 17:25               ` Andy Lutomirski
@ 2020-07-22  6:30                 ` Christoph Hellwig
  2020-07-22  8:39                   ` David Laight
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2020-07-22  6:30 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Christoph Hellwig, Jens Axboe, linux-arch, Linux API, LKML, io-uring

On Tue, Jul 21, 2020 at 10:25:24AM -0700, Andy Lutomirski wrote:
> On Tue, Jul 21, 2020 at 7:34 AM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Tue, Jul 21, 2020 at 07:31:02AM -0700, Andy Lutomirski wrote:
> > > > What do you mean with "properly wired up".  Do you really want to spread
> > > > ->compat_foo methods everywhere, including read and write?  I found
> > > > in_compat_syscall() a lot small and easier to maintain than all the
> > > > separate compat cruft.
> > >
> > > I was imagining using a flag.  Some of the net code uses
> > > MSG_CMSG_COMPAT for this purpose.
> >
> > Killing that nightmarish monster is what actually got me into looking
> > io_uring and starting this thread.
> 
> I agree that MSG_CMSG_COMPAT is nasty, but I think the concept is
> sound -- rather than tracking whether we're compat by using a
> different function or a per-thread variable, actually explicitly
> tracking the mode seems sensible.

I very strongly disagree.  Two recent projects I did was to remove
the compat_exec mess, and the compat get/setsockopt mess, and each
time it removed hundreds of lines of code duplicating native
functionality, often in slightly broken ways.  We need a generic
out of band way to transfer the information down and just check in
in a few strategic places, and in_compat_syscall() does the right
thing for that.

> If we're going to play in_compat_syscall() games, let's please make
> io_uring_enter() return -EINVAL if in_compat_syscall() != ctx->compat.

That sounds like a plan, but still doesn't help with submissions from
the offload WQ or the sqpoll thread.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: io_uring vs in_compat_syscall()
  2020-07-22  6:30                 ` Christoph Hellwig
@ 2020-07-22  8:39                   ` David Laight
  0 siblings, 0 replies; 11+ messages in thread
From: David Laight @ 2020-07-22  8:39 UTC (permalink / raw)
  To: 'Christoph Hellwig', Andy Lutomirski
  Cc: Jens Axboe, linux-arch, Linux API, LKML, io-uring

From: Christoph Hellwig
> Sent: 22 July 2020 07:31
...
> > I agree that MSG_CMSG_COMPAT is nasty, but I think the concept is
> > sound -- rather than tracking whether we're compat by using a
> > different function or a per-thread variable, actually explicitly
> > tracking the mode seems sensible.
> 
> I very strongly disagree.  Two recent projects I did was to remove
> the compat_exec mess, and the compat get/setsockopt mess, and each
> time it removed hundreds of lines of code duplicating native
> functionality, often in slightly broken ways.  We need a generic
> out of band way to transfer the information down and just check in
> in a few strategic places, and in_compat_syscall() does the right
> thing for that.

Hmmm... set_fs(KERNEL_DS) is a per-thread variable that indicates
that the current system call is being done by the kernel.

So you are pulling two different bits of code in opposite directions.

It has to be safer to track the flag through with the request.
Then once any conversion has been done the flag can be corrected.

Imagine something like a bpf hook on a compat syscall.
Having read the user buffer into kernel space it may decide
to reformat it to the native layout to process it.
After which the code has a native format buffer even though
in_compat_syscall() returns true.

To the native/compat flag is actually a property of the buffer
much the same as whether it is user/kernel.

The other property of the buffer is whether embedded addresses
are user or kernel.
If the buffer has been read from userspace then they are user.
If the request originated in the kernel they are kernel.
This difference may matter in the future.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-07-22  8:39 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-20  6:10 io_uring vs in_compat_syscall() Christoph Hellwig
2020-07-20 16:36 ` Jens Axboe
2020-07-20 16:58   ` Andy Lutomirski
2020-07-20 17:02     ` Jens Axboe
2020-07-20 17:28       ` Andy Lutomirski
2020-07-21  7:07         ` Christoph Hellwig
2020-07-21 14:31           ` Andy Lutomirski
2020-07-21 14:34             ` Christoph Hellwig
2020-07-21 17:25               ` Andy Lutomirski
2020-07-22  6:30                 ` Christoph Hellwig
2020-07-22  8:39                   ` David Laight

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).