Re: [PATCH 2/5] efivarfs: efivarfs_create() ensure we drop our reference on inode on error

Re: [PATCH 2/5] efivarfs: efivarfs_create() ensure we drop our reference on inode on error

[Khalid Aziz]

On Thu, 2012-10-11 at 11:32 +0100, Andy Whitcroft wrote:
> Signed-off-by: Andy Whitcroft <apw-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> ---
>  drivers/firmware/efivars.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index ae50d2f..0bbf742 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -866,7 +866,7 @@ static void efivarfs_hex_to_guid(const char *str, efi_guid_t *guid)
>  static int efivarfs_create(struct inode *dir, struct dentry *dentry,
>  			  umode_t mode, bool excl)
>  {
> -	struct inode *inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
> +	struct inode *inode;
>  	struct efivars *efivars = &__efivars;
>  	struct efivar_entry *var;
>  	int namelen, i = 0, err = 0;
> @@ -874,13 +874,15 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
>  	if (dentry->d_name.len < 38)
>  		return -EINVAL;
>  
> +	inode = efivarfs_get_inode(dir->i_sb, dir, mode, 0);
>  	if (!inode)
>  		return -ENOSPC;
>  
>  	var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
> -
> -	if (!var)
> -		return -ENOMEM;
> +	if (!var) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
>  

This does not read right. If kzalloc() fails, var will be a NULL
pointer. This code will set err to -ENOMEM and jump to out: where since
err is non-zero, this code will call kfree(Var) but var is a NULL
pointer at this point. Now kfree() does check for NULL pointer and this
will not cause any serious problems but why call kfree for a NULL
pointer?


>  	namelen = dentry->d_name.len - GUID_LEN;
>  
> @@ -908,8 +910,10 @@ static int efivarfs_create(struct inode *dir, struct dentry *dentry,
>  	d_instantiate(dentry, inode);
>  	dget(dentry);
>  out:
> -	if (err)
> +	if (err) {
>  		kfree(var);
> +		iput(inode);
> +	}
>  	return err;
>  }
>  

--
Khalid


--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] efivarfs: efivarfs_create() ensure we drop our reference on inode on error

[Matt Fleming]

On Fri, 2012-10-12 at 13:03 -0600, Khalid Aziz wrote:
> This does not read right. If kzalloc() fails, var will be a NULL
> pointer. This code will set err to -ENOMEM and jump to out: where since
> err is non-zero, this code will call kfree(Var) but var is a NULL
> pointer at this point. Now kfree() does check for NULL pointer and this
> will not cause any serious problems but why call kfree for a NULL
> pointer?

This is a common idiom used throughout the kernel to simply error paths.
As you noted, calling kfree(NULL) is harmless and there's certainly no
need to worry about the overhead of calling kfree() without doing any
freeing since the error path is also the slow path.

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] efivarfs: efivarfs_create() ensure we drop our reference on inode on error

[Khalid Aziz]

On Fri, 2012-10-12 at 20:21 +0100, Matt Fleming wrote:
> This is a common idiom used throughout the kernel to simply error paths.
> As you noted, calling kfree(NULL) is harmless and there's certainly no
> need to worry about the overhead of calling kfree() without doing any
> freeing since the error path is also the slow path.

A "return -ENOMEM" looks simpler and easier to read to me, but that is a
subjective opinion :)

--
Khalid

Re: [PATCH 5/5] efivarfs: efivarfs_fill_super() ensure we clean up correctly on error

[Jeremy Kerr]

Hi Andy,

>> If we break out of the loop on the second (and onwards) iteration,
>> won't we still have the other inodes and dentries remaining
>> allocated?
>
> As we calling this from the mount_single() wrapper:
>
>          return mount_single(fs_type, flags, data, efivarfs_fill_super);
>
>
> which does this:
>
>      struct dentry *mount_single(struct file_system_type *fs_type,
> 	    int flags, void *data,
> 	    int (*fill_super)(struct super_block *, void *, int))
>      {
>      [...]
> 		    error = fill_super(s, data, flags & MS_SILENT ? 1 : 0);
> 		    if (error) {
> 			    deactivate_locked_super(s);
> 			    return ERR_PTR(error);
> 		    }
>      [...]
>
> I am expecting us to get called back via deactivate_locked_super(),
> which calls sb->kill_sb() which is:
>
>      static void efivarfs_kill_sb(struct super_block *sb)
>      {
> 	    kill_litter_super(sb);
> 	    efivarfs_sb = NULL;
>      }
>
> Which I believe will clean them up.

Awesome, thanks for that. Looks good to me.

Acked-by: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

Cheers,


Jeremy Kerr

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html