Re: [patch 9/9 v3] x86: export x86 boot_params to sysfs

[Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>]

On Thu, Nov 21, 2013 at 02:17:13PM +0800, dyoung@redhat.com wrote:
> kexec-tools use boot_params for getting the 1st kernel hardware_subarch,
> the kexec kernel efi runtime support also need read the old efi_info from
> boot_params. Currently it exists in debugfs which is not a good place for
> such infomation. Per HPA, we should avoid of "sploit debugfs".
> 
> In this patch /sys/kernel/boot_params are exported, also the setup_data
> is exported as a subdirectory. For original debugfs since it's already
> there for long time and kexec-tools is using it for hardware_subarch so
> let's do not remove them for now.
> 
> Structure are like below:
> 
> /sys/kernel/boot_params
> ├── data		/* binary data for boot_params */
> ├── setup_data  	/* subdirectory for setup_data if there's any */
> │   ├── 0		/* the first setup_data node */
> │   │   ├── data	/* binary data for setup_data node 0 */
> │   │   └── type	/* setup_data type of setup_data node 0, hex string */
> |   [snip]		/* other setup_data nodes ... */
> └── version		/* hex string for boot protocal version */
> 
> Signed-off-by: Dave Young <dyoung@redhat.com>
> ---
>  Documentation/ABI/testing/sysfs-kernel-boot_params |   40 ++
>  arch/x86/kernel/Makefile                           |    2 
>  arch/x86/kernel/ksysfs.c                           |  326 +++++++++++++++++++++
>  3 files changed, 367 insertions(+), 1 deletion(-)
> 
> --- /dev/null
> +++ efi/Documentation/ABI/testing/sysfs-kernel-boot_params
> @@ -0,0 +1,40 @@
> +What:		/sys/kernel/boot_params
> +Date:		November 2013
> +Contact:	Dave Young <dyoung@redhat.com>
> +Description:
> +		The /sys/kernel/boot_params directory contains two
> +		files: "data" and "version" and one subdirectory "setup_data".
> +		It is used to export the kernel boot parameters of x86
> +		platform to user space for kexec and debugging purpose.
> +
> +		If there's no setup_data in boot_params the subdirectory will
> +		not be created.
> +
> +		"data" file is the binary representation of struct boot_params.
> +
> +		"version" file is the string representation of boot
> +		protocol version.
> +
> +		"setup_data" subdirectory contains the setup_data data
> +		structure in boot_params. setup_data is maintained in kernel
> +		as a link list. In "setup_data" subdirectory there's one
> +		subdirectory for each link list node named with the number
> +		of the list nodes. The list node subdirectory contains two
> +		files "type" and "data". "type" file is the string
> +		representation of setup_data type. "data" file is the binary
> +		representation of setup_data payload.
> +
> +		The whole boot_params directory structure is like below:
> +		/sys/kernel/boot_params
> +		├── data
> +		├── setup_data
> +		│   ├── 0
> +		│   │   ├── data
> +		│   │   └── type
> +		│   └── 1
> +		│       ├── data
> +		│       └── type
> +		└── version
> +
> +Users:
> +		Kexec Mailing List <kexec@lists.infradead.org>
> --- efi.orig/arch/x86/kernel/Makefile
> +++ efi/arch/x86/kernel/Makefile
> @@ -35,7 +35,7 @@ obj-y			+= alternative.o i8253.o pci-nom
>  obj-y			+= tsc.o io_delay.o rtc.o
>  obj-y			+= pci-iommu_table.o
>  obj-y			+= resource.o
> -
> +obj-$(CONFIG_SYSFS)	+= ksysfs.o
>  obj-y				+= process.o
>  obj-y				+= i387.o xsave.o
>  obj-y				+= ptrace.o
> --- /dev/null
> +++ efi/arch/x86/kernel/ksysfs.c
> @@ -0,0 +1,326 @@
> +/*
> + * Architecture specific sysfs attributes in /sys/kernel
> + *
> + * Copyright (C) 2007, Intel Corp.
> + *      Huang Ying <ying.huang@intel.com>
> + * Copyright (C) 2013, 2013 Red Hat, Inc.
> + *      Dave Young <dyoung@redhat.com>
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#include <linux/kobject.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/init.h>
> +#include <linux/stat.h>
> +#include <linux/slab.h>
> +#include <linux/mm.h>
> +
> +#include <asm/setup.h>
> +
> +static ssize_t boot_params_version_show(struct kobject *kobj,
> +					struct kobj_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "0x%04x\n", boot_params.hdr.version);
> +}
> +
> +static struct kobj_attribute boot_params_version_attr =
> +	__ATTR(version, S_IRUGO, boot_params_version_show, NULL);

__ATTR_RO() please.

> +static struct kobj_attribute type_attr =
> +		__ATTR(type, S_IRUGO, setup_data_type_show, NULL);

__ATTR_RO() here too.


> +static struct bin_attribute data_attr = {
> +	.attr = {
> +		.name = "data",
> +		.mode = S_IRUGO,
> +	},
> +	.read = setup_data_data_read,
> +};
> +
> +static int __init create_setup_data_node(struct kobject *parent,
> +					struct kobject **kobjp, int nr)
> +{
> +	int ret = 0;
> +	size_t size;
> +	struct kobject *kobj;
> +	char name[16]; /* should be enough for setup_data nodes numbers */
> +	snprintf(name, 16, "%d", nr);
> +
> +	kobj = kobject_create_and_add(name, parent);
> +	if (!kobj)
> +		return -ENOMEM;
> +
> +	ret = sysfs_create_file(kobj, &type_attr.attr);
> +	if (ret)
> +		goto out_kobj;
> +
> +	ret = get_setup_data_size(nr, &size);
> +	if (ret)
> +		goto out_kobj;
> +
> +	data_attr.size = size;
> +	ret = sysfs_create_bin_file(kobj, &data_attr);
> +	if (ret)
> +		goto out_file;
> +	*kobjp = kobj;

You just raced with userspace (creating and announcing the kobject and
then, afterward, at some later point in time, created the sysfs files.
Please use the groups option to create the files properly before
announcing the kobject to userspace.

> +static int __init boot_params_ksysfs_init(void)
> +{
> +	int ret;
> +	struct kobject *boot_params_kobj;
> +
> +	boot_params_kobj = kobject_create_and_add("boot_params",
> +						       kernel_kobj);
> +	if (!boot_params_kobj) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	ret = sysfs_create_file(boot_params_kobj,
> +				&boot_params_version_attr.attr);
> +	if (ret)
> +		goto out_boot_params_kobj;
> +	ret = sysfs_create_bin_file(boot_params_kobj,
> +				      &boot_params_data_attr);
> +	if (ret)
> +		goto out_create_file;
> +
> +	ret = create_setup_data_nodes(boot_params_kobj);
> +	if (ret)
> +		goto out_create_bin;
> +
> +	return 0;

Same race condition here as well.  Use a groups structure please.

thanks,

greg k-h

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec