Hi,
Thanks for your patch, please see my comments below.
On Tue, Feb 11, 2014 at 12:43:13PM +0200, Cristian Stoica wrote:
> If 'open /dev/crypto' fails, all allocated resources must be freed
> before "open" returns; "close" can't be called to clean-up since
> there is no file descriptor after a failed "open".
>
> Signed-off-by: Cristian Stoica <cristian.sto...@freescale.com>
> ---
> ioctl.c | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/ioctl.c b/ioctl.c
> index 4838870..8172b22 100644
> --- a/ioctl.c
> +++ b/ioctl.c
> @@ -440,13 +440,13 @@ static void cryptask_routine(struct work_struct *work)
> static int
> cryptodev_open(struct inode *inode, struct file *filp)
> {
> - struct todo_list_item *tmp;
> + struct todo_list_item *tmp, *tmp_next;
> struct crypt_priv *pcr;
> int i;
>
> pcr = kzalloc(sizeof(*pcr), GFP_KERNEL);
> if (!pcr)
> - return -ENOMEM;
> + goto open_err1;
This is unnecessary in my point of view. It adds a line of code (the
label below) and disturbs the reading flow.
> filp->private_data = pcr;
>
> mutex_init(&pcr->fcrypt.sem);
> @@ -466,7 +466,8 @@ cryptodev_open(struct inode *inode, struct file *filp)
> for (i = 0; i < DEF_COP_RINGSIZE; i++) {
> tmp = kzalloc(sizeof(struct todo_list_item), GFP_KERNEL);
> if (!tmp)
> - return -ENOMEM;
> + goto open_err2;
> +
Please use a more descriptive label, like "err_ringalloc" or so. In this
case it's not as important, but especially with many labels just
numbering them makes it hard to remember what came from where.
Do you think the added empty line helps readability?
> pcr->itemcount++;
> ddebug(2, "allocated new item at %p", tmp);
> list_add(&tmp->__hook, &pcr->free.list);
> @@ -475,6 +476,20 @@ cryptodev_open(struct inode *inode, struct file *filp)
> ddebug(2, "Cryptodev handle initialised, %d elements in queue",
> DEF_COP_RINGSIZE);
> return 0;
> +
> +/* In case of errors, free any memory allocated so far */
> +open_err2:
> + list_for_each_entry_safe(tmp, tmp_next, &pcr->free.list, __hook) {
> + list_del(&tmp->__hook);
> + kfree(tmp);
> + }
> + mutex_destroy(&pcr->done.lock);
> + mutex_destroy(&pcr->todo.lock);
> + mutex_destroy(&pcr->free.lock);
> + mutex_destroy(&pcr->fcrypt.sem);
> + kfree(pcr);
Not sure if it's a must-have, but cryptodev_release zeroes
filp->private_data in the end. Maybe duplicate it's semantics here or
abandon entirely?
> +open_err1:
> + return -ENOMEM;
> }
>
> static int
Best wishes, Phil
_______________________________________________
Cryptodev-linux-devel mailing list
Cryptodev-linux-devel@gna.org
https://mail.gna.org/listinfo/cryptodev-linux-devel
