Hey Al,
I'm CC'ing you into this thread, as you might have an opinion on the
matter. I also have a few odd questions and observations about
implementing this now that we use iters.
On Thu, Sep 08, 2022 at 11:51:52AM +0200, Jason A. Donenfeld wrote:
> The question now before us is whether to bring back the behavior that
> was there pre-5.6, or to keep the behavior that has existed since 5.6.
> Accidental regressions like this (I assume it was accidental, at least)
> that are unnoticed for so long tend to ossify and become the new
> expected behavior. It's been around 2.5 years since 5.6, and this is the
> first report of breakage. But the fact that it does break things for you
> *is* still significant.
>
> If this was just something you noticed during idle curiosity but doesn't
> have a real impact on anything, then I'm inclined to think we shouldn't
> go changing the behavior /again/ after 2.5 years. But it sounds like
> actually you have a real user space in a product that stopped working
> when you tried to upgrade the kernel from 4.4 to one >5.6. If this is
> the case, then this sounds truly like a userspace-breaking regression,
> which we should fix by restoring the old behavior. Can you confirm this
> is the case? And in the meantime, I'll prepare a patch for restoring
> that old behavior.
The tl;dr of the thread is that kernels before 5.6 would return -EAGAIN
when reading from /dev/random during early boot if the fd was opened
with O_NONBLOCK. Some refactoring done 2.5 years ago evidently removed
this, and so we're now getting a report that this might have broken
something. One question is whether to fix the regression, or if 2.5
years is time enough for ossification. But assuming it should be fixed,
I started looking into implementing that.
The most straight forward approach to directly handle the regression
would be just doing this:
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 79d7d4e4e582..09c944dce7ff 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1347,6 +1347,9 @@ static ssize_t random_read_iter(struct kiocb *kiocb, struct iov_iter *iter)
{
int ret;
+ if (!crng_ready() && (kiocb->ki_filp->f_flags & O_NONBLOCK))
+ return -EAGAIN;
+
ret = wait_for_random_bytes();
if (ret != 0)
return ret;
So that works. But then I started looking at the other knobs in kiocb
and in the fmode interface in general and landed in a world of
confusion. There are a few other things that might be done:
- Also check `kiocb->ki_flags & IOCB_NOWAIT`.
- Also check `kiocb->ki_flags & IOCB_NOIO`.
- Set the `FMODE_NOWAIT` when declaring the file.
- Other weird aio things?
Do any of these make sense to do?
I started playing with userspace programs to try and trigger some of
those ki_flags, and I saw that the preadv2(2) syscall has the RWF_NOWAIT
flag, so I coded that up and nothing happened. I went grepping and found
some things:
In linux/fs.h:
#define IOCB_NOWAIT (__force int) RWF_NOWAIT
so these are intended to be the same. That seems reflected too in
overlayfs/file.c:
if (ifl & IOCB_NOWAIT)
flags |= RWF_NOWAIT;
Then later in linux/fs.h, it seems like RWF_NOWAIT is *also* enabling
IOCB_NOIO:
if (flags & RWF_NOWAIT) {
if (!(ki->ki_filp->f_mode & FMODE_NOWAIT))
return -EOPNOTSUPP;
kiocb_flags |= IOCB_NOIO;
}
kiocb_flags |= (__force int) (flags & RWF_SUPPORTED);
But then most of the places involved with IOCB_NOIO are also checking
IOCB_NOWAIT at the same time. Except for one place in filemap?
So it's not really clear to me what the intended behavior is of these,
and I'm wondering if we're hitting another no_llseek-like redundancy
again here, or if something else is up. Or, more likely, I'm just
overwhelmed by the undocumented subtleties here.
Jason
