From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Geert Uytterhoeven <geert@linux-m68k.org>,
        Herbert Xu <herbert@gondor.apana.org.au>
CC:     Nathan Chancellor <nathan@kernel.org>,
        Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
        Vladis Dronov <vdronov@redhat.com>,
        Simo Sorce <ssorce@redhat.com>,
        Eric Biggers <ebiggers@kernel.org>,
        "llvm@lists.linux.dev" <llvm@lists.linux.dev>,
        kernel test robot <lkp@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] crypto: api - Do not create test larvals if manager is
 disabled
Thread-Topic: [PATCH] crypto: api - Do not create test larvals if manager is
 disabled
Thread-Index: AQHX0AAsn8beV0dNDkOTE4cq4i4dGw==
Date:   Thu, 4 Nov 2021 07:28:51 +0000
Message-ID: <DM6PR04MB708155E447FD9A79AB89686DE78D9@DM6PR04MB7081.namprd04.prod.outlook.com>
References: <20210913071251.GA15235@gondor.apana.org.au>
 <20210917002619.GA6407@gondor.apana.org.au>
 <YVNfqUVJ7w4Z3WXK@archlinux-ax161>
 <20211001055058.GA6081@gondor.apana.org.au>
 <YVdNFzs8HUQwHa54@archlinux-ax161>
 <20211003002801.GA5435@gondor.apana.org.au>
 <YV0K+EbrAqDdw2vp@archlinux-ax161>
 <20211019132802.GA14233@gondor.apana.org.au>
 <alpine.DEB.2.22.394.2111021636040.2330984@ramsan.of.borg>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?jgYsW8AOdlRXByZJmR81SL7udqrHhkga5jBfnWlPkvdOAJbH2lZFlefV+zb/?=
 =?us-ascii?Q?yQXFqsfLzMoxFBQ9RJHtv3KxKoVcPLAA4LsZyjdAB/mlJaFe5oGzG0FVlSRo?=
 =?us-ascii?Q?I84LEIgAo5imtHJU7BNOECU3YbbCFMvMG0D68lZB9fdLe8nDgjOorD9+e6Gs?=
 =?us-ascii?Q?jF1J2wFujU+ASsPOId8U/89TJ/wkKRjujecfDX7GAYq+ogQasCXeOcdal404?=
 =?us-ascii?Q?JtHH8SIPZ6yXckXQ/n89ecHxxiWFAK691Qf31SQE+jRbcGu5jAVZG5Qk5QyO?=
 =?us-ascii?Q?H8+clxaA8aAh9LE1lh0Dz60LNjDJ9fuALfMJs71WVwbfVcRkTZxBPhnY3tzs?=
 =?us-ascii?Q?eJhqTd0YLbg1IT/ZvR2xjq6EYVjGcjznSf3AL3Oj+BMmYKfuvpOXvPVp3QQH?=
 =?us-ascii?Q?Aq8udC0bgeSbhqLzNQXxNX6ABXlukFsYey++I9xSOuJMbdEZ+wdpnZJ1n6Ub?=
 =?us-ascii?Q?63J5+gI5jgQxFMahn07aaJS/pbZImXdQrUXxCVexwocHxzsTSd2v4YzDYcz4?=
 =?us-ascii?Q?tFR6e67hmr0graagk+/gjpmkMo6492T9GLewlEcnUH519sO6ZxAw1A1cnFxL?=
 =?us-ascii?Q?ZfadG2dzHC04NnZTuZ784dkuWLSJ1rGuJ/zofeBYn/4aQUOauFH6y3Yktklz?=
 =?us-ascii?Q?4/AotcEX16veXmwpEnEbvM/n9ONA71krqV+WQCdwdwtj9DErrOQoSlRQtT69?=
 =?us-ascii?Q?BMjbafxdNndfuzaQRUSgQQP9YqlTrAw1EPwGQykY4s/Lvdsg7QmP0mOk2rU7?=
 =?us-ascii?Q?fHN4yIFXWZQd8eny3Ix7EEJKDLBUG2sxfun0pXHWmFVHSmEvG+Dvp4Dy9cFP?=
 =?us-ascii?Q?vdi1LSe2eLWhzDmA6QB0ud8one2sjrDdvkJ6f32MzprItQ+pSsh62i2m8o/7?=
 =?us-ascii?Q?7iqqvhzbz/PIr2ZR97h/piD0tMM8C6cRkrCF7Xq57XsMiMAik3FnA9va5htG?=
 =?us-ascii?Q?e4a0I84f3aEFeyXshX0jDgW8dRILY0m5+qf96FfUOFhUxmHYkOBVGZwHX7Ci?=
 =?us-ascii?Q?x2QEClSgZvXAIVQpL/NFO9u4iufXCQHgZLtYomVOTpEqwsM0Qmu2Y0HwGObk?=
 =?us-ascii?Q?dgk3PaTVzxc3diDqX3aG4BZN2r2zpnHMiMII/OmEX8tCTwAvp2yyGZ4pLJvJ?=
 =?us-ascii?Q?lTQjgRhhW3h45MDMw6NGYx0/yQ96V/ZpBFwGp5G3Dn4a8XktTAspVWMLd6mH?=
 =?us-ascii?Q?8uPbAW//Nvp26dH5M5Pwg7SAIwWzjrJa7+S09hDVtPvfey+QwIi8TA1HoitZ?=
 =?us-ascii?Q?XlTGYxuQzZ2EqnWFv1eptdaFqXzaFYnEMiUvuV3cNkNMhM0ifECplZRDJn3g?=
 =?us-ascii?Q?g5UbB6VMCY1od4CtuAMnbcmKwd/fP6fsq6rf/1caGuLrBINcq4tQX7VRvqEJ?=
 =?us-ascii?Q?2SB9cHcp6gexI9PnKWLZJTbN8uedImteRu9zsi7iuxI0sNZ64KQREdtANgRJ?=
 =?us-ascii?Q?QJsjHVGNhsecA/NiqY0Xy9GEaKeqZVryGuPoZTM+CZBlvlvius5pfJ9/fGPx?=
 =?us-ascii?Q?y9FdVbnC0hUZk2cBxsgu7SI01g7NFKzd6vaSkhO/AIhQax1t8DTva7tG2hkC?=
 =?us-ascii?Q?+qCSkuc852vM5nqRA5hGyKstvFemHG/yJ4c/2NCraWKVtoaK/8MfCZgKtq1h?=
 =?us-ascii?Q?o3VEYLh/fBZKk5dN2DogtqtFnj22WceurWdslYTJgPjJGjS5qT+voHAv2JW5?=
 =?us-ascii?Q?wIQ2HgA9ERz+xM+CXrKu8Pk+wVmixY1d8tChNrRBNGQ8t8Qi?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: wdc.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR04MB7081.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 80f1ef62-ca89-4f95-1213-08d99f64bfc9
X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Nov 2021 07:28:51.2140
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 9T1auCOgn1ehAoSjRjmcZjgoMN1idw+ux1p1aqIizNlHu6VYrTHTa5ObBbCk8ns4V0WUsSutDukt0QsrHppvkw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR04MB1243
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2021/11/03 0:41, Geert Uytterhoeven wrote:=0A=
>  	Hi Herbert,=0A=
> =0A=
> On Tue, 19 Oct 2021, Herbert Xu wrote:=0A=
>> On Tue, Oct 05, 2021 at 07:33:28PM -0700, Nathan Chancellor wrote:=0A=
>>> I assume this is the diff you mean? This does not resolve the issue. My=
=0A=
>>> apologies if I am slow to respond, I am on vacation until the middle of=
=0A=
>>> next week.=0A=
>>=0A=
>> Sorry for the delay.  The kernel robot figured out the problem=0A=
>> for me.  It's the crypto_alg_tested call that causes api.c to=0A=
>> depend on algapi.c.  This call is only invoked in the case where=0A=
>> the crypto manager is turned off.  We could instead simply make=0A=
>> test larvals disappear in that case.=0A=
>>=0A=
>> ---8<---=0A=
>> The delayed boot-time testing patch created a dependency loop=0A=
>> between api.c and algapi.c because it added a crypto_alg_tested=0A=
>> call to the former when the crypto manager is disabled.=0A=
>>=0A=
>> We could instead avoid creating the test larvals if the crypto=0A=
>> manager is disabled.  This avoids the dependency loop as well=0A=
>> as saving some unnecessary work, albeit in a very unlikely case.=0A=
>>=0A=
>> Reported-by: Nathan Chancellor <nathan@kernel.org>=0A=
>> Reported-by: Naresh Kamboju <naresh.kamboju@linaro.org>=0A=
>> Reported-by: kernel test robot <lkp@intel.com>=0A=
>> Fixes: adad556efcdd ("crypto: api - Fix built-in testing dependency fail=
ures")=0A=
>> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>=0A=
> =0A=
> Thanks for your patch, which is now commit cad439fc040efe5f=0A=
> ("crypto: api - Do not create test larvals if manager is disabled").=0A=
> =0A=
> I have bisected a failure to mount the root file system on k210 to this=
=0A=
> commit.=0A=
> =0A=
> Dmesg before/after:=0A=
> =0A=
>       mmcblk0: mmc0:0000 SA04G 3.68 GiB=0A=
>       random: fast init done=0A=
>        mmcblk0: p1=0A=
>      -EXT4-fs (mmcblk0p1): mounted filesystem with ordered data mode. Opt=
s: (null). Quota mode: disabled.=0A=
>      -VFS: Mounted root (ext4 filesystem) readonly on device 179:1.=0A=
>      +EXT4-fs (mmcblk0p1): Cannot load crc32c driver.=0A=
>      +VFS: Cannot open root device "mmcblk0p1" or unknown-block(179,1): e=
rror -80=0A=
=0A=
p1 exist as the message 2 lines above shows. And since the mount error is -=
80=0A=
(ELIBBAD), it is really all about crypto. Since the default k210 config com=
pile=0A=
everything in-kernel (no modules), it should work. Was crc32c compiled as a=
=0A=
module ? If yes, then the k210 will need to be booted with U-Boot and use a=
 real=0A=
initrd, which likely will all end-up in a no memory situation. ext4 in itse=
lf=0A=
will consume way too much memory...=0A=
=0A=
>      +Please append a correct "root=3D" boot option; here are the availab=
le partitions:=0A=
>      +b300         3858432 mmcblk0=0A=
>      + driver: mmcblk=0A=
>      +  b301         3854336 mmcblk0p1 00000000-01=0A=
> =0A=
>> --- a/crypto/algapi.c=0A=
>> +++ b/crypto/algapi.c=0A=
>> @@ -216,6 +216,32 @@ void crypto_remove_spawns(struct crypto_alg *alg, s=
truct list_head *list,=0A=
>> }=0A=
>> EXPORT_SYMBOL_GPL(crypto_remove_spawns);=0A=
>>=0A=
>> +static struct crypto_larval *crypto_alloc_test_larval(struct crypto_alg=
 *alg)=0A=
>> +{=0A=
>> +	struct crypto_larval *larval;=0A=
>> +=0A=
>> +	if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER))=0A=
>> +		return NULL;=0A=
>> +=0A=
>> +	larval =3D crypto_larval_alloc(alg->cra_name,=0A=
>> +				     alg->cra_flags | CRYPTO_ALG_TESTED, 0);=0A=
>> +	if (IS_ERR(larval))=0A=
>> +		return larval;=0A=
>> +=0A=
>> +	larval->adult =3D crypto_mod_get(alg);=0A=
>> +	if (!larval->adult) {=0A=
>> +		kfree(larval);=0A=
>> +		return ERR_PTR(-ENOENT);=0A=
>> +	}=0A=
>> +=0A=
>> +	refcount_set(&larval->alg.cra_refcnt, 1);=0A=
>> +	memcpy(larval->alg.cra_driver_name, alg->cra_driver_name,=0A=
>> +	       CRYPTO_MAX_ALG_NAME);=0A=
>> +	larval->alg.cra_priority =3D alg->cra_priority;=0A=
>> +=0A=
>> +	return larval;=0A=
>> +}=0A=
>> +=0A=
>> static struct crypto_larval *__crypto_register_alg(struct crypto_alg *al=
g)=0A=
>> {=0A=
>> 	struct crypto_alg *q;=0A=
>> @@ -250,31 +276,20 @@ static struct crypto_larval *__crypto_register_alg=
(struct crypto_alg *alg)=0A=
>> 			goto err;=0A=
>> 	}=0A=
>>=0A=
>> -	larval =3D crypto_larval_alloc(alg->cra_name,=0A=
>> -				     alg->cra_flags | CRYPTO_ALG_TESTED, 0);=0A=
>> +	larval =3D crypto_alloc_test_larval(alg);=0A=
>> 	if (IS_ERR(larval))=0A=
>> 		goto out;=0A=
>>=0A=
>> -	ret =3D -ENOENT;=0A=
>> -	larval->adult =3D crypto_mod_get(alg);=0A=
>> -	if (!larval->adult)=0A=
>> -		goto free_larval;=0A=
>> -=0A=
>> -	refcount_set(&larval->alg.cra_refcnt, 1);=0A=
>> -	memcpy(larval->alg.cra_driver_name, alg->cra_driver_name,=0A=
>> -	       CRYPTO_MAX_ALG_NAME);=0A=
>> -	larval->alg.cra_priority =3D alg->cra_priority;=0A=
>> -=0A=
>> 	list_add(&alg->cra_list, &crypto_alg_list);=0A=
>> -	list_add(&larval->alg.cra_list, &crypto_alg_list);=0A=
>> +=0A=
>> +	if (larval)=0A=
>> +		list_add(&larval->alg.cra_list, &crypto_alg_list);=0A=
>>=0A=
>> 	crypto_stats_init(alg);=0A=
>>=0A=
>> out:=0A=
>> 	return larval;=0A=
>>=0A=
>> -free_larval:=0A=
>> -	kfree(larval);=0A=
>> err:=0A=
>> 	larval =3D ERR_PTR(ret);=0A=
>> 	goto out;=0A=
>> @@ -403,10 +418,11 @@ int crypto_register_alg(struct crypto_alg *alg)=0A=
>> 	down_write(&crypto_alg_sem);=0A=
>> 	larval =3D __crypto_register_alg(alg);=0A=
>> 	test_started =3D static_key_enabled(&crypto_boot_test_finished);=0A=
>> -	larval->test_started =3D test_started;=0A=
>> +	if (!IS_ERR_OR_NULL(larval))=0A=
>> +		larval->test_started =3D test_started;=0A=
>> 	up_write(&crypto_alg_sem);=0A=
>>=0A=
>> -	if (IS_ERR(larval))=0A=
>> +	if (IS_ERR_OR_NULL(larval))=0A=
>> 		return PTR_ERR(larval);=0A=
>>=0A=
>> 	if (test_started)=0A=
>> @@ -616,8 +632,8 @@ int crypto_register_instance(struct crypto_template =
*tmpl,=0A=
>> 	larval =3D __crypto_register_alg(&inst->alg);=0A=
>> 	if (IS_ERR(larval))=0A=
>> 		goto unlock;=0A=
>> -=0A=
>> -	larval->test_started =3D true;=0A=
>> +	else if (larval)=0A=
>> +		larval->test_started =3D true;=0A=
>>=0A=
>> 	hlist_add_head(&inst->list, &tmpl->instances);=0A=
>> 	inst->tmpl =3D tmpl;=0A=
>> @@ -626,7 +642,7 @@ int crypto_register_instance(struct crypto_template =
*tmpl,=0A=
>> 	up_write(&crypto_alg_sem);=0A=
>>=0A=
>> 	err =3D PTR_ERR(larval);=0A=
>> -	if (IS_ERR(larval))=0A=
>> +	if (IS_ERR_OR_NULL(larval))=0A=
>> 		goto err;=0A=
>>=0A=
>> 	crypto_wait_for_test(larval);=0A=
>> diff --git a/crypto/api.c b/crypto/api.c=0A=
>> index ee5991fe11f8..cf0869dd130b 100644=0A=
>> --- a/crypto/api.c=0A=
>> +++ b/crypto/api.c=0A=
>> @@ -167,11 +167,8 @@ void crypto_wait_for_test(struct crypto_larval *lar=
val)=0A=
>> 	int err;=0A=
>>=0A=
>> 	err =3D crypto_probing_notify(CRYPTO_MSG_ALG_REGISTER, larval->adult);=
=0A=
>> -	if (err !=3D NOTIFY_STOP) {=0A=
>> -		if (WARN_ON(err !=3D NOTIFY_DONE))=0A=
>> -			goto out;=0A=
>> -		crypto_alg_tested(larval->alg.cra_driver_name, 0);=0A=
>> -	}=0A=
>> +	if (WARN_ON_ONCE(err !=3D NOTIFY_STOP))=0A=
>> +		goto out;=0A=
>>=0A=
>> 	err =3D wait_for_completion_killable(&larval->completion);=0A=
>> 	WARN_ON(err);=0A=
> =0A=
> Gr{oetje,eeting}s,=0A=
> =0A=
>  						Geert=0A=
> =0A=
> --=0A=
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m6=
8k.org=0A=
> =0A=
> In personal conversations with technical people, I call myself a hacker. =
But=0A=
> when I'm talking to journalists I just say "programmer" or something like=
 that.=0A=
>  							    -- Linus Torvalds=0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=