[PATCH 1/2] io_uring: fix big-endian compat signal mask handling

[PATCH 1/2] io_uring: fix big-endian compat signal mask handling

[Arnd Bergmann]

On big-endian architectures, the signal masks are differnet
between 32-bit and 64-bit tasks, so we have to use a different
function for reading them from user space.

io_cqring_wait() initially got this wrong, and always interprets
this as a native structure. This is ok on x86 and most arm64,
but not on s390, ppc64be, mips64be, sparc64 and parisc.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/io_uring.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6aaa30580a2b..8f48d29abf76 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1968,7 +1968,15 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
 		return 0;
 
 	if (sig) {
-		ret = set_user_sigmask(sig, &ksigmask, &sigsaved, sigsz);
+#ifdef CONFIG_COMPAT
+		if (in_compat_syscall())
+			ret = set_compat_user_sigmask((const compat_sigset_t __user *)sig,
+						      &ksigmask, &sigsaved, sigsz);
+		else
+#endif
+			ret = set_user_sigmask(sig, &ksigmask,
+					       &sigsaved, sigsz);
+
 		if (ret)
 			return ret;
 	}
-- 
2.20.0

Re: [PATCH 1/2] io_uring: fix big-endian compat signal mask handling

[Jens Axboe]

On 3/25/19 8:34 AM, Arnd Bergmann wrote:
> On big-endian architectures, the signal masks are differnet
> between 32-bit and 64-bit tasks, so we have to use a different
> function for reading them from user space.
> 
> io_cqring_wait() initially got this wrong, and always interprets
> this as a native structure. This is ok on x86 and most arm64,
> but not on s390, ppc64be, mips64be, sparc64 and parisc.

Thanks Arnd, applied.

Was there a 2/2 patch? I only received this one, 1/2.

-- 
Jens Axboe

Re: [PATCH 1/2] io_uring: fix big-endian compat signal mask handling

[Arnd Bergmann]

On Mon, Mar 25, 2019 at 5:05 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 3/25/19 8:34 AM, Arnd Bergmann wrote:
> > On big-endian architectures, the signal masks are differnet
> > between 32-bit and 64-bit tasks, so we have to use a different
> > function for reading them from user space.
> >
> > io_cqring_wait() initially got this wrong, and always interprets
> > this as a native structure. This is ok on x86 and most arm64,
> > but not on s390, ppc64be, mips64be, sparc64 and parisc.
>
> Thanks Arnd, applied.
>
> Was there a 2/2 patch? I only received this one, 1/2.

Sorry I missed you on Cc:
https://lore.kernel.org/lkml/20190325144737.703921-1-arnd@arndb.de/T/#u

This one went out to all the affected arch maintainers.

    Arnd

Re: [PATCH 1/2] io_uring: fix big-endian compat signal mask handling

[Jens Axboe]

On 3/25/19 10:11 AM, Arnd Bergmann wrote:
> On Mon, Mar 25, 2019 at 5:05 PM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> On 3/25/19 8:34 AM, Arnd Bergmann wrote:
>>> On big-endian architectures, the signal masks are differnet
>>> between 32-bit and 64-bit tasks, so we have to use a different
>>> function for reading them from user space.
>>>
>>> io_cqring_wait() initially got this wrong, and always interprets
>>> this as a native structure. This is ok on x86 and most arm64,
>>> but not on s390, ppc64be, mips64be, sparc64 and parisc.
>>
>> Thanks Arnd, applied.
>>
>> Was there a 2/2 patch? I only received this one, 1/2.
> 
> Sorry I missed you on Cc:
> https://lore.kernel.org/lkml/20190325144737.703921-1-arnd@arndb.de/T/#u
> 
> This one went out to all the affected arch maintainers.

Ah gotcha, just wanted to make sure I didn't miss a patch.

-- 
Jens Axboe

Re: [PATCH 1/2] io_uring: fix big-endian compat signal mask handling

[James Bottomley]

On Mon, 2019-03-25 at 15:34 +0100, Arnd Bergmann wrote:
> On big-endian architectures, the signal masks are differnet
> between 32-bit and 64-bit tasks, so we have to use a different
> function for reading them from user space.
> 
> io_cqring_wait() initially got this wrong, and always interprets
> this as a native structure. This is ok on x86 and most arm64,
> but not on s390, ppc64be, mips64be, sparc64 and parisc.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  fs/io_uring.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 6aaa30580a2b..8f48d29abf76 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1968,7 +1968,15 @@ static int io_cqring_wait(struct io_ring_ctx
> *ctx, int min_events,
>  		return 0;
>  
>  	if (sig) {
> -		ret = set_user_sigmask(sig, &ksigmask, &sigsaved,
> sigsz);
> +#ifdef CONFIG_COMPAT
> +		if (in_compat_syscall())
> +			ret = set_compat_user_sigmask((const
> compat_sigset_t __user *)sig,
> +						      &ksigmask,
> &sigsaved, sigsz);
> +		else
> +#endif

This looks a bit suboptimal: shouldn't in_compat_syscall() be hard
coded to return 0 if CONFIG_COMPAT isn't defined?  That way the
compiler can do the correct optimization and we don't have to litter
#ifdefs and worry about undefined variables and other things.

James

Re: [PATCH 1/2] io_uring: fix big-endian compat signal mask handling

[Jens Axboe]

On 3/25/19 10:19 AM, James Bottomley wrote:
> On Mon, 2019-03-25 at 15:34 +0100, Arnd Bergmann wrote:
>> On big-endian architectures, the signal masks are differnet
>> between 32-bit and 64-bit tasks, so we have to use a different
>> function for reading them from user space.
>>
>> io_cqring_wait() initially got this wrong, and always interprets
>> this as a native structure. This is ok on x86 and most arm64,
>> but not on s390, ppc64be, mips64be, sparc64 and parisc.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  fs/io_uring.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 6aaa30580a2b..8f48d29abf76 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -1968,7 +1968,15 @@ static int io_cqring_wait(struct io_ring_ctx
>> *ctx, int min_events,
>>  		return 0;
>>  
>>  	if (sig) {
>> -		ret = set_user_sigmask(sig, &ksigmask, &sigsaved,
>> sigsz);
>> +#ifdef CONFIG_COMPAT
>> +		if (in_compat_syscall())
>> +			ret = set_compat_user_sigmask((const
>> compat_sigset_t __user *)sig,
>> +						      &ksigmask,
>> &sigsaved, sigsz);
>> +		else
>> +#endif
> 
> This looks a bit suboptimal: shouldn't in_compat_syscall() be hard
> coded to return 0 if CONFIG_COMPAT isn't defined?  That way the
> compiler can do the correct optimization and we don't have to litter
> #ifdefs and worry about undefined variables and other things.

That requires the types to be valid for !CONFIG_COMPAT, as well as the
sigmask helper.

-- 
Jens Axboe

Re: [PATCH 1/2] io_uring: fix big-endian compat signal mask handling

[Arnd Bergmann]

On Mon, Mar 25, 2019 at 5:19 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:

> > --- a/fs/io_uring.c
> > +++ b/fs/io_uring.c
> > @@ -1968,7 +1968,15 @@ static int io_cqring_wait(struct io_ring_ctx
> > *ctx, int min_events,
> >               return 0;
> >
> >       if (sig) {
> > -             ret = set_user_sigmask(sig, &ksigmask, &sigsaved,
> > sigsz);
> > +#ifdef CONFIG_COMPAT
> > +             if (in_compat_syscall())
> > +                     ret = set_compat_user_sigmask((const
> > compat_sigset_t __user *)sig,
> > +                                                   &ksigmask,
> > &sigsaved, sigsz);
> > +             else
> > +#endif
>
> This looks a bit suboptimal: shouldn't in_compat_syscall() be hard
> coded to return 0 if CONFIG_COMPAT isn't defined?  That way the
> compiler can do the correct optimization and we don't have to litter
> #ifdefs and worry about undefined variables and other things.

The check can be outside of the #ifdef, but set_compat_user_sigmask
is not declared then.

I think for the future we can consider just moving the compat logic
into set_user_sigmask(), which would simplify most of the callers,
but that seemed to invasive as a bugfix for 5.1.

      Arnd

Re: [PATCH 1/2] io_uring: fix big-endian compat signal mask handling

[James Bottomley]

On Mon, 2019-03-25 at 17:24 +0100, Arnd Bergmann wrote:
> On Mon, Mar 25, 2019 at 5:19 PM James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> 
> > > --- a/fs/io_uring.c
> > > +++ b/fs/io_uring.c
> > > @@ -1968,7 +1968,15 @@ static int io_cqring_wait(struct
> > > io_ring_ctx
> > > *ctx, int min_events,
> > >               return 0;
> > > 
> > >       if (sig) {
> > > -             ret = set_user_sigmask(sig, &ksigmask, &sigsaved,
> > > sigsz);
> > > +#ifdef CONFIG_COMPAT
> > > +             if (in_compat_syscall())
> > > +                     ret = set_compat_user_sigmask((const
> > > compat_sigset_t __user *)sig,
> > > +                                                   &ksigmask,
> > > &sigsaved, sigsz);
> > > +             else
> > > +#endif
> > 
> > This looks a bit suboptimal: shouldn't in_compat_syscall() be hard
> > coded to return 0 if CONFIG_COMPAT isn't defined?  That way the
> > compiler can do the correct optimization and we don't have to
> > litter #ifdefs and worry about undefined variables and other
> > things.
> 
> The check can be outside of the #ifdef, but set_compat_user_sigmask
> is not declared then.

Right, but shouldn't it be declared?  I thought BUILD_BUG_ON had nice
magic that allowed it to work here (meaning if the compiler doesn't
eliminate the branch we get a build bug).

> I think for the future we can consider just moving the compat logic
> into set_user_sigmask(), which would simplify most of the callers,
> but that seemed to invasive as a bugfix for 5.1.

Well, that too.  I've just been on a recent bender to stop #ifdefs
after I saw what some people were doing with them.

James

Re: [PATCH 1/2] io_uring: fix big-endian compat signal mask handling

[Arnd Bergmann]

On Tue, Mar 26, 2019 at 1:13 AM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Mon, 2019-03-25 at 17:24 +0100, Arnd Bergmann wrote:
> > On Mon, Mar 25, 2019 at 5:19 PM James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > > This looks a bit suboptimal: shouldn't in_compat_syscall() be hard
> > > coded to return 0 if CONFIG_COMPAT isn't defined?  That way the
> > > compiler can do the correct optimization and we don't have to
> > > litter #ifdefs and worry about undefined variables and other
> > > things.
> >
> > The check can be outside of the #ifdef, but set_compat_user_sigmask
> > is not declared then.
>
> Right, but shouldn't it be declared?  I thought BUILD_BUG_ON had nice
> magic that allowed it to work here (meaning if the compiler doesn't
> eliminate the branch we get a build bug).

My y2038 series originally went in that direction by allowing much more
of the compat code to be compiled and then discarded without the
#ifdefs (and combine it with the 32-bit time_t handling on 32-bit
architectures). I went away from that after Christoph and others found
the reuse of the compat interfaces too confusing.

The current state now is that most compat_* interfaces cannot be
compiled unless CONFIG_COMPAT is set, and making that work
in general is a lot of work, so I followed the usual precedent here
and used that #ifdef. This also matches what is done elsewhere
in the same file (see io_import_iovec).

> > I think for the future we can consider just moving the compat logic
> > into set_user_sigmask(), which would simplify most of the callers,
> > but that seemed to invasive as a bugfix for 5.1.
>
> Well, that too.  I've just been on a recent bender to stop #ifdefs
> after I saw what some people were doing with them.

I absolutely agree in general, and have sent many patches to
remove #ifdefs in other code when there was a good alternative
and the #ifdefs are wrong (which they are at least 30% of the time
in my experience).

The problems for doing this in general for compat code are

- some structures have a conditional compat_ioctl() callback
  pointer, and need an #ifdef around the assignment until
  we change the struct as well.
- Most compat handlers require the use of the compat_ptr()
  wrapper, I have a patch to move this to common code, but
  that was rejected previously
- many compat handlers rely on types from asm/compat.h
  that does not exist on architectures without compat support.

In this specific case, compat_sigset_t is required for declaring
set_compat_user_sigmask(), and the former is not easy to
define on non-compat architectures. I still think that the best
way forward here is to move it into set_user_sigmask()
next merge window, rather than doing a larger scale rewrite
of linux/compat.h to get this bug fixed now.

      Arnd