Re: [PATCH 5/5] efivarfs: efivarfs_fill_super() ensure we clean up correctly on error
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
Cheers,
Jeremy Kerr
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] efivarfs: efivarfs_fill_super() ensure we clean up correctly on error
On Thu, Oct 11, 2012 at 10:04:28PM +0800, Jeremy Kerr wrote:
> Hi Andy,
>
> >@@ -969,16 +970,18 @@
> > return -ENOMEM;
> >
> > 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;
> >
> >+inode = NULL;
> >+
> > len = utf16_strlen(entry->var.VariableName);
> >
> > /* GUID plus trailing NULL */
> > name = kmalloc(len + 38, GFP_ATOMIC);
> >+if (!name)
> >+goto fail;
> >
> > for (i = 0; i < len; i++)
> > name[i] = entry->var.VariableName[i] & 0xFF;
> >@@ -991,7 +994,13 @@
> >
> > inode = efivarfs_get_inode(efivarfs_sb, root->d_inode,
> > S_IFREG | 0644, 0);
> >+if (!inode)
> >+goto fail_name;
> >+
> > dentry = d_alloc_name(root, name);
> >+if (!dentry)
> >+goto fail_inode;
> >+
> > /* copied by the above to local storage in the dentry. */
> > kfree(name);
>
> 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.
-apw
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 5/5] efivarfs: efivarfs_fill_super() ensure we clean up correctly on error
Hi Andy,
@@ -969,16 +970,18 @@
return -ENOMEM;
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;
+ inode = NULL;
+
len = utf16_strlen(entry->var.VariableName);
/* GUID plus trailing NULL */
name = kmalloc(len + 38, GFP_ATOMIC);
+ if (!name)
+ goto fail;
for (i = 0; i < len; i++)
name[i] = entry->var.VariableName[i] & 0xFF;
@@ -991,7 +994,13 @@
inode = efivarfs_get_inode(efivarfs_sb, root->d_inode,
S_IFREG | 0644, 0);
+ if (!inode)
+ goto fail_name;
+
dentry = d_alloc_name(root, name);
+ if (!dentry)
+ goto fail_inode;
+
/* copied by the above to local storage in the dentry. */
kfree(name);
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?
Cheers,
Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 5/5] efivarfs: efivarfs_fill_super() ensure we clean up correctly on error
Ensure we free both the name and inode on error when building the
individual variables.
Signed-off-by: Andy Whitcroft
---
drivers/firmware/efivars.c | 20 ++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index dcb34ae..0cad5d9 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -948,6 +948,7 @@ int efivarfs_fill_super(struct super_block *sb, void *data,
int silent)
struct dentry *root;
struct efivar_entry *entry, *n;
struct efivars *efivars = &__efivars;
+ char *name;
efivarfs_sb = sb;
@@ -969,16 +970,18 @@ int efivarfs_fill_super(struct super_block *sb, void
*data, int silent)
return -ENOMEM;
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;
+ inode = NULL;
+
len = utf16_strlen(entry->var.VariableName);
/* GUID plus trailing NULL */
name = kmalloc(len + 38, GFP_ATOMIC);
+ if (!name)
+ goto fail;
for (i = 0; i < len; i++)
name[i] = entry->var.VariableName[i] & 0xFF;
@@ -991,7 +994,13 @@ int efivarfs_fill_super(struct super_block *sb, void
*data, int silent)
inode = efivarfs_get_inode(efivarfs_sb, root->d_inode,
S_IFREG | 0644, 0);
+ if (!inode)
+ goto fail_name;
+
dentry = d_alloc_name(root, name);
+ if (!dentry)
+ goto fail_inode;
+
/* copied by the above to local storage in the dentry. */
kfree(name);
@@ -1009,6 +1018,13 @@ int efivarfs_fill_super(struct super_block *sb, void
*data, int silent)
}
return 0;
+
+fail_inode:
+ iput(inode);
+fail_name:
+ kfree(name);
+fail:
+ return -ENOMEM;
}
static struct dentry *efivarfs_mount(struct file_system_type *fs_type,
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
