Messages in this thread | | | Date | Tue, 4 Jun 2019 22:24:41 +0000 | From | Eric Wong <> | Subject | Re: [PATCH] signal: remove the wrong signal_pending() check in restore_user_sigmask() |
| |
Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Tue, Jun 4, 2019 at 6:41 AM Oleg Nesterov <oleg@redhat.com> wrote: > > > > This is the minimal fix for stable, I'll send cleanups later. > > Ugh. I htink this is correct, but I wish we had a better and more > intuitive interface.
I had the same thoughts, but am not a regular kernel hacker, so I didn't say anything earlier.
> In particular, since restore_user_sigmask() basically wants to check > for "signal_pending()" anyway (to decide if the mask should be > restored by signal handling or by that function), I really get the > feeling that a lot of these patterns like > > > - restore_user_sigmask(ksig.sigmask, &sigsaved); > > - if (signal_pending(current) && !ret) > > + > > + interrupted = signal_pending(current); > > + restore_user_sigmask(ksig.sigmask, &sigsaved, interrupted); > > + if (interrupted && !ret) > > ret = -ERESTARTNOHAND; > > are wrong to begin with, and we really should aim for an interface > which says "tell me whether you completed the system call, and I'll > give you an error return if not". > > How about we make restore_user_sigmask() take two return codes: the > 'ret' we already have, and the return we would get if there is a > signal pending and w're currently returning zero. > > IOW, I think the above could become > > ret = restore_user_sigmask(ksig.sigmask, &sigsaved, ret, -ERESTARTHAND); > > instead if we just made the right interface decision.
But that falls down if ret were ever expected to match several similar error codes (not sure if it happens)
When I was considering fixing this on my own a few weeks ago, I was looking for an inline that could quickly tell if `ret' was any of the EINTR-like error codes; but couldn't find one...
It'd probably end up being switch/case statement so I'm not sure if it'd be too big and slow or not...
The caller would just do:
ret = restore_user_sigmask(ksig.sigmask, &sigsaved, ret);
And restore_user_sigmask would call some "was_interrupted(ret)" inline which could return true if `ret' matched any of the too-many-to-keep-track-of EINTR-like codes. But I figured there's probably a good reason it did not exist, already *shrug*
/me goes back to the wonderful world of userspace...
| |