On Wed, May 1, 2019 at 1:48 PM Eric Wong <e@xxxxxxxxx> wrote:
>
> Deepa Dinamani <deepa.kernel@xxxxxxxxx> wrote:
> > So here is my analysis:
>
> <snip everything I agree with>
>
> > So the 854a6ed56839a40f6 seems to be better than the original code in
> > that it detects the signal.
>
> OTOH, does matter to anybody that a signal is detected slightly
> sooner than it would've been, otherwise?
The original code drops the signal altogether. This is because it
overwrites the current's sigmask with the provided
one(set_current_blocked()). If a signal bit was set, it is lost
forever. It does not detect it sooner. The check for pending signal is
sooner and not just before the syscall returns.
This is what the patch in discussion does: check for signals just
before returning.
>
> > But, the problem is that it doesn't
> > communicate it to the userspace.
>
> Yup, that's a big problem :)
>
> > So a patch like below solves the problem. This is incomplete. I'll
> > verify and send you a proper fix you can test soon. This is just for
> > the sake of discussion:
> >
> > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> > index 4a0e98d87fcc..63a387329c3d 100644
> > --- a/fs/eventpoll.c
> > +++ b/fs/eventpoll.c
> > @@ -2317,7 +2317,7 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct
> > epoll_event __user *, events,
> > int, maxevents, int, timeout, const sigset_t __user *, sigmask,
> > size_t, sigsetsize)
> > {
> > - int error;
> > + int error, signal_detected;
> > sigset_t ksigmask, sigsaved;
> >
> > /*
> > @@ -2330,7 +2330,10 @@ SYSCALL_DEFINE6(epoll_pwait, int, epfd, struct
> > epoll_event __user *, events,
> >
> > error = do_epoll_wait(epfd, events, maxevents, timeout);
> >
> > - restore_user_sigmask(sigmask, &sigsaved);
> > + signal_detected = restore_user_sigmask(sigmask, &sigsaved);
> > +
> > + if (signal_detected && !error)
> > + return -EITNR;
> >
> > return error;
>
> Looks like a reasonable API.
>
> > @@ -2862,7 +2862,7 @@ void restore_user_sigmask(const void __user
> > *usigmask, sigset_t *sigsaved)
> > if (signal_pending(current)) {
> > current->saved_sigmask = *sigsaved;
> > set_restore_sigmask();
> > - return;
> > + return 0;
>
> Shouldn't that "return 1" if a signal is pending?
Yep, I meant this to be 1.
-Deepa
