On Tue, Jun 06, 2017 at 07:47:55PM +0200, Jason A. Donenfeld wrote:
> -static inline void key_alloc_serial(struct key *key)
> +static inline int key_alloc_serial(struct key *key)
> @@ -170,7 +168,7 @@ static inline void key_alloc_serial(struct key *key)
>       rb_insert_color(&key->serial_node, &key_serial_tree);
>  
>       spin_unlock(&key_serial_lock);
> -     return;
> +     return 0;
>  
>       /* we found a key with the proposed serial number - walk the tree from
>        * that point looking for the next unused serial number */

> @@ -314,7 +312,9 @@ struct key *key_alloc(struct key_type *type, const char 
> *desc,
>  
>       /* publish the key by giving it a serial number */
>       atomic_inc(&user->nkeys);
> -     key_alloc_serial(key);
> +     ret = key_alloc_serial(key);
> +     if (ret < 0)
> +             goto security_error;
>  
>  error:
>       return key;

I'm guessing you changed key_alloc_serial() to return an int back when
you were thinking that you might use get_random_bytes_wait(), which
could return -ERESTARTSYS.

Now that you're not doing this, but using get_random_u32() instead,
there's no point to change the function signature of
key_alloc_serial() and add an error check in key_alloc() that will
never fail, right?  That's just adding a dead code path.  Which the
compiler can probably optimize away, but why make the code slightly
harder to read than necessasry?

                                                - Ted

Reply via email to