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...