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

Message ID 20190325143521.34928-1-arnd@arndb.de
State Accepted
Commit 9e75ad5d8f399a21c86271571aa630dd080223e2
Headers show
Series
  • [1/2] io_uring: fix big-endian compat signal mask handling
Related show

Commit Message

Arnd Bergmann March 25, 2019, 2:34 p.m.
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(-)

-- 
2.20.0

Comments

Jens Axboe March 25, 2019, 4:05 p.m. | #1
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
Arnd Bergmann March 25, 2019, 4:11 p.m. | #2
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
Jens Axboe March 25, 2019, 4:15 p.m. | #3
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
James Bottomley March 25, 2019, 4:19 p.m. | #4
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
Jens Axboe March 25, 2019, 4:23 p.m. | #5
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
Arnd Bergmann March 25, 2019, 4:24 p.m. | #6
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
James Bottomley March 26, 2019, 12:13 a.m. | #7
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
Arnd Bergmann March 26, 2019, 8:35 a.m. | #8
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

Patch

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;
 	}