Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200712134331.8169-1-gage.eads@intel.com> <20200712134331.8169-5-gage.eads@intel.com>
In-Reply-To: <20200712134331.8169-5-gage.eads@intel.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Sun, 12 Jul 2020 17:28:18 +0200
Message-ID: <CAK8P3a2OmSPGNghM+Y9ThH7hgKJKVSKRPaSPN17gUVRUh_o3bQ@mail.gmail.com>
Subject: Re: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls
To:     Gage Eads <gage.eads@intel.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        gregkh <gregkh@linuxfoundation.org>, magnus.karlsson@intel.com,
        =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn.topel@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Jul 12, 2020 at 3:46 PM Gage Eads <gage.eads@intel.com> wrote:
>
> This commit introduces the dlb2 device ioctl layer, and the first four
> ioctls: query device version, driver version, and available resources; and
> create a scheduling domain. This commit also introduces the user-space
> interface file dlb2_user.h.
>
> The PF hardware operation for scheduling domain creation will be added in a
> subsequent commit.
>
> Signed-off-by: Gage Eads <gage.eads@intel.com>
> Reviewed-by: Magnus Karlsson <magnus.karlsson@intel.com>

I'm looking only at the ioctl interface here, as that is usually the
hardest part to get right, and you can't easily change it after it gets
merged.

The definitions all look portable and will work fine in compat mode
(which is often a problem), but you have added a bit of complexity
compared to how it is commonly done, so it's better to simplify it
and write it more like other drivers do. More on that below.

> +/* Verify the ioctl argument size and copy the argument into kernel memory */
> +static int dlb2_copy_from_user(struct dlb2_dev *dev,
> +                              unsigned long user_arg,
> +                              u16 user_size,
> +                              void *arg,
> +                              size_t size)
> +{
> +       if (user_size != size) {
> +               dev_err(dev->dlb2_device,
> +                       "[%s()] Invalid ioctl size\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       if (copy_from_user(arg, (void __user *)user_arg, size)) {
> +               dev_err(dev->dlb2_device,
> +                       "[%s()] Invalid ioctl argument pointer\n", __func__);
> +               return -EFAULT;
> +       }
> +
> +       return 0;
> +}

You should avoid error messages that are triggered based on user input.
and can cause a denial-of-service when the console is spammed that
way.

A plain copy_from_user() in place of this function should be fine.

> +/* [7:0]: device revision, [15:8]: device version */
> +#define DLB2_SET_DEVICE_VERSION(ver, rev) (((ver) << 8) | (rev))
> +
> +static int dlb2_ioctl_get_device_version(struct dlb2_dev *dev,
> +                                        unsigned long user_arg,
> +                                        u16 size)
> +{
> +       struct dlb2_get_device_version_args arg;
> +       struct dlb2_cmd_response response;
> +       int ret;
> +
> +       dev_dbg(dev->dlb2_device, "Entering %s()\n", __func__);
> +
> +       response.status = 0;
> +       response.id = DLB2_SET_DEVICE_VERSION(2, DLB2_REV_A0);
> +
> +       ret = dlb2_copy_from_user(dev, user_arg, size, &arg, sizeof(arg));
> +       if (ret)
> +               return ret;
> +
> +       ret = dlb2_copy_resp_to_user(dev, arg.response, &response);

Better avoid any indirect pointers. As you always return a constant
here, I think the entire ioctl command can be removed until you
actually need it. If you have an ioctl command that needs both
input and output, use _IOWR() to define it and put all arguments
into the same structure.

> +static int dlb2_ioctl_create_sched_domain(struct dlb2_dev *dev,
> +                                         unsigned long user_arg,
> +                                         u16 size)
> +{
> +       struct dlb2_create_sched_domain_args arg;
> +       struct dlb2_cmd_response response = {0};
> +       int ret;
> +
> +       dev_dbg(dev->dlb2_device, "Entering %s()\n", __func__);

I assume you have finished debugging this version and can remove
these debug comments again. If you find new bugs, you can add them
temporarily, but nothing is gained by these here. You can use ftrace
to see when functions are called.

> +
> +       ret = dlb2_copy_from_user(dev, user_arg, size, &arg, sizeof(arg));
> +       if (ret)
> +               return ret;
> +
> +       /* Copy zeroes to verify the user-provided response pointer */
> +       ret = dlb2_copy_resp_to_user(dev, arg.response, &response);
> +       if (ret)
> +               return ret;

There is no need to verify the response pointer. If it fails later,
it's the user's fault, and they get to deal with it.

> +static int dlb2_ioctl_get_driver_version(struct dlb2_dev *dev,
> +                                        unsigned long user_arg,
> +                                        u16 size)
> +{
> +       struct dlb2_get_driver_version_args arg;
> +       struct dlb2_cmd_response response;
> +       int ret;
> +
> +       dev_dbg(dev->dlb2_device, "Entering %s()\n", __func__);
> +
> +       response.status = 0;
> +       response.id = DLB2_VERSION;

Just remove the driver version command, trying to have explicit
interface versions creates more problems than it solves.

> +int dlb2_ioctl_dispatcher(struct dlb2_dev *dev,
> +                         unsigned int cmd,
> +                         unsigned long arg)
> +{
> +       u16 sz = _IOC_SIZE(cmd);
> +
> +       if (_IOC_NR(cmd) >= NUM_DLB2_CMD) {
> +               dev_err(dev->dlb2_device,
> +                       "[%s()] Unexpected DLB command %d\n",
> +                       __func__, _IOC_NR(cmd));
> +               return -1;
> +       }
> +
> +       return dlb2_ioctl_callback_fns[_IOC_NR(cmd)](dev, arg, sz);
> +}

This is usually written with a switch/case statement, so doing the same here
tends to make it easier to understand.

> +static long
> +dlb2_ioctl(struct file *f, unsigned int cmd, unsigned long arg)
> +{
> +       struct dlb2_dev *dev;
> +
> +       dev = container_of(f->f_inode->i_cdev, struct dlb2_dev, cdev);
> +
> +       if (_IOC_TYPE(cmd) != DLB2_IOC_MAGIC) {
> +               dev_err(dev->dlb2_device,
> +                       "[%s()] Bad magic number!\n", __func__);
> +               return -EINVAL;
> +       }
> +
> +       return dlb2_ioctl_dispatcher(dev, cmd, arg);
> +}

This function can also be removed then, just call the dispatcher
directly.
>         int err;
>
> -       pr_info("%s\n", dlb2_driver_name);
> +       pr_info("%s - version %d.%d.%d\n", dlb2_driver_name,
> +               DLB2_VERSION_MAJOR_NUMBER,
> +               DLB2_VERSION_MINOR_NUMBER,
> +               DLB2_VERSION_REVISION_NUMBER);
>         pr_info("%s\n", dlb2_driver_copyright);

Just remove the pr_info completely.

      Arnd