On Wed, Oct 04, 2017 at 05:48:39PM -0500, Mario Limonciello wrote:
> This userspace character device will be used to perform SMBIOS calls
> from any applications.
>
> It provides an ioctl that will allow passing the 32k WMI calling
> interface buffer between userspace and kernel space.
{sigh} Did you really test this? It feels like it wasn't, due to the
api you are using here. Did you run it with a 32bit userspace and 64bit
kernel? 32bit kernel/userspace? How well did your userspace developer
fall down crying when you tried that? :)
> This character device is intended to deprecate the dcdbas kernel module
> and the interface that it provides to userspace.
At least that driver has a well-documented api to userspace, you are
throwing all of that away here, are you _sure_ you want to do that?
Seems like you just made things much harder.
> It's important for the driver to provide a R/W ioctl to ensure that
> two competing userspace processes don't race to provide or read each
> others data.
The whole goal of this patch is to provide that ioctl, right? So of
course it is "important" :)
> The API for interacting with this interface is defined in documentation
> as well as a uapi header provides the format of the structures.
Ok, let's _just_ review that api please:
> diff --git a/include/uapi/linux/dell-smbios-wmi.h b/include/uapi/linux/dell-smbios-wmi.h
> new file mode 100644
> index 000000000000..0d0d09b04021
> --- /dev/null
> +++ b/include/uapi/linux/dell-smbios-wmi.h
> @@ -0,0 +1,25 @@
> +#ifndef _UAPI_DELL_SMBIOS_WMI_H_
> +#define _UAPI_DELL_SMBIOS_WMI_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/wmi.h>
> +
> +struct wmi_calling_interface_buffer {
> + u16 class;
> + u16 select;
> + u32 input[4];
> + u32 output[4];
> + u32 argattrib;
> + u32 blength;
> + u8 *data;
> +} __packed;
{sigh}
For structures that cross the user/kernel boundry, you _HAVE_ to use the
correct types. For some things, that is easy, u16 needs to be __u16,
u32 needs to be __u32, but u8*? Hah, good luck! Remember what I
mentioned above about 32/64 bit issues?
Why do you need a pointer here at all? You are providing a huge chunk
of memory to the ioctl, what's the use of a pointer? How are you
dereferenceing this pointer (remember, it's a userspace pointer, which
you are not saying here, so odds are the kernel code is wrong...)
> +struct wmi_smbios_ioctl {
> + u32 length;
__u32.
And why not __u64? Is 32 bits always going to be ok?
> + struct wmi_calling_interface_buffer *buf;
Another pointer? 2 pointer dereferences in the same ioctl structure?
Crazy, you are wanting to make your life harder than it has to be...
> +};
> +
> +/* only offers on the single instance */
> +#define DELL_WMI_SMBIOS_CMD WMI_IOWR(0)
I don't understand the comment, please explain it better.
Please sit down and work out your api here, I don't think you have
thought it through properly, given the number of pointers alone.
thanks,
greg k-h
