Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 18 Jun 2022 00:47:12 +0200
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
To:     Sebastian Siewior <bigeasy@linutronix.de>
Cc:     Jann Horn <jannh@google.com>, Theodore Ts'o <tytso@mit.edu>,
        LKML <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH] random: Fix signal_pending() usage
Message-ID: <Yq0EcG9GsAkoiF3H@zx2c4.com>
References: <20220405163931.1108442-1-jannh@google.com>
 <CAHmME9pW6heXtPrfCP7J6ODgSc8sotsv6E3dnJoVBaPi+Ph=HA@mail.gmail.com>
 <YqywapDM7NPC/X+E@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <YqywapDM7NPC/X+E@linutronix.de>
Precedence: bulk

Hi Sebastian,

On Fri, Jun 17, 2022 at 06:48:42PM +0200, Sebastian Siewior wrote:
> On 2022-04-05 20:07:27 [+0200], Jason A. Donenfeld wrote:
> > One funny aspect of the fact that signal_pending() hasn't worked right
> > since the genesis commit is that this has probably led to a lot of
> > userspace code that doesn't check the result from read() or
> > getrandom(), and that code has worked mostly fine.
> 
> :)
> 
> > I wonder if we should do something about that. Worth noting is that
> > we're no longer contending with /dev/random periodically blocking as
> > the "entropy runs out" nonsense. I can think of two possible changes,
> > which maybe aren't mutually exclusive:
> > 
> > 1) Turn signal_pending() into fatal_signal_pending() throughout the file.
> > 2) Rather than not checking signal_pending() for reads of length <=
> > 256, we could not check for signal_pending() for the first 256 bytes
> > of any length read.
> > 
> > Both of these would be changing userspace behavior, so it should
> > probably be considered carefully. Any thoughts?
> 
> You are not doing any blocking as far as I can tell so there won't be
> any wake up via TASK_INTERRUPTIBLE for you here.
> You check for the signal_pending() every PAGE_SIZE so there will be at
> least 4KiB of data, not sure where this 256 is coming from.
> Since you always return the number of bytes, there won't be any visible
> change for requests < PAGE_SIZE. And for requests > PAGE_SIZE your
> getrandom() invocation may return less than asked for. This is _now_.
> 
> If you drop that signal_pending() check then the user will always get
> the number of bytes he asked for. Given that this is *quick* as in
> no blocking, then there should be no harm in dropping this signal check.

You're a bit late to the thread :). It used to be 256. Now it's page
size. PAGE_SIZE is also what /dev/zero and others in mem.c use.

As for your suggestion to drop it entirely: that'd be nice, in that it'd
add a guarantee that currently doesn't exist. But it can lead to
somewhat large delays if somebody tries to read 2 gigabytes at a time
and hits Ctrl+C during it. That seems potentially bad?

Or that's not bad, which would be quite nice, as I would really love to
add that guarantee. So if you have an argument that not responding to
signals for that amount of time is fine, I'd be interested to hear it.

Jason