Message-ID: <CABeXuvquQoBTWb3sx0DCkpeSjphF9w6W-dMh0v85N7qrjQJCSg@mail.gmail.com>
Date:   Fri, 3 May 2019 15:53:42 -0700
From:   Deepa Dinamani <deepa.kernel@...il.com>
To:     Davidlohr Bueso <dave@...olabs.net>,
        Thomas Gleixner <tglx@...utronix.de>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Eric Wong <e@...24.org>, Omar Kilani <omar.kilani@...il.com>,
        Jason Baron <jbaron@...mai.com>, Arnd Bergmann <arnd@...db.de>,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] signal: Adjust error codes according to restore_user_sigmask()

The original patch was merged through the tip tree. Adding tglx just in case.

I will post the revised patch to everyone on this thread.

> >For all the syscalls that receive a sigmask from the userland,
> >the user sigmask is to be in effect through the syscall execution.
> >At the end of syscall, sigmask of the current process is restored
> >to what it was before the switch over to user sigmask.
> >But, for this to be true in practice, the sigmask should be restored
> >only at the the point we change the saved_sigmask. Anything before
> >that loses signals. And, anything after is just pointless as the
> >signal is already lost by restoring the sigmask.
> >
> >The issue was detected because of a regression caused by 854a6ed56839a.
> >The patch moved the signal_pending() check closer to restoring of the
> >user sigmask. But, it failed to update the error code accordingly.
> >
> >Detailed issue discussion permalink:
> >https://lore.kernel.org/linux-fsdevel/20190427093319.sgicqik2oqkez3wk@dcvr/
> >
> >Note that the patch returns interrupted errors (EINTR, ERESTARTNOHAND,
> >etc) only when there is no other error. If there is a signal and an error
> >like EINVAL, the syscalls return -EINVAL rather than the interrupted
> >error codes.
>
> Thanks for doing this; I've reviewed the epoll bits (along with the overall
> idea) and it seems like a sane alternative to reverting the offending patch.

Sorry maybe the description wasn't clear. What I actually am saying is
that all these syscalls were dropping signals before and
854a6ed56839a4 actually did things right by making sure they did not
do so.
But, there was a bug in that it did not communicate to userspace when
the error code was not already set.
However, we could still argue that the check and flipping of the mask
isn't atomic and there is still a way this can theoretically happen.
But, this will also mean that these syscalls will slow down further.
But, they are already expected to be slow so maybe it doesn't matter.
I will note this down in the commit text.
I don't think reverting was an alternative. 854a6ed56839a4 exposed a
bug that was already there.

> Feel free to add:
>
> Reviewed-by: Davidlohr Bueso <dbueso@...e.de>
>
> A small nit, I think we should be a bit more verbose about the return semantics
> of restore_user_sigmask()... see at the end.
>
> >
> >Reported-by: Eric Wong <e@...24.org>
> >Fixes: 854a6ed56839a40f6b5d02a2962f48841482eec4 ("signal: Add restore_user_sigmask()")
> >Signed-off-by: Deepa Dinamani <deepa.kernel@...il.com>
> >--- a/kernel/signal.c
> >+++ b/kernel/signal.c
> >@@ -2845,15 +2845,16 @@ EXPORT_SYMBOL(set_compat_user_sigmask);
> >  * usigmask: sigmask passed in from userland.
> >  * sigsaved: saved sigmask when the syscall started and changed the sigmask to
> >  *           usigmask.
> >+ * returns 1 in case a pending signal is detected.
>
> How about:
>
> "
> Callers must carefully coordinate between signal_pending() checks between the
> actual system call and restore_user_sigmask() - for which a new pending signal
> may come in between calls and therefore must consider this for returning a proper
> error code.
>
> Returns 1 in case a signal pending is detected, otherwise 0.

Ok, I will add more verbiage here.

Thanks,
Deepa

Powered by blists - more mailing lists