diff mbox series

[v1,2/2] certs: Remove panic() calls from system_trusted_keyring_init()

Message ID 20220311174741.250424-3-mic@digikod.net (mailing list archive)
State New
Headers show
Series Remove panic() from keyring init calls | expand

Commit Message

Mickaël Salaün March 11, 2022, 5:47 p.m. UTC 
From: Mickaël Salaün <mic@linux.microsoft.com>

Replace panic() calls from device_initcall(system_trusted_keyring_init)
with proper error handling using -ENODEV.

Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
Link: https://lore.kernel.org/r/20220311174741.250424-3-mic@digikod.net
---
 certs/system_keyring.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

Comments

Jarkko Sakkinen March 17, 2022, 7:36 a.m. UTC | #1 
On Fri, Mar 11, 2022 at 06:47:41PM +0100, Mickaël Salaün wrote:
> From: Mickaël Salaün <mic@linux.microsoft.com>
> 
> Replace panic() calls from device_initcall(system_trusted_keyring_init)
> with proper error handling using -ENODEV.
> 
> Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
> Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> Link: https://lore.kernel.org/r/20220311174741.250424-3-mic@digikod.net
> ---
>  certs/system_keyring.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 05b66ce9d1c9..428046a7aa7f 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -148,8 +148,10 @@ static __init int system_trusted_keyring_init(void)
>  			      KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
>  			      KEY_ALLOC_NOT_IN_QUOTA,
>  			      NULL, NULL);
> -	if (IS_ERR(builtin_trusted_keys))
> -		panic("Can't allocate builtin trusted keyring\n");
> +	if (IS_ERR(builtin_trusted_keys)) {
> +		pr_err("Can't allocate builtin trusted keyring\n");
> +		return -ENODEV;
> +	}
>  
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>  	secondary_trusted_keys =
> @@ -161,14 +163,26 @@ static __init int system_trusted_keyring_init(void)
>  			      KEY_ALLOC_NOT_IN_QUOTA,
>  			      get_builtin_and_secondary_restriction(),
>  			      NULL);
> -	if (IS_ERR(secondary_trusted_keys))
> -		panic("Can't allocate secondary trusted keyring\n");
> +	if (IS_ERR(secondary_trusted_keys)) {
> +		pr_err("Can't allocate secondary trusted keyring\n");
> +		goto err_secondary;
> +	}
>  
> -	if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
> -		panic("Can't link trusted keyrings\n");
> +	if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0) {
> +		pr_err("Can't link trusted keyrings\n");
> +		goto err_link;
> +	}
>  #endif
>  
>  	return 0;
> +
> +err_link:
> +	key_put(secondary_trusted_keys);
> +
> +err_secondary:
> +	key_put(builtin_trusted_keys);
> +
> +	return -ENODEV;
>  }
>  
>  /*
> -- 
> 2.35.1
> 

Changes make sense to me but you should implement all this to the original
patch set.

BR, Jarkko
Mickaël Salaün March 17, 2022, 8:30 a.m. UTC | #2 
On 17/03/2022 08:36, Jarkko Sakkinen wrote:
> On Fri, Mar 11, 2022 at 06:47:41PM +0100, Mickaël Salaün wrote:
>> From: Mickaël Salaün <mic@linux.microsoft.com>
>>
>> Replace panic() calls from device_initcall(system_trusted_keyring_init)
>> with proper error handling using -ENODEV.
>>
>> Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
>> Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>> Link: https://lore.kernel.org/r/20220311174741.250424-3-mic@digikod.net
>> ---
>>   certs/system_keyring.c | 26 ++++++++++++++++++++------
>>   1 file changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index 05b66ce9d1c9..428046a7aa7f 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -148,8 +148,10 @@ static __init int system_trusted_keyring_init(void)
>>   			      KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
>>   			      KEY_ALLOC_NOT_IN_QUOTA,
>>   			      NULL, NULL);
>> -	if (IS_ERR(builtin_trusted_keys))
>> -		panic("Can't allocate builtin trusted keyring\n");
>> +	if (IS_ERR(builtin_trusted_keys)) {
>> +		pr_err("Can't allocate builtin trusted keyring\n");
>> +		return -ENODEV;
>> +	}
>>   
>>   #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>>   	secondary_trusted_keys =
>> @@ -161,14 +163,26 @@ static __init int system_trusted_keyring_init(void)
>>   			      KEY_ALLOC_NOT_IN_QUOTA,
>>   			      get_builtin_and_secondary_restriction(),
>>   			      NULL);
>> -	if (IS_ERR(secondary_trusted_keys))
>> -		panic("Can't allocate secondary trusted keyring\n");
>> +	if (IS_ERR(secondary_trusted_keys)) {
>> +		pr_err("Can't allocate secondary trusted keyring\n");
>> +		goto err_secondary;
>> +	}
>>   
>> -	if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
>> -		panic("Can't link trusted keyrings\n");
>> +	if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0) {
>> +		pr_err("Can't link trusted keyrings\n");
>> +		goto err_link;
>> +	}
>>   #endif
>>   
>>   	return 0;
>> +
>> +err_link:
>> +	key_put(secondary_trusted_keys);
>> +
>> +err_secondary:
>> +	key_put(builtin_trusted_keys);
>> +
>> +	return -ENODEV;
>>   }
>>   
>>   /*
>> -- 
>> 2.35.1
>>
> 
> Changes make sense to me but you should implement all this to the original
> patch set.

You agreed to add this patch on top of the others a few days ago: 
https://lore.kernel.org/r/f8b1ea77afe8d6698b4a2122254ff8be310412b1.camel@kernel.org

What do you think about Paul's concerns?
Mickaël Salaün March 17, 2022, 8:31 a.m. UTC | #3 
On 17/03/2022 09:30, Mickaël Salaün wrote:
> 
> On 17/03/2022 08:36, Jarkko Sakkinen wrote:
>> On Fri, Mar 11, 2022 at 06:47:41PM +0100, Mickaël Salaün wrote:
>>> From: Mickaël Salaün <mic@linux.microsoft.com>
>>>
>>> Replace panic() calls from device_initcall(system_trusted_keyring_init)
>>> with proper error handling using -ENODEV.
>>>
>>> Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
>>> Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
>>> Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
>>> Link: https://lore.kernel.org/r/20220311174741.250424-3-mic@digikod.net
>>> ---
>>>   certs/system_keyring.c | 26 ++++++++++++++++++++------
>>>   1 file changed, 20 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>>> index 05b66ce9d1c9..428046a7aa7f 100644
>>> --- a/certs/system_keyring.c
>>> +++ b/certs/system_keyring.c
>>> @@ -148,8 +148,10 @@ static __init int system_trusted_keyring_init(void)
>>>                     KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
>>>                     KEY_ALLOC_NOT_IN_QUOTA,
>>>                     NULL, NULL);
>>> -    if (IS_ERR(builtin_trusted_keys))
>>> -        panic("Can't allocate builtin trusted keyring\n");
>>> +    if (IS_ERR(builtin_trusted_keys)) {
>>> +        pr_err("Can't allocate builtin trusted keyring\n");
>>> +        return -ENODEV;
>>> +    }
>>>   #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>>>       secondary_trusted_keys =
>>> @@ -161,14 +163,26 @@ static __init int 
>>> system_trusted_keyring_init(void)
>>>                     KEY_ALLOC_NOT_IN_QUOTA,
>>>                     get_builtin_and_secondary_restriction(),
>>>                     NULL);
>>> -    if (IS_ERR(secondary_trusted_keys))
>>> -        panic("Can't allocate secondary trusted keyring\n");
>>> +    if (IS_ERR(secondary_trusted_keys)) {
>>> +        pr_err("Can't allocate secondary trusted keyring\n");
>>> +        goto err_secondary;
>>> +    }
>>> -    if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
>>> -        panic("Can't link trusted keyrings\n");
>>> +    if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0) {
>>> +        pr_err("Can't link trusted keyrings\n");
>>> +        goto err_link;
>>> +    }
>>>   #endif
>>>       return 0;
>>> +
>>> +err_link:
>>> +    key_put(secondary_trusted_keys);
>>> +
>>> +err_secondary:
>>> +    key_put(builtin_trusted_keys);
>>> +
>>> +    return -ENODEV;
>>>   }
>>>   /*
>>> -- 
>>> 2.35.1
>>>
>>
>> Changes make sense to me but you should implement all this to the 
>> original
>> patch set.

Do you mean to squash these two patches together?

> 
> You agreed to add this patch on top of the others a few days ago: 
> https://lore.kernel.org/r/f8b1ea77afe8d6698b4a2122254ff8be310412b1.camel@kernel.org 
> 
> 
> What do you think about Paul's concerns?
Jarkko Sakkinen March 20, 2022, 9:06 p.m. UTC | #4 
On Thu, Mar 17, 2022 at 09:30:02AM +0100, Mickaël Salaün wrote:
> 
> On 17/03/2022 08:36, Jarkko Sakkinen wrote:
> > On Fri, Mar 11, 2022 at 06:47:41PM +0100, Mickaël Salaün wrote:
> > > From: Mickaël Salaün <mic@linux.microsoft.com>
> > > 
> > > Replace panic() calls from device_initcall(system_trusted_keyring_init)
> > > with proper error handling using -ENODEV.
> > > 
> > > Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
> > > Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
> > > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> > > Link: https://lore.kernel.org/r/20220311174741.250424-3-mic@digikod.net
> > > ---
> > >   certs/system_keyring.c | 26 ++++++++++++++++++++------
> > >   1 file changed, 20 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > > index 05b66ce9d1c9..428046a7aa7f 100644
> > > --- a/certs/system_keyring.c
> > > +++ b/certs/system_keyring.c
> > > @@ -148,8 +148,10 @@ static __init int system_trusted_keyring_init(void)
> > >   			      KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
> > >   			      KEY_ALLOC_NOT_IN_QUOTA,
> > >   			      NULL, NULL);
> > > -	if (IS_ERR(builtin_trusted_keys))
> > > -		panic("Can't allocate builtin trusted keyring\n");
> > > +	if (IS_ERR(builtin_trusted_keys)) {
> > > +		pr_err("Can't allocate builtin trusted keyring\n");
> > > +		return -ENODEV;
> > > +	}
> > >   #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> > >   	secondary_trusted_keys =
> > > @@ -161,14 +163,26 @@ static __init int system_trusted_keyring_init(void)
> > >   			      KEY_ALLOC_NOT_IN_QUOTA,
> > >   			      get_builtin_and_secondary_restriction(),
> > >   			      NULL);
> > > -	if (IS_ERR(secondary_trusted_keys))
> > > -		panic("Can't allocate secondary trusted keyring\n");
> > > +	if (IS_ERR(secondary_trusted_keys)) {
> > > +		pr_err("Can't allocate secondary trusted keyring\n");
> > > +		goto err_secondary;
> > > +	}
> > > -	if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
> > > -		panic("Can't link trusted keyrings\n");
> > > +	if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0) {
> > > +		pr_err("Can't link trusted keyrings\n");
> > > +		goto err_link;
> > > +	}
> > >   #endif
> > >   	return 0;
> > > +
> > > +err_link:
> > > +	key_put(secondary_trusted_keys);
> > > +
> > > +err_secondary:
> > > +	key_put(builtin_trusted_keys);
> > > +
> > > +	return -ENODEV;
> > >   }
> > >   /*
> > > -- 
> > > 2.35.1
> > > 
> > 
> > Changes make sense to me but you should implement all this to the original
> > patch set.
> 
> You agreed to add this patch on top of the others a few days ago: https://lore.kernel.org/r/f8b1ea77afe8d6698b4a2122254ff8be310412b1.camel@kernel.org
> 
> What do you think about Paul's concerns?

Yes, but I missed this part.

I think the right call would be to include Paul's concerns documented.

BR, Jarkko
Jarkko Sakkinen March 20, 2022, 9:07 p.m. UTC | #5 
On Thu, Mar 17, 2022 at 09:31:10AM +0100, Mickaël Salaün wrote:
> 
> On 17/03/2022 09:30, Mickaël Salaün wrote:
> > 
> > On 17/03/2022 08:36, Jarkko Sakkinen wrote:
> > > On Fri, Mar 11, 2022 at 06:47:41PM +0100, Mickaël Salaün wrote:
> > > > From: Mickaël Salaün <mic@linux.microsoft.com>
> > > > 
> > > > Replace panic() calls from device_initcall(system_trusted_keyring_init)
> > > > with proper error handling using -ENODEV.
> > > > 
> > > > Suggested-by: Jarkko Sakkinen <jarkko@kernel.org> [1]
> > > > Link: https://lore.kernel.org/r/Yik0C2t7G272YZ73@iki.fi [1]
> > > > Signed-off-by: Mickaël Salaün <mic@linux.microsoft.com>
> > > > Link: https://lore.kernel.org/r/20220311174741.250424-3-mic@digikod.net
> > > > ---
> > > >   certs/system_keyring.c | 26 ++++++++++++++++++++------
> > > >   1 file changed, 20 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> > > > index 05b66ce9d1c9..428046a7aa7f 100644
> > > > --- a/certs/system_keyring.c
> > > > +++ b/certs/system_keyring.c
> > > > @@ -148,8 +148,10 @@ static __init int system_trusted_keyring_init(void)
> > > >                     KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
> > > >                     KEY_ALLOC_NOT_IN_QUOTA,
> > > >                     NULL, NULL);
> > > > -    if (IS_ERR(builtin_trusted_keys))
> > > > -        panic("Can't allocate builtin trusted keyring\n");
> > > > +    if (IS_ERR(builtin_trusted_keys)) {
> > > > +        pr_err("Can't allocate builtin trusted keyring\n");
> > > > +        return -ENODEV;
> > > > +    }
> > > >   #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> > > >       secondary_trusted_keys =
> > > > @@ -161,14 +163,26 @@ static __init int
> > > > system_trusted_keyring_init(void)
> > > >                     KEY_ALLOC_NOT_IN_QUOTA,
> > > >                     get_builtin_and_secondary_restriction(),
> > > >                     NULL);
> > > > -    if (IS_ERR(secondary_trusted_keys))
> > > > -        panic("Can't allocate secondary trusted keyring\n");
> > > > +    if (IS_ERR(secondary_trusted_keys)) {
> > > > +        pr_err("Can't allocate secondary trusted keyring\n");
> > > > +        goto err_secondary;
> > > > +    }
> > > > -    if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
> > > > -        panic("Can't link trusted keyrings\n");
> > > > +    if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0) {
> > > > +        pr_err("Can't link trusted keyrings\n");
> > > > +        goto err_link;
> > > > +    }
> > > >   #endif
> > > >       return 0;
> > > > +
> > > > +err_link:
> > > > +    key_put(secondary_trusted_keys);
> > > > +
> > > > +err_secondary:
> > > > +    key_put(builtin_trusted_keys);
> > > > +
> > > > +    return -ENODEV;
> > > >   }
> > > >   /*
> > > > -- 
> > > > 2.35.1
> > > > 
> > > 
> > > Changes make sense to me but you should implement all this to the
> > > original
> > > patch set.
> 
> Do you mean to squash these two patches together?

You could squash function documentation to the corresponding
patch addressing the use of panic() so that we know why things
are done against normal status quo.

BR, Jarkko
diff mbox series

Patch

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 05b66ce9d1c9..428046a7aa7f 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -148,8 +148,10 @@  static __init int system_trusted_keyring_init(void)
 			      KEY_USR_VIEW | KEY_USR_READ | KEY_USR_SEARCH),
 			      KEY_ALLOC_NOT_IN_QUOTA,
 			      NULL, NULL);
-	if (IS_ERR(builtin_trusted_keys))
-		panic("Can't allocate builtin trusted keyring\n");
+	if (IS_ERR(builtin_trusted_keys)) {
+		pr_err("Can't allocate builtin trusted keyring\n");
+		return -ENODEV;
+	}
 
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 	secondary_trusted_keys =
@@ -161,14 +163,26 @@  static __init int system_trusted_keyring_init(void)
 			      KEY_ALLOC_NOT_IN_QUOTA,
 			      get_builtin_and_secondary_restriction(),
 			      NULL);
-	if (IS_ERR(secondary_trusted_keys))
-		panic("Can't allocate secondary trusted keyring\n");
+	if (IS_ERR(secondary_trusted_keys)) {
+		pr_err("Can't allocate secondary trusted keyring\n");
+		goto err_secondary;
+	}
 
-	if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0)
-		panic("Can't link trusted keyrings\n");
+	if (key_link(secondary_trusted_keys, builtin_trusted_keys) < 0) {
+		pr_err("Can't link trusted keyrings\n");
+		goto err_link;
+	}
 #endif
 
 	return 0;
+
+err_link:
+	key_put(secondary_trusted_keys);
+
+err_secondary:
+	key_put(builtin_trusted_keys);
+
+	return -ENODEV;
 }
 
 /*