From: Matthew Garrett <[email protected]>
The existing EFI variables code only supports variables of up to 1024
bytes. This limitation existed in version 0.99 of the EFI specification,
but was removed before any full releases. Since variables can now be
larger than a single page, sysfs isn't the best interface for this. So,
instead, let's add a filesystem. Variables can be read, written and
created, with the first 4 bytes of each variable representing its UEFI
attributes. The create() method doesn't actually commit to flash since
zero-length variables can't exist per-spec.
Updates from Jeremy Kerr <[email protected]>.
Signed-off-by: Matthew Garrett <[email protected]>
Signed-off-by: Jeremy Kerr <[email protected]>
---
drivers/firmware/efivars.c | 384 ++++++++++++++++++++++++++++++++++++-
include/linux/efi.h | 5
2 files changed, 383 insertions(+), 6 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 47408e8..4174f1b 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -80,6 +80,10 @@
#include <linux/slab.h>
#include <linux/pstore.h>
+#include <linux/fs.h>
+#include <linux/ramfs.h>
+#include <linux/pagemap.h>
+
#include <asm/uaccess.h>
#define EFIVARS_VERSION "0.08"
@@ -91,6 +95,7 @@ MODULE_LICENSE("GPL");
MODULE_VERSION(EFIVARS_VERSION);
#define DUMP_NAME_LEN 52
+#define GUID_LEN 37
/*
* The maximum size of VariableName + Data = 1024
@@ -108,7 +113,6 @@ struct efi_variable {
__u32 Attributes;
} __attribute__((packed));
-
struct efivar_entry {
struct efivars *efivars;
struct efi_variable var;
@@ -122,6 +126,9 @@ struct efivar_attribute {
ssize_t (*store)(struct efivar_entry *entry, const char *buf, size_t count);
};
+static struct efivars __efivars;
+static struct efivar_operations ops;
+
#define PSTORE_EFI_ATTRIBUTES \
(EFI_VARIABLE_NON_VOLATILE | \
EFI_VARIABLE_BOOTSERVICE_ACCESS | \
@@ -618,14 +625,380 @@ static struct kobj_type efivar_ktype = {
.default_attrs = def_attrs,
};
-static struct pstore_info efi_pstore_info;
-
static inline void
efivar_unregister(struct efivar_entry *var)
{
kobject_put(&var->kobj);
}
+static int efivarfs_file_open(struct inode *inode, struct file *file)
+{
+ file->private_data = inode->i_private;
+ return 0;
+}
+
+static ssize_t efivarfs_file_write(struct file *file,
+ const char __user *userbuf, size_t count, loff_t *ppos)
+{
+ struct efivar_entry *var = file->private_data;
+ struct efivars *efivars;
+ efi_status_t status;
+ void *data;
+ u32 attributes;
+ struct inode *inode = file->f_mapping->host;
+ int datasize = count - sizeof(attributes);
+
+ if (count < sizeof(attributes))
+ return -EINVAL;
+
+ data = kmalloc(datasize, GFP_KERNEL);
+
+ if (!data)
+ return -ENOMEM;
+
+ efivars = var->efivars;
+
+ if (copy_from_user(&attributes, userbuf, sizeof(attributes))) {
+ count = -EFAULT;
+ goto out;
+ }
+
+ if (attributes & ~(EFI_VARIABLE_MASK)) {
+ count = -EINVAL;
+ goto out;
+ }
+
+ if (copy_from_user(data, userbuf + sizeof(attributes), datasize)) {
+ count = -EFAULT;
+ goto out;
+ }
+
+ if (validate_var(&var->var, data, datasize) == false) {
+ count = -EINVAL;
+ goto out;
+ }
+
+ status = efivars->ops->set_variable(var->var.VariableName,
+ &var->var.VendorGuid,
+ attributes, datasize,
+ data);
+
+ switch (status) {
+ case EFI_SUCCESS:
+ mutex_lock(&inode->i_mutex);
+ i_size_write(inode, count);
+ mutex_unlock(&inode->i_mutex);
+ break;
+ case EFI_INVALID_PARAMETER:
+ count = -EINVAL;
+ break;
+ case EFI_OUT_OF_RESOURCES:
+ count = -ENOSPC;
+ break;
+ case EFI_DEVICE_ERROR:
+ count = -EIO;
+ break;
+ case EFI_WRITE_PROTECTED:
+ count = -EROFS;
+ break;
+ case EFI_SECURITY_VIOLATION:
+ count = -EACCES;
+ break;
+ case EFI_NOT_FOUND:
+ count = -ENOENT;
+ break;
+ default:
+ count = -EINVAL;
+ break;
+ }
+out:
+ kfree(data);
+
+ return count;
+}
+
+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)
+ return 0;
+
+ memcpy(data, &attributes, 4);
+ size = simple_read_from_buffer(userbuf, count, ppos,
+ data, datasize + 4);
+ kfree(data);
+
+ return size;
+}
+
+static void efivarfs_evict_inode(struct inode *inode)
+{
+ clear_inode(inode);
+}
+
+static const struct super_operations efivarfs_ops = {
+ .statfs = simple_statfs,
+ .drop_inode = generic_delete_inode,
+ .evict_inode = efivarfs_evict_inode,
+ .show_options = generic_show_options,
+};
+
+static struct super_block *efivarfs_sb;
+
+static const struct inode_operations efivarfs_dir_inode_operations;
+
+static const struct file_operations efivarfs_file_operations = {
+ .open = efivarfs_file_open,
+ .read = efivarfs_file_read,
+ .write = efivarfs_file_write,
+ .llseek = default_llseek,
+};
+
+static struct inode *efivarfs_get_inode(struct super_block *sb,
+ const struct inode *dir, int mode, dev_t dev)
+{
+ struct inode *inode = new_inode(sb);
+
+ if (inode) {
+ inode->i_ino = get_next_ino();
+ inode->i_uid = inode->i_gid = 0;
+ inode->i_mode = mode;
+ inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+ switch (mode & S_IFMT) {
+ case S_IFREG:
+ inode->i_fop = &efivarfs_file_operations;
+ break;
+ case S_IFDIR:
+ inode->i_op = &efivarfs_dir_inode_operations;
+ inode->i_fop = &simple_dir_operations;
+ inc_nlink(inode);
+ break;
+ }
+ }
+ return inode;
+}
+
+static void efivarfs_hex_to_guid(const char *str, efi_guid_t *guid)
+{
+ guid->b[0] = hex_to_bin(str[6]) << 4 | hex_to_bin(str[7]);
+ guid->b[1] = hex_to_bin(str[4]) << 4 | hex_to_bin(str[5]);
+ guid->b[2] = hex_to_bin(str[2]) << 4 | hex_to_bin(str[3]);
+ guid->b[3] = hex_to_bin(str[0]) << 4 | hex_to_bin(str[1]);
+ guid->b[4] = hex_to_bin(str[11]) << 4 | hex_to_bin(str[12]);
+ guid->b[5] = hex_to_bin(str[9]) << 4 | hex_to_bin(str[10]);
+ guid->b[6] = hex_to_bin(str[16]) << 4 | hex_to_bin(str[17]);
+ guid->b[7] = hex_to_bin(str[14]) << 4 | hex_to_bin(str[15]);
+ guid->b[8] = hex_to_bin(str[19]) << 4 | hex_to_bin(str[20]);
+ guid->b[9] = hex_to_bin(str[21]) << 4 | hex_to_bin(str[22]);
+ guid->b[10] = hex_to_bin(str[24]) << 4 | hex_to_bin(str[25]);
+ guid->b[11] = hex_to_bin(str[26]) << 4 | hex_to_bin(str[27]);
+ guid->b[12] = hex_to_bin(str[28]) << 4 | hex_to_bin(str[29]);
+ guid->b[13] = hex_to_bin(str[30]) << 4 | hex_to_bin(str[31]);
+ guid->b[14] = hex_to_bin(str[32]) << 4 | hex_to_bin(str[33]);
+ guid->b[15] = hex_to_bin(str[34]) << 4 | hex_to_bin(str[35]);
+}
+
+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 efivars *efivars = &__efivars;
+ struct efivar_entry *var;
+ int namelen, i = 0, err = 0;
+
+ if (dentry->d_name.len < 38)
+ return -EINVAL;
+
+ if (!inode)
+ return -ENOSPC;
+
+ var = kzalloc(sizeof(struct efivar_entry), GFP_KERNEL);
+
+ if (!var)
+ return -ENOMEM;
+
+ namelen = dentry->d_name.len - GUID_LEN;
+
+ efivarfs_hex_to_guid(dentry->d_name.name + namelen + 1,
+ &var->var.VendorGuid);
+
+ for (i = 0; i < namelen; i++)
+ var->var.VariableName[i] = dentry->d_name.name[i];
+
+ var->var.VariableName[i] = '\0';
+
+ inode->i_private = var;
+ var->efivars = efivars;
+ var->kobj.kset = efivars->kset;
+
+ err = kobject_init_and_add(&var->kobj, &efivar_ktype, NULL, "%s",
+ dentry->d_name.name);
+ if (err)
+ goto out;
+
+ kobject_uevent(&var->kobj, KOBJ_ADD);
+ spin_lock(&efivars->lock);
+ list_add(&var->list, &efivars->list);
+ spin_unlock(&efivars->lock);
+ d_instantiate(dentry, inode);
+ dget(dentry);
+out:
+ if (err)
+ kfree(var);
+ return err;
+}
+
+static int efivarfs_unlink(struct inode *dir, struct dentry *dentry)
+{
+ struct efivar_entry *var = dentry->d_inode->i_private;
+ struct efivars *efivars = var->efivars;
+ efi_status_t status;
+
+ spin_lock(&efivars->lock);
+
+ status = efivars->ops->set_variable(var->var.VariableName,
+ &var->var.VendorGuid,
+ 0, 0, NULL);
+
+ if (status == EFI_SUCCESS || status == EFI_NOT_FOUND) {
+ list_del(&var->list);
+ spin_unlock(&efivars->lock);
+ efivar_unregister(var);
+ drop_nlink(dir);
+ dput(dentry);
+ return 0;
+ }
+
+ spin_unlock(&efivars->lock);
+ return -EINVAL;
+};
+
+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);
+
+ 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);
+ dentry = d_alloc_name(root, name);
+
+ efivars->ops->get_variable(entry->var.VariableName,
+ &entry->var.VendorGuid,
+ &entry->var.Attributes,
+ &size,
+ NULL);
+
+ mutex_lock(&inode->i_mutex);
+ inode->i_private = entry;
+ i_size_write(inode, size+4);
+ mutex_unlock(&inode->i_mutex);
+ d_add(dentry, inode);
+ }
+
+ return 0;
+fail:
+ iput(inode);
+ return err;
+}
+
+static struct dentry *efivarfs_mount(struct file_system_type *fs_type,
+ int flags, const char *dev_name, void *data)
+{
+ return mount_single(fs_type, flags, data, efivarfs_fill_super);
+}
+
+static void efivarfs_kill_sb(struct super_block *sb)
+{
+ kill_litter_super(sb);
+ efivarfs_sb = NULL;
+}
+
+static struct file_system_type efivarfs_type = {
+ .name = "efivarfs",
+ .mount = efivarfs_mount,
+ .kill_sb = efivarfs_kill_sb,
+};
+
+static const struct inode_operations efivarfs_dir_inode_operations = {
+ .lookup = simple_lookup,
+ .unlink = efivarfs_unlink,
+ .create = efivarfs_create,
+};
+
+static struct pstore_info efi_pstore_info;
+
#ifdef CONFIG_PSTORE
static int efi_pstore_open(struct pstore_info *psi)
@@ -1187,6 +1560,8 @@ int register_efivars(struct efivars *efivars,
pstore_register(&efivars->efi_pstore_info);
}
+ register_filesystem(&efivarfs_type);
+
out:
kfree(variable_name);
@@ -1194,9 +1569,6 @@ out:
}
EXPORT_SYMBOL_GPL(register_efivars);
-static struct efivars __efivars;
-static struct efivar_operations ops;
-
/*
* For now we register the efi subsystem with the firmware subsystem
* and the vars subsystem with the efi subsystem. In the future, it
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ec45ccd..1829a97 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -29,7 +29,12 @@
#define EFI_UNSUPPORTED ( 3 | (1UL << (BITS_PER_LONG-1)))
#define EFI_BAD_BUFFER_SIZE ( 4 | (1UL << (BITS_PER_LONG-1)))
#define EFI_BUFFER_TOO_SMALL ( 5 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_NOT_READY ( 6 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_DEVICE_ERROR ( 7 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_WRITE_PROTECTED ( 8 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_OUT_OF_RESOURCES ( 9 | (1UL << (BITS_PER_LONG-1)))
#define EFI_NOT_FOUND (14 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_SECURITY_VIOLATION (26 | (1UL << (BITS_PER_LONG-1)))
typedef unsigned long efi_status_t;
typedef u8 efi_bool_t;
A write to an efivarfs file will not always result in a variable of
'count' size after the EFI SetVariable() call. We may have appended to
the existing data (ie, with the EFI_VARIABLE_APPEND_WRITE attribute), or
even have deleted the variable (with an authenticated variable update,
with a zero datasize).
This change re-reads the updated variable from firmware, to check for
size changes and deletions. In the latter case, we need to drop the
dentry.
Signed-off-by: Jeremy Kerr <[email protected]>
---
drivers/firmware/efivars.c | 49 +++++++++++++++++++++++++++++--------
1 file changed, 39 insertions(+), 10 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index e1253d6..a422de3 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -647,6 +647,7 @@ static ssize_t efivarfs_file_write(struct file *file,
u32 attributes;
struct inode *inode = file->f_mapping->host;
int datasize = count - sizeof(attributes);
+ unsigned long newdatasize;
if (count < sizeof(attributes))
return -EINVAL;
@@ -685,32 +686,60 @@ static ssize_t efivarfs_file_write(struct file *file,
switch (status) {
case EFI_SUCCESS:
- mutex_lock(&inode->i_mutex);
- i_size_write(inode, count);
- mutex_unlock(&inode->i_mutex);
break;
case EFI_INVALID_PARAMETER:
count = -EINVAL;
- break;
+ goto out;
case EFI_OUT_OF_RESOURCES:
count = -ENOSPC;
- break;
+ goto out;
case EFI_DEVICE_ERROR:
count = -EIO;
- break;
+ goto out;
case EFI_WRITE_PROTECTED:
count = -EROFS;
- break;
+ goto out;
case EFI_SECURITY_VIOLATION:
count = -EACCES;
- break;
+ goto out;
case EFI_NOT_FOUND:
count = -ENOENT;
- break;
+ goto out;
default:
count = -EINVAL;
- break;
+ goto out;
}
+
+ /*
+ * Writing to the variable may have caused a change in size (which
+ * could either be an append or an overwrite), or the variable to be
+ * deleted. Perform a GetVariable() so we can tell what actually
+ * happened.
+ */
+ newdatasize = 0;
+ status = efivars->ops->get_variable(var->var.VariableName,
+ &var->var.VendorGuid,
+ NULL, &newdatasize,
+ NULL);
+
+ if (status == EFI_BUFFER_TOO_SMALL) {
+ mutex_lock(&inode->i_mutex);
+ i_size_write(inode, newdatasize + sizeof(attributes));
+ mutex_unlock(&inode->i_mutex);
+
+ } else if (status == EFI_NOT_FOUND) {
+ spin_lock(&efivars->lock);
+ list_del(&var->list);
+ spin_unlock(&efivars->lock);
+ efivar_unregister(var);
+ drop_nlink(inode);
+ dput(file->f_dentry);
+
+ } else {
+ pr_warn("efivarfs: inconsistent EFI variable implementation? "
+ "status = %lx\n", status);
+ }
+
out:
kfree(data);
From: Lee, Chun-Yi <[email protected]>
UEFI variable filesystem need a new mount point, so this patch add
efivars kobject to efi_kobj for create a /sys/firmware/efi/efivars
folder.
Cc: Matt Fleming <[email protected]>
Cc: Jeremy Kerr <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Signed-off-by: Lee, Chun-Yi <[email protected]>
Signed-off-by: Jeremy Kerr <[email protected]>
---
drivers/firmware/efivars.c | 11 +++++++++++
include/linux/efi.h | 1 +
2 files changed, 12 insertions(+)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 4174f1b..e1253d6 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1487,6 +1487,7 @@ void unregister_efivars(struct efivars *efivars)
sysfs_remove_bin_file(&efivars->kset->kobj, efivars->del_var);
kfree(efivars->new_var);
kfree(efivars->del_var);
+ kobject_put(efivars->kobject);
kset_unregister(efivars->kset);
}
EXPORT_SYMBOL_GPL(unregister_efivars);
@@ -1518,6 +1519,13 @@ int register_efivars(struct efivars *efivars,
goto out;
}
+ efivars->kobject = kobject_create_and_add("efivars", parent_kobj);
+ if (!efivars->kobject) {
+ pr_err("efivars: Subsystem registration failed.\n");
+ error = -ENOMEM;
+ goto err_unreg_vars;
+ }
+
/*
* Per EFI spec, the maximum storage allocated for both
* the variable name and variable data is 1024 bytes.
@@ -1562,6 +1570,9 @@ int register_efivars(struct efivars *efivars,
register_filesystem(&efivarfs_type);
+err_unreg_vars:
+ kset_unregister(efivars->kset);
+
out:
kfree(variable_name);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 1829a97..c993f54 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -654,6 +654,7 @@ struct efivars {
spinlock_t lock;
struct list_head list;
struct kset *kset;
+ struct kobject *kobject;
struct bin_attribute *new_var, *del_var;
const struct efivar_operations *ops;
struct efivar_entry *walk_entry;
Hi Jeremy,
於 五，2012-10-05 於 13:54 +0800，Jeremy Kerr 提到：
>
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 4174f1b..e1253d6 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -1487,6 +1487,7 @@ void unregister_efivars(struct efivars *efivars)
> sysfs_remove_bin_file(&efivars->kset->kobj,
> efivars->del_var);
> kfree(efivars->new_var);
> kfree(efivars->del_var);
> + kobject_put(efivars->kobject);
> kset_unregister(efivars->kset);
> }
> EXPORT_SYMBOL_GPL(unregister_efivars);
> @@ -1518,6 +1519,13 @@ int register_efivars(struct efivars *efivars,
Since more people prefer to use "efivarfs", so, I modified the patch for
replace "efivars" to "efivarfs" like following:
Thanks
Joey Lee
>From 5e64382a9a4ad538dd5ca94072d19c7a70e4c650 Mon Sep 17 00:00:00 2001
From: Lee, Chun-Yi <[email protected]>
Date: Sat, 15 Sep 2012 10:33:46 +0800
Subject: [PATCH] efi: add efivarfs kobject to efi sysfs folder
UEFI variable filesystem need a new mount point, so this patch add
efivarfs kobject to efi_kobj for create a /sys/firmware/efi/efivarfs
folder.
Cc: Matt Fleming <[email protected]>
Cc: Jeremy Kerr <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Signed-off-by: Lee, Chun-Yi <[email protected]>
---
drivers/firmware/efivars.c | 11 +++++++++++
include/linux/efi.h | 1 +
2 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 1e1aad0..7c1234e 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1487,6 +1487,7 @@ void unregister_efivars(struct efivars *efivars)
sysfs_remove_bin_file(&efivars->kset->kobj, efivars->del_var);
kfree(efivars->new_var);
kfree(efivars->del_var);
+ kobject_put(efivars->kobject);
kset_unregister(efivars->kset);
}
EXPORT_SYMBOL_GPL(unregister_efivars);
@@ -1518,6 +1519,13 @@ int register_efivars(struct efivars *efivars,
goto out;
}
+ efivars->kobject = kobject_create_and_add("efivarfs", parent_kobj);
+ if (!efivars->kobject) {
+ pr_err("efivars: Subsystem registration failed.\n");
+ error = -ENOMEM;
+ goto err_unreg_vars;
+ }
+
/*
* Per EFI spec, the maximum storage allocated for both
* the variable name and variable data is 1024 bytes.
@@ -1562,6 +1570,9 @@ int register_efivars(struct efivars *efivars,
register_filesystem(&efivars_fs_type);
+err_unreg_vars:
+ kset_unregister(efivars->kset);
+
out:
kfree(variable_name);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 1829a97..c993f54 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -654,6 +654,7 @@ struct efivars {
spinlock_t lock;
struct list_head list;
struct kset *kset;
+ struct kobject *kobject;
struct bin_attribute *new_var, *del_var;
const struct efivar_operations *ops;
struct efivar_entry *walk_entry;
--
1.7.7
Hi Joey,
> Since more people prefer to use "efivarfs", so, I modified the patch for
> replace "efivars" to "efivarfs" like following:
I'd intentionally left this patch as-is. I think that the filesystem
should be called "efivarfs", while the directory should be called
"efivars". Same style as sysfs -> /sys.
Cheers,
Jeremy
On Fri, 2012-10-05 at 13:54 +0800, Jeremy Kerr wrote:
> From: Matthew Garrett <[email protected]>
>
> The existing EFI variables code only supports variables of up to 1024
> bytes. This limitation existed in version 0.99 of the EFI specification,
> but was removed before any full releases. Since variables can now be
> larger than a single page, sysfs isn't the best interface for this. So,
> instead, let's add a filesystem. Variables can be read, written and
> created, with the first 4 bytes of each variable representing its UEFI
> attributes. The create() method doesn't actually commit to flash since
> zero-length variables can't exist per-spec.
>
> Updates from Jeremy Kerr <[email protected]>.
>
> Signed-off-by: Matthew Garrett <[email protected]>
> Signed-off-by: Jeremy Kerr <[email protected]>
Thanks, I've applied this series.
Signed-off-by: Andy Whitcroft <[email protected]>
---
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;
+ }
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;
}
--
1.7.9.5
Signed-off-by: Andy Whitcroft <[email protected]>
---
drivers/firmware/efivars.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 4b12a8fd..ae50d2f 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -766,7 +766,7 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
unsigned long datasize = 0;
u32 attributes;
void *data;
- ssize_t size;
+ ssize_t size = 0;
status = efivars->ops->get_variable(var->var.VariableName,
&var->var.VendorGuid,
@@ -784,13 +784,13 @@ static ssize_t efivarfs_file_read(struct file *file, char __user *userbuf,
&var->var.VendorGuid,
&attributes, &datasize,
(data + 4));
-
if (status != EFI_SUCCESS)
- return 0;
+ goto out_free;
memcpy(data, &attributes, 4);
size = simple_read_from_buffer(userbuf, count, ppos,
data, datasize + 4);
+out_free:
kfree(data);
return size;
--
1.7.9.5
We have recently been looking to backport the efivarfs support as posted,
to 3.5.x. Inspired by some searching questions from Tetsuo Handa I have
been reviewing this code. The following a first pass at fixing up some
of the error handling. As they represent error paths they are hard to
truly verify so deserve review.
On top of 7b218e8e5d433fc8b531ce911926e06de3e6f1f6 in Matt Flemmings efi.git
repo.
-apw
Andy Whitcroft (5):
efivarfs: efivarfs_file_read ensure we free data in error paths
efivarfs: efivarfs_create() ensure we drop our reference on inode on error
efivarfs: efivarfs_fill_super() fix inode reference counts
efivarfs: efivarfs_fill_super() ensure we free our temporary name
efivarfs: efivarfs_fill_super() ensure we clean up correctly on error
drivers/firmware/efivars.c | 56 +++++++++++++++++++++++++++-----------------
1 file changed, 35 insertions(+), 21 deletions(-)
--
1.7.9.5
Ensure we free both the name and inode on error when building the
individual variables.
Signed-off-by: Andy Whitcroft <[email protected]>
---
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
When d_make_root() fails it will automatically drop the reference
on the root inode. We should not be doing so as well.
Signed-off-by: Andy Whitcroft <[email protected]>
---
drivers/firmware/efivars.c | 16 ++++------------
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 0bbf742..79f3e4e 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -948,7 +948,6 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
struct dentry *root;
struct efivar_entry *entry, *n;
struct efivars *efivars = &__efivars;
- int err;
efivarfs_sb = sb;
@@ -960,18 +959,14 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
sb->s_time_gran = 1;
inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0);
- if (!inode) {
- err = -ENOMEM;
- goto fail;
- }
+ if (!inode)
+ return -ENOMEM;
inode->i_op = &efivarfs_dir_inode_operations;
root = d_make_root(inode);
sb->s_root = root;
- if (!root) {
- err = -ENOMEM;
- goto fail;
- }
+ if (!root)
+ return -ENOMEM;
list_for_each_entry_safe(entry, n, &efivars->list, list) {
struct inode *inode;
@@ -1012,9 +1007,6 @@ int efivarfs_fill_super(struct super_block *sb, void *data, int silent)
}
return 0;
-fail:
- iput(inode);
- return err;
}
static struct dentry *efivarfs_mount(struct file_system_type *fs_type,
--
1.7.9.5
d_alloc_name() copies the passed name to new storage, once complete we
no longer need our name.
Signed-off-by: Andy Whitcroft <[email protected]>
---
drivers/firmware/efivars.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 79f3e4e..dcb34ae 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -992,6 +992,8 @@ 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);
dentry = d_alloc_name(root, name);
+ /* copied by the above to local storage in the dentry. */
+ kfree(name);
efivars->ops->get_variable(entry->var.VariableName,
&entry->var.VendorGuid,
--
1.7.9.5
For the set:
Acked-by: Matthew Garrett <[email protected]>
--
Matthew Garrett | [email protected]
On Thu, 2012-10-11 at 11:32 +0100, Andy Whitcroft wrote:
> We have recently been looking to backport the efivarfs support as posted,
> to 3.5.x. Inspired by some searching questions from Tetsuo Handa I have
> been reviewing this code. The following a first pass at fixing up some
> of the error handling. As they represent error paths they are hard to
> truly verify so deserve review.
>
> On top of 7b218e8e5d433fc8b531ce911926e06de3e6f1f6 in Matt Flemmings efi.git
> repo.
>
> -apw
>
> Andy Whitcroft (5):
> efivarfs: efivarfs_file_read ensure we free data in error paths
> efivarfs: efivarfs_create() ensure we drop our reference on inode on error
> efivarfs: efivarfs_fill_super() fix inode reference counts
> efivarfs: efivarfs_fill_super() ensure we free our temporary name
> efivarfs: efivarfs_fill_super() ensure we clean up correctly on error
>
> drivers/firmware/efivars.c | 56 +++++++++++++++++++++++++++-----------------
> 1 file changed, 35 insertions(+), 21 deletions(-)
Thanks Andy, I've added Matthew's Acked-by and applied this series to my
'next' branch.
Hi Andy,
> Signed-off-by: Andy Whitcroft <[email protected]>
> ---
> drivers/firmware/efivars.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
Looks good to me.
Acked-by: Jeremy Kerr <[email protected]>
Cheers,
Jeremy
Hi Andy,
> d_alloc_name() copies the passed name to new storage, once complete we
> no longer need our name.
Acked-by: Jeremy Kerr <[email protected]>
Cheers,
Jeremy
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
Hi Andy,
> When d_make_root() fails it will automatically drop the reference
> on the root inode. We should not be doing so as well.
Looks good:
Acked-by: Jeremy Kerr <[email protected]>
Cheers,
Jeremy
Hi Andy,
Looks good. Thanks for taking the time to review the efivarfs changes.
Acked-by: Jeremy Kerr <[email protected]>
Cheers,
Jeremy
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
On Thu, 2012-10-11 at 11:32 +0100, Andy Whitcroft wrote:
> Signed-off-by: Andy Whitcroft <[email protected]>
> ---
> 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
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.
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
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 <[email protected]>
Cheers,
Jeremy Kerr