RE: [PATCH v2] signal: Adjust error codes according to restore_user_sigmask()

[David Laight]

From: Deepa Dinamani
> Sent: 22 May 2019 17:34
> On Wed, May 22, 2019 at 9:14 AM Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> >
> > On 05/22, Deepa Dinamani wrote:
> > >
> > > -Deepa
> > >
> > > > On May 22, 2019, at 8:05 AM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> > > >
> > > >> On 05/21, Deepa Dinamani wrote:
> > > >>
> > > >> Note that this 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.
> > > >
> > > > Ugh. I need to re-check, but at first glance I really dislike this change.
> > > >
> > > > I think we can fix the problem _and_ simplify the code. Something like below.
> > > > The patch is obviously incomplete, it changes only only one caller of
> > > > set_user_sigmask(), epoll_pwait() to explain what I mean.
> > > > restore_user_sigmask() should simply die. Although perhaps another helper
> > > > makes sense to add WARN_ON(test_tsk_restore_sigmask() && !signal_pending).
> > >
> > > restore_user_sigmask() was added because of all the variants of these
> > > syscalls we added because of y2038 as noted in commit message:
> > >
> > >   signal: Add restore_user_sigmask()
> > >
> > >     Refactor the logic to restore the sigmask before the syscall
> > >     returns into an api.
> > >     This is useful for versions of syscalls that pass in the
> > >     sigmask and expect the current->sigmask to be changed during
> > >     the execution and restored after the execution of the syscall.
> > >
> > >     With the advent of new y2038 syscalls in the subsequent patches,
> > >     we add two more new versions of the syscalls (for pselect, ppoll
> > >     and io_pgetevents) in addition to the existing native and compat
> > >     versions. Adding such an api reduces the logic that would need to
> > >     be replicated otherwise.
> >
> > Again, I need to re-check, will continue tomorrow. But so far I am not sure
> > this helper can actually help.
> >
> > > > --- a/fs/eventpoll.c
> > > > +++ b/fs/eventpoll.c
> > > > @@ -2318,19 +2318,19 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct epoll_event __user *,
> events,
> > > >        size_t, sigsetsize)
> > > > {
> > > >    int error;
> > > > -    sigset_t ksigmask, sigsaved;
> > > >
> > > >    /*
> > > >     * If the caller wants a certain signal mask to be set during the wait,
> > > >     * we apply it here.
> > > >     */
> > > > -    error = set_user_sigmask(sigmask, &ksigmask, &sigsaved, sigsetsize);
> > > > +    error = set_user_sigmask(sigmask, sigsetsize);
> > > >    if (error)
> > > >        return error;
> > > >
> > > >    error = do_epoll_wait(epfd, events, maxevents, timeout);
> > > >
> > > > -    restore_user_sigmask(sigmask, &sigsaved);
> > > > +    if (error != -EINTR)
> > >
> > > As you address all the other syscalls this condition becomes more and
> > > more complicated.
> >
> > May be.
> >
> > > > --- a/include/linux/sched/signal.h
> > > > +++ b/include/linux/sched/signal.h
> > > > @@ -416,7 +416,6 @@ void task_join_group_stop(struct task_struct *task);
> > > > static inline void set_restore_sigmask(void)
> > > > {
> > > >    set_thread_flag(TIF_RESTORE_SIGMASK);
> > > > -    WARN_ON(!test_thread_flag(TIF_SIGPENDING));
> > >
> > > So you always want do_signal() to be called?
> >
> > Why do you think so? No. This is just to avoid the warning, because with the
> > patch I sent set_restore_sigmask() is called "in advance".
> >
> > > You will have to check each architecture's implementation of
> > > do_signal() to check if that has any side effects.
> >
> > I don't think so.
> 
> Why not?
> 
> > > Although this is not what the patch is solving.
> >
> > Sure. But you know, after I tried to read the changelog, I am not sure
> > I understand what exactly you are trying to fix. Could you please explain
> > this part
> >
> >         The behavior
> >         before 854a6ed56839a was that the signals were dropped after the error
> >         code was decided. This resulted in lost signals but the userspace did not
> >         notice it
> >
> > ? I fail to understand it, sorry. It looks as if the code was already buggy before
> > that commit and it could miss a signal or something like this, but I do not see how.
> 
> Did you read the explanation pointed to in the commit text? :
> 
> https://lore.kernel.org/linux-fsdevel/20190427093319.sgicqik2oqkez3wk@dcvr/
> 
> Let me know what part you don't understand and I can explain more.
> 
> It would be better to understand the isssue before we start discussing the fix.


I'm confused...
I thought:

EINTR should only be returned if a blocking sleep (eg in do_epoll_wait() itself)
was interrupted by a signal that was enabled at the time of the sleep.

The handlers for all unblocked signals should be run on return to user.
This is after the mask has been restored and regardless of the error code.

So epoll() can return 'success' or 'timeout' (etc) and the handler for SIG_URG
should still be called.
This is exactly equivalent to the interrupt that generates the signal happening
just after the 'return to user' of the system call.

	David

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