lkml.org 
[lkml]   [2017]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v4 13/14] platform/x86: dell-smbios-wmi: introduce userspace interface
    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

    \
     
     \ /
      Last update: 2017-10-05 09:24    [W:4.133 / U:0.552 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site