Received-SPF: pass (google.com: domain of linux-crypto+bounces-457-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <tencent_0AD55DB29954CFB30D9825850DEE12325D07@qq.com>
From: Edward Adam Davis <eadavis@qq.com>
To: herbert@gondor.apana.org.au
Cc: davem@davemloft.net,
	eadavis@qq.com,
	linux-crypto@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	olivia@selenic.com,
	syzbot+c52ab18308964d248092@syzkaller.appspotmail.com,
	syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH] hwrng: core - fix task hung in hwrng_fillfn
Date: Fri,  1 Dec 2023 19:37:39 +0800
In-Reply-To: <ZWmt6wrbxN1W+cnv@gondor.apana.org.au>
References: <ZWmt6wrbxN1W+cnv@gondor.apana.org.au>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

On Fri, 1 Dec 2023 17:56:59 +0800, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index 420f155d251f..7323ddc958ce 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -225,17 +225,18 @@ static ssize_t rng_dev_read(struct file *filp, char __user *buf,
> >                        goto out;
> >                }
> >
> > -               if (mutex_lock_interruptible(&reading_mutex)) {
> > -                       err = -ERESTARTSYS;
> > -                       goto out_put;
> > -               }
> >                if (!data_avail) {
> > +                       if (mutex_lock_interruptible(&reading_mutex)) {
> > +                               err = -ERESTARTSYS;
> > +                               goto out_put;
> > +                       }
> >                        bytes_read = rng_get_data(rng, rng_buffer,
> >                                rng_buffer_size(),
> >                                !(filp->f_flags & O_NONBLOCK));
> > +                       mutex_unlock(&reading_mutex);
> >                        if (bytes_read < 0) {
> >                                err = bytes_read;
> > -                               goto out_unlock_reading;
> > +                               goto out_put;
> >                        }
> >                        data_avail = bytes_read;
> >                }
> 
> Does this change anything at all? Please explain why it was holding
> this lock for 143 seconds in the first place.  If it's doing it in
> rng_get_data, then your change has zero effect.
Reduce the scope of critical zone protection.
The original critical zone contains a too large range, especially like 
copy_to_user() should not be included in the critical zone.
> 
> > @@ -501,7 +499,10 @@ static int hwrng_fillfn(void *unused)
> >                rng = get_current_rng();
> >                if (IS_ERR(rng) || !rng)
> >                        break;
> > -               mutex_lock(&reading_mutex);
> > +               if (mutex_lock_interruptible(&reading_mutex)) {
> > +                       put_rng(rng);
> > +                       return -ERESTARTSYS;
> > +               }
> 
> No this is just the symptom.  The real problem is why is the driver
> spending 143 seconds in rng_get_data?
In the second version of the patch, I have removed the fix in hwrng_fillfn().
But for some reason, the V2 patch did not appear in the mailing list.

I think it was due to consuming too much time while executing copy_to_user() 
that resulted in 143s.
So, I narrowed down the scope of the critical area and moved the code 
copy_to_user() out of the critical area.

Edward