[SRU,bionic] random: Make getrandom() ready earlier

Message ID 1530713750-22453-1-git-send-email-stefan.bader@canonical.com
State New
Headers show
Series
  • [SRU,bionic] random: Make getrandom() ready earlier
Related show

Commit Message

Stefan Bader July 4, 2018, 2:15 p.m.
From: Ben Hutchings <ben@decadent.org.uk>

This effectively reverts commit 725e828 "random: fix crng_ready()
test" which was commit 43838a23a05f upstream.  Unfortunately some
users of getrandom() don't expect it to block for long, and they need
to be fixed before we can allow this change into stable.

This doesn't directly revert that commit, but only weakens the ready
condition used by getrandom() when the GRND_RANDOM flag is not set.
Calls to getrandom() that return before the RNG is fully seeded will
generate warnings, just like reads from /dev/urandom.

https://bugs.launchpad.net/bugs/1780062

(backported from ://salsa.debian.org/kernel-team/linux/raw/stretch/debian/patches/debian/random-make-getrandom-ready-earlier.patch)
[smb: open code waiting in getrandom directly]
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
This is based on the patch from Ben Hutchings in Debian. But since
wait_for_random_bytes, which is used in the 4.15 code is exported
and used by other things, I did instead do its own wait in getrandom().

-Stefan

 drivers/char/random.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Colin Ian King July 4, 2018, 2:56 p.m. | #1
On 04/07/18 15:15, Stefan Bader wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> 
> This effectively reverts commit 725e828 "random: fix crng_ready()
> test" which was commit 43838a23a05f upstream.  Unfortunately some
> users of getrandom() don't expect it to block for long, and they need
> to be fixed before we can allow this change into stable.
> 
> This doesn't directly revert that commit, but only weakens the ready
> condition used by getrandom() when the GRND_RANDOM flag is not set.
> Calls to getrandom() that return before the RNG is fully seeded will
> generate warnings, just like reads from /dev/urandom.
> 
> https://bugs.launchpad.net/bugs/1780062
> 
> (backported from ://salsa.debian.org/kernel-team/linux/raw/stretch/debian/patches/debian/random-make-getrandom-ready-earlier.patch)
> [smb: open code waiting in getrandom directly]
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
> This is based on the patch from Ben Hutchings in Debian. But since
> wait_for_random_bytes, which is used in the 4.15 code is exported
> and used by other things, I did instead do its own wait in getrandom().
> 
> -Stefan
> 
>  drivers/char/random.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index d5f1211..6021405 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1976,10 +1976,10 @@ SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
>  	if (flags & GRND_RANDOM)
>  		return _random_read(flags & GRND_NONBLOCK, buf, count);
>  
> -	if (!crng_ready()) {
> +	if (crng_init == 0) {
>  		if (flags & GRND_NONBLOCK)
>  			return -EAGAIN;
> -		ret = wait_for_random_bytes();
> +		ret = wait_event_interruptible(crng_init_wait, crng_init > 0);
>  		if (unlikely(ret))
>  			return ret;
>  	}
> 

Looks good to me; this is a good workaround for now; fixes the issue on
my X230 too, so I've tested this and it works.

The bug link needs fixing up when this is applied.

Thanks Stefan.

Acked-by: Colin Ian King <colin.king@canonical.com>
Marcelo Henrique Cerri July 4, 2018, 3:11 p.m. | #2
Acked-by: Marcelo Henrique Cerri <marcelo.cerri@canonical.com>
Khalid Elmously July 4, 2018, 3:12 p.m. | #3
On 2018-07-04 15:56:35 , Colin Ian King wrote:
> On 04/07/18 15:15, Stefan Bader wrote:
> > From: Ben Hutchings <ben@decadent.org.uk>
> > 
> > This effectively reverts commit 725e828 "random: fix crng_ready()
> > test" which was commit 43838a23a05f upstream.  Unfortunately some
> > users of getrandom() don't expect it to block for long, and they need
> > to be fixed before we can allow this change into stable.
> > 
> > This doesn't directly revert that commit, but only weakens the ready
> > condition used by getrandom() when the GRND_RANDOM flag is not set.
> > Calls to getrandom() that return before the RNG is fully seeded will
> > generate warnings, just like reads from /dev/urandom.
> > 
> > https://bugs.launchpad.net/bugs/1780062
> > 
> > (backported from ://salsa.debian.org/kernel-team/linux/raw/stretch/debian/patches/debian/random-make-getrandom-ready-earlier.patch)
> > [smb: open code waiting in getrandom directly]
> > Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> > ---
> > This is based on the patch from Ben Hutchings in Debian. But since
> > wait_for_random_bytes, which is used in the 4.15 code is exported
> > and used by other things, I did instead do its own wait in getrandom().
> > 
> > -Stefan
> > 
> >  drivers/char/random.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/char/random.c b/drivers/char/random.c
> > index d5f1211..6021405 100644
> > --- a/drivers/char/random.c
> > +++ b/drivers/char/random.c
> > @@ -1976,10 +1976,10 @@ SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
> >  	if (flags & GRND_RANDOM)
> >  		return _random_read(flags & GRND_NONBLOCK, buf, count);
> >  
> > -	if (!crng_ready()) {
> > +	if (crng_init == 0) {
> >  		if (flags & GRND_NONBLOCK)
> >  			return -EAGAIN;
> > -		ret = wait_for_random_bytes();
> > +		ret = wait_event_interruptible(crng_init_wait, crng_init > 0);
> >  		if (unlikely(ret))
> >  			return ret;
> >  	}
> > 
> 
> Looks good to me; this is a good workaround for now; fixes the issue on
> my X230 too, so I've tested this and it works.
> 
> The bug link needs fixing up when this is applied.

The buglink seems to work fine for me (just don't copy the closing
parenthesis)

> 
> Thanks Stefan.
> 
> Acked-by: Colin Ian King <colin.king@canonical.com>
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
Khalid Elmously July 4, 2018, 3:15 p.m. | #4
On 2018-07-04 11:12:36 , Khaled Elmously wrote:
> On 2018-07-04 15:56:35 , Colin Ian King wrote:
> > On 04/07/18 15:15, Stefan Bader wrote:
> > > From: Ben Hutchings <ben@decadent.org.uk>
> > > 
> > > This effectively reverts commit 725e828 "random: fix crng_ready()
> > > test" which was commit 43838a23a05f upstream.  Unfortunately some
> > > users of getrandom() don't expect it to block for long, and they need
> > > to be fixed before we can allow this change into stable.
> > > 
> > > This doesn't directly revert that commit, but only weakens the ready
> > > condition used by getrandom() when the GRND_RANDOM flag is not set.
> > > Calls to getrandom() that return before the RNG is fully seeded will
> > > generate warnings, just like reads from /dev/urandom.
> > > 
> > > https://bugs.launchpad.net/bugs/1780062
> > > 
> > > (backported from ://salsa.debian.org/kernel-team/linux/raw/stretch/debian/patches/debian/random-make-getrandom-ready-earlier.patch)
> > > [smb: open code waiting in getrandom directly]
> > > Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> > > ---
> > > This is based on the patch from Ben Hutchings in Debian. But since
> > > wait_for_random_bytes, which is used in the 4.15 code is exported
> > > and used by other things, I did instead do its own wait in getrandom().
> > > 
> > > -Stefan
> > > 
> > >  drivers/char/random.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/char/random.c b/drivers/char/random.c
> > > index d5f1211..6021405 100644
> > > --- a/drivers/char/random.c
> > > +++ b/drivers/char/random.c
> > > @@ -1976,10 +1976,10 @@ SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
> > >  	if (flags & GRND_RANDOM)
> > >  		return _random_read(flags & GRND_NONBLOCK, buf, count);
> > >  
> > > -	if (!crng_ready()) {
> > > +	if (crng_init == 0) {
> > >  		if (flags & GRND_NONBLOCK)
> > >  			return -EAGAIN;
> > > -		ret = wait_for_random_bytes();
> > > +		ret = wait_event_interruptible(crng_init_wait, crng_init > 0);
> > >  		if (unlikely(ret))
> > >  			return ret;
> > >  	}
> > > 
> > 
> > Looks good to me; this is a good workaround for now; fixes the issue on
> > my X230 too, so I've tested this and it works.
> > 
> > The bug link needs fixing up when this is applied.
> 
> The buglink seems to work fine for me (just don't copy the closing
> parenthesis)
>

Sorry, I misspoke. I meant the "patch link" works fine for me (though
the bug link seems to work fine too)


> > 
> > Thanks Stefan.
> > 
> > Acked-by: Colin Ian King <colin.king@canonical.com>
> > 
> > -- 
> > kernel-team mailing list
> > kernel-team@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Kleber Sacilotto de Souza July 5, 2018, 7:43 a.m. | #5
On 07/04/18 16:15, Stefan Bader wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> 
> This effectively reverts commit 725e828 "random: fix crng_ready()
> test" which was commit 43838a23a05f upstream.  Unfortunately some
> users of getrandom() don't expect it to block for long, and they need
> to be fixed before we can allow this change into stable.
> 
> This doesn't directly revert that commit, but only weakens the ready
> condition used by getrandom() when the GRND_RANDOM flag is not set.
> Calls to getrandom() that return before the RNG is fully seeded will
> generate warnings, just like reads from /dev/urandom.
> 
> https://bugs.launchpad.net/bugs/1780062
> 
> (backported from ://salsa.debian.org/kernel-team/linux/raw/stretch/debian/patches/debian/random-make-getrandom-ready-earlier.patch)
> [smb: open code waiting in getrandom directly]
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
> This is based on the patch from Ben Hutchings in Debian. But since
> wait_for_random_bytes, which is used in the 4.15 code is exported
> and used by other things, I did instead do its own wait in getrandom().
> 
> -Stefan
> 
>  drivers/char/random.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index d5f1211..6021405 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1976,10 +1976,10 @@ SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
>  	if (flags & GRND_RANDOM)
>  		return _random_read(flags & GRND_NONBLOCK, buf, count);
>  
> -	if (!crng_ready()) {
> +	if (crng_init == 0) {
>  		if (flags & GRND_NONBLOCK)
>  			return -EAGAIN;
> -		ret = wait_for_random_bytes();
> +		ret = wait_event_interruptible(crng_init_wait, crng_init > 0);
>  		if (unlikely(ret))
>  			return ret;
>  	}
> 

This patch has been applied to bionic/master-next branch.

Thanks,
Kleber
Seth Forshee July 9, 2018, 10:18 p.m. | #6
On Wed, Jul 04, 2018 at 04:15:50PM +0200, Stefan Bader wrote:
> From: Ben Hutchings <ben@decadent.org.uk>
> 
> This effectively reverts commit 725e828 "random: fix crng_ready()
> test" which was commit 43838a23a05f upstream.  Unfortunately some
> users of getrandom() don't expect it to block for long, and they need
> to be fixed before we can allow this change into stable.
> 
> This doesn't directly revert that commit, but only weakens the ready
> condition used by getrandom() when the GRND_RANDOM flag is not set.
> Calls to getrandom() that return before the RNG is fully seeded will
> generate warnings, just like reads from /dev/urandom.
> 
> https://bugs.launchpad.net/bugs/1780062
> 
> (backported from ://salsa.debian.org/kernel-team/linux/raw/stretch/debian/patches/debian/random-make-getrandom-ready-earlier.patch)
> [smb: open code waiting in getrandom directly]
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>

Applied to cosmic/master-next and unstable/master, thanks!

Patch

diff --git a/drivers/char/random.c b/drivers/char/random.c
index d5f1211..6021405 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1976,10 +1976,10 @@  SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
 	if (flags & GRND_RANDOM)
 		return _random_read(flags & GRND_NONBLOCK, buf, count);
 
-	if (!crng_ready()) {
+	if (crng_init == 0) {
 		if (flags & GRND_NONBLOCK)
 			return -EAGAIN;
-		ret = wait_for_random_bytes();
+		ret = wait_event_interruptible(crng_init_wait, crng_init > 0);
 		if (unlikely(ret))
 			return ret;
 	}