linux-kernel - Re: [PATCH v2] signal: Adjust error codes according to restore_user

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 24 May 2019 16:10:54 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Deepa Dinamani <deepa.kernel@...il.com>
Cc:     David Laight <David.Laight@...lab.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Arnd Bergmann <arnd@...db.de>,
        "dbueso@...e.de" <dbueso@...e.de>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        Davidlohr Bueso <dave@...olabs.net>, Eric Wong <e@...24.org>,
        Jason Baron <jbaron@...mai.com>,
        Linux FS-devel Mailing List <linux-fsdevel@...r.kernel.org>,
        linux-aio <linux-aio@...ck.org>,
        Omar Kilani <omar.kilani@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v2] signal: Adjust error codes according to
 restore_user_sigmask()

On 05/23, Deepa Dinamani wrote:
>
> Ok, since there has been quite a bit of argument here, I will
> backtrack a little bit and maybe it will help us understand what's
> happening here.
> There are many scenarios being discussed on this thread:
> a. State of code before 854a6ed56839a

I think everything was correct,

> b. State after 854a6ed56839a

obviously buggy,

> c. Proposed fix as per the patchset in question.

Nack, sorry. I'll try to finish my patch on Monday. It will restore the state
before 854a6ed56839a and (imo) cleanup/simplify this code.

At leat this is what I think right now. May be I will have to change my mind
after this discussion. But in any case I can't believe I will ever agree with
your fix ;)

> These are particularly meant for a scenario(d) such as below:
>
> 1. block the signals you don't care about.
> 2. syscall()
> 3. unblock the signals blocked in 1.
>
> The problem here is that if there is a signal that is not blocked by 1
> and such a signal is delivered between 1 and 2(since they are not
> atomic), the syscall in 2 might block forever as it never found out
> about the signal.

and that is why we have pselect/etc to make this sequence "atomic".

> As per [a] and let's consider the case of epoll_pwait only first for simplicity.
>
> As I said before, ep_poll() is what checks for signal_pending() and is
> responsible for setting errno to -EINTR when there is a signal.

To clarify, if do_epoll_wait() return -EINTR then signal_pending() is true,
right?

> So if a signal is received after ep_poll() and ep_poll() returns
> success, it is never noticed by the syscall during execution.

What you are saying looks very confusing to me, I will assume that you
meant something like

	- a signal SIG_XXX was blocked before sys_epoll_pwait() was called

	- sys_epoll_pwait(sigmask) unblocks SIG_XXX according to sigmask

	- sys_epoll_pwait() calls do_epoll_wait() which returns success

	- SIG_XXX comes after that and it is "never noticed"

Yes. Everything is correct. And see my reply to David, SIG_XXX can even
come _before_ sys_epoll_pwait() was called.

> So the question is does the userspace have to know about this signal
> or not.

If userspace needs to know about SIG_XXX it should not block it, that is all.

> What [b] does is to move the signal check closer to the restoration of
> the signal.

FOR NO REASON, afaics (to simplify, lets forget the problem with the wrong
return value you are trying to fix).

And even if there were ANY reason to do this, note that (with or without this
fix) the signal_pending() check inside restore_user_sigmask() can NOT help,
simply because SIG_XXX can come right after this check.

Oleg.