2021-01-02 21:18:34

by Herbert Xu

Subject: Re: [PATCH v2] hwrng: fix khwrng lifecycle

On Wed, Dec 16, 2020 at 11:59:06AM +0100, Luca Dariz wrote:
>
> @@ -432,12 +433,15 @@ static int hwrng_fillfn(void *unused)
> {
> long rc;
>
> + complete(&hwrng_started);
> while (!kthread_should_stop()) {
> struct hwrng *rng;
>
> rng = get_current_rng();
> - if (IS_ERR(rng) || !rng)
> - break;
> + if (IS_ERR(rng) || !rng) {
> + msleep_interruptible(10);
> + continue;

Please fix this properly with reference counting.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2021-02-15 12:54:39

by Luca Dariz

Subject: RE: [PATCH v2] hwrng: fix khwrng lifecycle

>On Wed, Dec 16, 2020 at 11:59:06AM +0100, Luca Dariz wrote:
>>
>> @@ -432,12 +433,15 @@ static int hwrng_fillfn(void *unused) {
>> long rc;
>>
>> + complete(&hwrng_started);
>> while (!kthread_should_stop()) {
>> struct hwrng *rng;
>>
>> rng = get_current_rng();
>> - if (IS_ERR(rng) || !rng)
>> - break;
>> + if (IS_ERR(rng) || !rng) {
>> + msleep_interruptible(10);
>> + continue;
>
>Please fix this properly with reference counting.

I thought a bit more about it, but I always find a potential race condition with kthread_stop() and the hwrng_fill NULL pointer check.
In my opinion the thread termination should be only triggered with kthread_stop(), otherwise it might be called with an invalid or NULL hwrng_fill.
Am I missing something?

Thanks
Luca