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>
 <CAK8P3a2OmSPGNghM+Y9ThH7hgKJKVSKRPaSPN17gUVRUh_o3bQ@mail.gmail.com> <SN6PR11MB2574F33CF9C422517B3C13CFF67C0@SN6PR11MB2574.namprd11.prod.outlook.com>
In-Reply-To: <SN6PR11MB2574F33CF9C422517B3C13CFF67C0@SN6PR11MB2574.namprd11.prod.outlook.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 17 Jul 2020 20:56:41 +0200
Message-ID: <CAK8P3a09C6-auonW9jw30z_CijLgT+LkWfYowYoVdrwSzNGWKg@mail.gmail.com>
Subject: Re: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls
To:     "Eads, Gage" <gage.eads@intel.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        gregkh <gregkh@linuxfoundation.org>,
        "Karlsson, Magnus" <magnus.karlsson@intel.com>,
        "Topel, Bjorn" <bjorn.topel@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Jul 17, 2020 at 8:19 PM Eads, Gage <gage.eads@intel.com> wrote:

> > A plain copy_from_user() in place of this function should be fine.
>
> This function also validates the user size arg to prevent buffer overflow; centralizing it here avoids the case where a programmer accidentally forgets the check in an ioctl handler (and reduces code duplication). If it's alright with you, I'll keep the function but drop the dev_err() prints.

Once you use a 'switch(cmd)' statement in the top ioctl handler, the
data structure size will be fixed, so there is no way the argument
size can go wrong.

> >
> > > +/* [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.
>
> Ok, I'll merge the response structure into the ioctl structure (here and elsewhere).
>
> Say I add this command later: without driver versioning, how would
> user-space know in advance whether the command is supported?
> It could attempt the command and interpret -ENOTTY as "unsupported",
> but that strikes me as an inelegant way to reverse-engineer the version.

There is not really a driver "version" once the driver is upstream, the concept
doesn't really make sense here when arbitrary patches can get backported
from the latest kernel into whatever the user is running.

The ENOTTY check is indeed the normal way that user space deals
with interfaces that may have been added later. What you normally want
is to keep using the original interfaces anyway, unless you absolutely
need a later revision for a certain feature, and in that case the user space
program will fail no matter what.

> > 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.
>
> Can you elaborate? Printing the driver name/copyright/etc. seems to be a common pattern in upstream drivers.

Most drivers don't do it, and it's generally not recommended. You can
print a message when something goes wrong, but most users don't
care about that stuff and it clutters up the kernel log if each driver
prints a line or two.

     Arnd