[PATCH 1/4] efi: Add support for a UEFI variable filesystem

[Andy Whitcroft]

On Tue, Oct 09, 2012 at 08:41:42PM +0900, Tetsuo Handa wrote:
> Andy Whitcroft wrote:
> > +static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
> > +		size_t count, loff_t *ppos)
> > +{
> > +	struct efivar_entry *var = file->private_data;
> > +	struct efivars *efivars = var->efivars;
> > +	efi_status_t status;
> > +	unsigned long datasize = 0;
> > +	u32 attributes;
> > +	void *data;
> > +	ssize_t size;
> > +
> > +	status = efivars->ops->get_variable(var->var.VariableName,
> > +					    &var->var.VendorGuid,
> > +					    &attributes, &datasize, NULL);
> > +
> > +	if (status != EFI_BUFFER_TOO_SMALL)
> > +		return 0;
> > +
> > +	data = kmalloc(datasize + 4, GFP_KERNEL);
> > +
> > +	if (!data)
> > +		return 0;
> > +
> > +	status = efivars->ops->get_variable(var->var.VariableName,
> > +					    &var->var.VendorGuid,
> > +					    &attributes, &datasize,
> > +					    (data + 4));
> > +
> > +	if (status != EFI_SUCCESS)
> 
> Why no kfree(data) before return?
> 
> > +		return 0;
> > +
> > +	memcpy(data, &attributes, 4);
> > +	size = simple_read_from_buffer(userbuf, count, ppos,
> > +					data, datasize + 4);
> > +	kfree(data);
> > +
> > +	return size;
> > +}
> 
> > +int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
> > +{
> > +	struct inode *inode = NULL;
> > +	struct dentry *root;
> > +	struct efivar_entry *entry, *n;
> > +	struct efivars *efivars = &__efivars;
> > +	int err;
> > +
> > +	efivarfs_sb = sb;
> > +
> > +	sb->s_maxbytes          = MAX_LFS_FILESIZE;
> > +	sb->s_blocksize         = PAGE_CACHE_SIZE;
> > +	sb->s_blocksize_bits    = PAGE_CACHE_SHIFT;
> > +	sb->s_magic             = PSTOREFS_MAGIC;
> > +	sb->s_op                = &efivarfs_ops;
> > +	sb->s_time_gran         = 1;
> > +
> > +	inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0);
> > +	if (!inode) {
> > +		err = -ENOMEM;
> > +		goto fail;
> > +	}
> > +	inode->i_op = &efivarfs_dir_inode_operations;
> > +
> > +	root = d_make_root(inode);
> > +	sb->s_root = root;
> > +	if (!root) {
> > +		err = -ENOMEM;
> > +		goto fail;
> > +	}
> > +
> > +	list_for_each_entry_safe(entry, n, &efivars->list, list) {
> > +		struct inode *inode;
> > +		struct dentry *dentry, *root = efivarfs_sb->s_root;
> > +		char *name;
> > +		unsigned long size = 0;
> > +		int len, i;
> > +
> > +		len = utf16_strlen(entry->var.VariableName);
> > +
> > +		/* GUID plus trailing NULL */
> > +		name = kmalloc(len + 38, GFP_ATOMIC);
> 
> Why no name != NULL check?
> 
> > +
> > +		for (i = 0; i < len; i++)
> > +			name[i] = entry->var.VariableName[i] & 0xFF;
> > +
> > +		name[len] = '-';
> > +
> > +		efi_guid_unparse(&entry->var.VendorGuid, name + len + 1);
> > +
> > +		name[len+GUID_LEN] = '\0';
> > +
> > +		inode = efivarfs_get_inode(efivarfs_sb, root->d_inode,
> > +					  S_IFREG | 0644, 0);
> 
> Why no inode != NULL check?
> 
> > +		dentry = d_alloc_name(root, name);
> 
> Why no dentry != NULL check?
> 

And actually why no "free(name);"

Sigh ... 

Ignore this heap for now.

-apw