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:   Fri, 17 Jun 2022 18:48:42 +0200
From:   Sebastian Siewior <bigeasy@linutronix.de>
To:     "Jason A. Donenfeld" <Jason@zx2c4.com>
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: <YqywapDM7NPC/X+E@linutronix.de>
References: <20220405163931.1108442-1-jannh@google.com>
 <CAHmME9pW6heXtPrfCP7J6ODgSc8sotsv6E3dnJoVBaPi+Ph=HA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAHmME9pW6heXtPrfCP7J6ODgSc8sotsv6E3dnJoVBaPi+Ph=HA@mail.gmail.com>
Precedence: bulk

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.

> Jason

Sebastian