[PATCH -next 0/2] A few fixup patches for crypto

[PATCH -next 0/2] A few fixup patches for crypto

[Gaosheng Cui]

This series contains a fixup patches, fix IS_ERR() vs NULL check,
and select CRYPTO_MANAGER when enable CRYPTO_CRC32C. Thanks!

Gaosheng Cui (2):
  crypto: api - Fix IS_ERR() vs NULL check
  crypto: crc32c - add missing Kconfig option select

 crypto/Kconfig  | 1 +
 crypto/algapi.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH -next 1/2] crypto: api - Fix IS_ERR() vs NULL check

[Gaosheng Cui]

The crypto_alloc_test_larval() will return null if manager is disabled,
it may not return error pointers, so using IS_ERR_OR_NULL()
to check the return value to fix this.

The __crypto_register_alg() will return null if manager is disabled,
it may not return error pointers, so using IS_ERR_OR_NULL()
to check the return value to fix this.

Fixes: cad439fc040e ("crypto: api - Do not create test larvals if manager is disabled")
Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
---
 crypto/algapi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 5c69ff8e8fa5..5a080b8aaa11 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -283,7 +283,7 @@ static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg)
 	}
 
 	larval = crypto_alloc_test_larval(alg);
-	if (IS_ERR(larval))
+	if (IS_ERR_OR_NULL(larval))
 		goto out;
 
 	list_add(&alg->cra_list, &crypto_alg_list);
@@ -651,7 +651,7 @@ int crypto_register_instance(struct crypto_template *tmpl,
 	inst->alg.cra_flags |= (fips_internal & CRYPTO_ALG_FIPS_INTERNAL);
 
 	larval = __crypto_register_alg(&inst->alg);
-	if (IS_ERR(larval))
+	if (IS_ERR_OR_NULL(larval))
 		goto unlock;
 	else if (larval)
 		larval->test_started = true;
-- 
2.25.1

[PATCH -next 2/2] crypto: crc32c - add missing Kconfig option select

[Gaosheng Cui]

The CRYPTO_CRC32C is using functions provided by CRYPTO_MANAGER,
otherwise the following error will occur:

    EXT4-fs (mmcblk0): Cannot load crc32c driver.

So select CRYPTO_MANAGER when enable CRYPTO_CRC32C.

Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
---
 crypto/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/crypto/Kconfig b/crypto/Kconfig
index b1ccf873779d..7f124604323b 100644
--- a/crypto/Kconfig
+++ b/crypto/Kconfig
@@ -641,6 +641,7 @@ config CRYPTO_CRC32C
 	tristate "CRC32c CRC algorithm"
 	select CRYPTO_HASH
 	select CRC32
+	select CRYPTO_MANAGER
 	help
 	  Castagnoli, et al Cyclic Redundancy-Check Algorithm.  Used
 	  by iSCSI for header and data digests and by others.
-- 
2.25.1

Re: [PATCH -next 1/2] crypto: api - Fix IS_ERR() vs NULL check

[Herbert Xu]

On Thu, Aug 25, 2022 at 04:41:37PM +0800, Gaosheng Cui wrote:
> The crypto_alloc_test_larval() will return null if manager is disabled,
> it may not return error pointers, so using IS_ERR_OR_NULL()
> to check the return value to fix this.
> 
> The __crypto_register_alg() will return null if manager is disabled,
> it may not return error pointers, so using IS_ERR_OR_NULL()
> to check the return value to fix this.
> 
> Fixes: cad439fc040e ("crypto: api - Do not create test larvals if manager is disabled")
> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> ---
>  crypto/algapi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 5c69ff8e8fa5..5a080b8aaa11 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -283,7 +283,7 @@ static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg)
>  	}
>  
>  	larval = crypto_alloc_test_larval(alg);
> -	if (IS_ERR(larval))
> +	if (IS_ERR_OR_NULL(larval))
>  		goto out;

A NULL indicates success, why are you jumping to the error path?

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

Re: [PATCH -next 2/2] crypto: crc32c - add missing Kconfig option select

[Herbert Xu]

On Thu, Aug 25, 2022 at 04:41:38PM +0800, Gaosheng Cui wrote:
> The CRYPTO_CRC32C is using functions provided by CRYPTO_MANAGER,
> otherwise the following error will occur:
> 
>     EXT4-fs (mmcblk0): Cannot load crc32c driver.
> 
> So select CRYPTO_MANAGER when enable CRYPTO_CRC32C.
> 
> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
> ---
>  crypto/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/crypto/Kconfig b/crypto/Kconfig
> index b1ccf873779d..7f124604323b 100644
> --- a/crypto/Kconfig
> +++ b/crypto/Kconfig
> @@ -641,6 +641,7 @@ config CRYPTO_CRC32C
>  	tristate "CRC32c CRC algorithm"
>  	select CRYPTO_HASH
>  	select CRC32
> +	select CRYPTO_MANAGER
>  	help
>  	  Castagnoli, et al Cyclic Redundancy-Check Algorithm.  Used
>  	  by iSCSI for header and data digests and by others.

Why exactly does it need CRYPTO_MANAGER?

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

Re: [PATCH -next 2/2] crypto: crc32c - add missing Kconfig option select

[cuigaosheng]

Thanks for your reply.

While I was debugging the kernel code of linux-next, I start the kernel
with qemu-system-arm with following commands:

     make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- vexpress_defconfig
     make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- -j32
     qemu-system-arm -M vexpress-a9 -m 1024M -s -nographic -kernel arch/arm/boot/zImage \
                    -dtb arch/arm/boot/dts/vexpress-v2p-ca9.dtb -sd /home/rootfs.sd \
                    -append "root=/dev/mmcblk0 rw console=ttyAMA0"

But it failed, so I tried to locate the cause of the failure and finally found that
it failed from this patch(cad439fc040e crypto: api - Do not create test larvals if manager is disabled)，
logs as follows:
> EXT4-fs (mmcblk0): Cannot load crc32c driver. VFS: Cannot open root 
> device "mmcblk0" or unknown-block(179,0): error -80 Please append a 
> correct "root=" boot option; here are the available partitions: 1f00 
> 131072 mtdblock0 (driver?) 1f01 32768 mtdblock1 (driver?) b300 32768 
> mmcblk0 driver: mmcblk Kernel panic - not syncing: VFS: Unable to 
> mount root fs on unknown-block(179,0) CPU: 0 PID: 1 Comm: swapper/0 
> Not tainted 5.15.0-rc1+ #1 Hardware name: ARM-Versatile Express 
> [<8010f334>] (unwind_backtrace) from [<8010b08c>] 
> (show_stack+0x10/0x14) [<8010b08c>] (show_stack) from [<8083f2a4>] 
> (dump_stack_lvl+0x40/0x4c) [<8083f2a4>] (dump_stack_lvl) from 
> [<8083b210>] (panic+0xf8/0x2f4) [<8083b210>] (panic) from [<80b0175c>] 
> (mount_block_root+0x178/0x200) [<80b0175c>] (mount_block_root) from 
> [<80b01bac>] (prepare_namespace+0x150/0x18c) [<80b01bac>] 
> (prepare_namespace) from [<8084384c>] (kernel_init+0x10/0x124) 
> [<8084384c>] (kernel_init) from [<80100130>] (ret_from_fork+0x14/0x24) 
> Exception stack(0x8108bfb0 to 0x8108bff8) bfa0: ???????? ???????? 
> ???????? ???????? bfc0: ???????? ???????? ???????? ???????? ???????? 
> ???????? ???????? ???????? bfe0: ???????? ???????? ???????? ???????? 
> ???????? ???????? ---[ end Kernel panic - not syncing: VFS: Unable to 
> mount root fs on unknown-block(179,0) ]---

In the patch, crypto_alloc_test_larval will return NULL if CONFIG_CRYPTO_MANAGER disabled, so
I checked to see if this change was the cause "EXT4-fs (mmcblk0): Cannot load crc32c driver
", the success logs does not have this error.

When I enabled CONFIG_CRYPTO_MANAGER, kernel can be boot successfully.

Could that be the reason? I would be very grateful if you could give me some advice.

Thanks very much!

在 2022/8/25 16:50, Herbert Xu 写道:
> On Thu, Aug 25, 2022 at 04:41:38PM +0800, Gaosheng Cui wrote:
>> The CRYPTO_CRC32C is using functions provided by CRYPTO_MANAGER,
>> otherwise the following error will occur:
>>
>>      EXT4-fs (mmcblk0): Cannot load crc32c driver.
>>
>> So select CRYPTO_MANAGER when enable CRYPTO_CRC32C.
>>
>> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
>> ---
>>   crypto/Kconfig | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/crypto/Kconfig b/crypto/Kconfig
>> index b1ccf873779d..7f124604323b 100644
>> --- a/crypto/Kconfig
>> +++ b/crypto/Kconfig
>> @@ -641,6 +641,7 @@ config CRYPTO_CRC32C
>>   	tristate "CRC32c CRC algorithm"
>>   	select CRYPTO_HASH
>>   	select CRC32
>> +	select CRYPTO_MANAGER
>>   	help
>>   	  Castagnoli, et al Cyclic Redundancy-Check Algorithm.  Used
>>   	  by iSCSI for header and data digests and by others.
> Why exactly does it need CRYPTO_MANAGER?
>
> Cheers,

Re: [PATCH -next 1/2] crypto: api - Fix IS_ERR() vs NULL check

[cuigaosheng]

Thanks for taking the time to review this patch.

crypto_alloc_test_larval() will return null if manager is disabled,
it will not return error pointers, IS_ERR should not be used to checking
return value, should we fix it? or use another solution?

It would be helpful if you could give some advice or fix the problem by yourself.

Thanks very much!

在 2022/8/25 16:50, Herbert Xu 写道:
> On Thu, Aug 25, 2022 at 04:41:37PM +0800, Gaosheng Cui wrote:
>> The crypto_alloc_test_larval() will return null if manager is disabled,
>> it may not return error pointers, so using IS_ERR_OR_NULL()
>> to check the return value to fix this.
>>
>> The __crypto_register_alg() will return null if manager is disabled,
>> it may not return error pointers, so using IS_ERR_OR_NULL()
>> to check the return value to fix this.
>>
>> Fixes: cad439fc040e ("crypto: api - Do not create test larvals if manager is disabled")
>> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com>
>> ---
>>   crypto/algapi.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/crypto/algapi.c b/crypto/algapi.c
>> index 5c69ff8e8fa5..5a080b8aaa11 100644
>> --- a/crypto/algapi.c
>> +++ b/crypto/algapi.c
>> @@ -283,7 +283,7 @@ static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg)
>>   	}
>>   
>>   	larval = crypto_alloc_test_larval(alg);
>> -	if (IS_ERR(larval))
>> +	if (IS_ERR_OR_NULL(larval))
>>   		goto out;
> A NULL indicates success, why are you jumping to the error path?
>
> Cheers,

Re: [PATCH -next 2/2] crypto: crc32c - add missing Kconfig option select

[Herbert Xu]

On Thu, Aug 25, 2022 at 08:55:12PM +0800, cuigaosheng wrote:
> Thanks for your reply.
> 
> While I was debugging the kernel code of linux-next, I start the kernel
> with qemu-system-arm with following commands:
> 
>     make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- vexpress_defconfig
>     make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi- -j32
>     qemu-system-arm -M vexpress-a9 -m 1024M -s -nographic -kernel arch/arm/boot/zImage \
>                    -dtb arch/arm/boot/dts/vexpress-v2p-ca9.dtb -sd /home/rootfs.sd \
>                    -append "root=/dev/mmcblk0 rw console=ttyAMA0"
> 
> But it failed, so I tried to locate the cause of the failure and finally found that
> it failed from this patch(cad439fc040e crypto: api - Do not create test larvals if manager is disabled)，
> logs as follows:
> > EXT4-fs (mmcblk0): Cannot load crc32c driver. VFS: Cannot open root
> > device "mmcblk0" or unknown-block(179,0): error -80 Please append a
> > correct "root=" boot option; here are the available partitions: 1f00
> > 131072 mtdblock0 (driver?) 1f01 32768 mtdblock1 (driver?) b300 32768
> > mmcblk0 driver: mmcblk Kernel panic - not syncing: VFS: Unable to mount
> > root fs on unknown-block(179,0) CPU: 0 PID: 1 Comm: swapper/0 Not
> > tainted 5.15.0-rc1+ #1 Hardware name: ARM-Versatile Express [<8010f334>]
> > (unwind_backtrace) from [<8010b08c>] (show_stack+0x10/0x14) [<8010b08c>]
> > (show_stack) from [<8083f2a4>] (dump_stack_lvl+0x40/0x4c) [<8083f2a4>]
> > (dump_stack_lvl) from [<8083b210>] (panic+0xf8/0x2f4) [<8083b210>]
> > (panic) from [<80b0175c>] (mount_block_root+0x178/0x200) [<80b0175c>]
> > (mount_block_root) from [<80b01bac>] (prepare_namespace+0x150/0x18c)
> > [<80b01bac>] (prepare_namespace) from [<8084384c>]
> > (kernel_init+0x10/0x124) [<8084384c>] (kernel_init) from [<80100130>]
> > (ret_from_fork+0x14/0x24) Exception stack(0x8108bfb0 to 0x8108bff8)
> > bfa0: ???????? ???????? ???????? ???????? bfc0: ???????? ????????
> > ???????? ???????? ???????? ???????? ???????? ???????? bfe0: ????????
> > ???????? ???????? ???????? ???????? ???????? ---[ end Kernel panic - not
> > syncing: VFS: Unable to mount root fs on unknown-block(179,0) ]---
> 
> In the patch, crypto_alloc_test_larval will return NULL if CONFIG_CRYPTO_MANAGER disabled, so
> I checked to see if this change was the cause "EXT4-fs (mmcblk0): Cannot load crc32c driver
> ", the success logs does not have this error.
> 
> When I enabled CONFIG_CRYPTO_MANAGER, kernel can be boot successfully.
> 
> Could that be the reason? I would be very grateful if you could give me some advice.

Can you please provide the whole .config file?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH -next 1/2] crypto: api - Fix IS_ERR() vs NULL check

[Herbert Xu]

On Thu, Aug 25, 2022 at 09:10:49PM +0800, cuigaosheng wrote:
> Thanks for taking the time to review this patch.
> 
> crypto_alloc_test_larval() will return null if manager is disabled,
> it will not return error pointers, IS_ERR should not be used to checking
> return value, should we fix it? or use another solution?

That's because NULL is returned indicating success.  When a genuine
error occurs then an error pointer will be returned.  IS_ERR will be
true only in case of a genuine error.  It will be false when either
NULL or a real larval pointer is returned.

You need to describe your problem more clearly as I have no idea what
you're trying to fix.

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