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
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.k...@canonical.com 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, >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, >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/
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/
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/