linux-kernel - Re: [PATCH] signal: remove the wrong signal_pending() check in 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20190605085604.GA32406@redhat.com>
Date:   Wed, 5 Jun 2019 10:56:04 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Deepa Dinamani <deepa.kernel@...il.com>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Arnd Bergmann <arnd@...db.de>,
        Davidlohr Bueso <dbueso@...e.de>, Jens Axboe <axboe@...nel.dk>,
        Davidlohr Bueso <dave@...olabs.net>, e@...24.org,
        Jason Baron <jbaron@...mai.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-aio@...ck.org, omar.kilani@...il.com,
        Thomas Gleixner <tglx@...utronix.de>,
        stable <stable@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        David Laight <David.Laight@...lab.com>
Subject: Re: [PATCH] signal: remove the wrong signal_pending() check in
 restore_user_sigmask()

On 06/04, Linus Torvalds wrote:
>
> On Tue, Jun 4, 2019 at 6:41 AM Oleg Nesterov <oleg@...hat.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.

Yes,

> In particular, since restore_user_sigmask() basically wants to check
> for "signal_pending()" anyway

No, the caller should check signal_pending() anyway and this is enough.

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

This is fs/aio.c and I have already mentioned that this code doesn't look
right anyway.

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

I think this particular code should simply do

		ret = do_io_getevents(...);

		if (ret == -ERESTARTSYS)
			ret = -EINTR;

		restore_user_sigmask(ret == -EINTR);

However I agree that another helper(s) which takes/returns the error code makes
sense and I was going to do this. Lets do this step by step, I think we should
kill sigmask/sigsaved first.

Oleg.