On Fri, Jul 17, 2020 at 08:05:08PM +0000, Eads, Gage wrote:
>
>
> > -----Original Message-----
> > From: Arnd Bergmann <a...@arndb.de>
> > Sent: Friday, July 17, 2020 1:57 PM
> > To: Eads, Gage <gage.e...@intel.com>
> > Cc: linux-kernel@vger.kernel.org; gregkh <gre...@linuxfoundation.org>;
> > Karlsson, Magnus <magnus.karls...@intel.com>; Topel, Bjorn
> > <bjorn.to...@intel.com>
> > Subject: Re: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls
> >
> > On Fri, Jul 17, 2020 at 8:19 PM Eads, Gage <gage.e...@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.
> >
>
> Ah, understood. Will fix in v2.
>
> > > >
> > > > > +/* [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.
> >
>
> "Driver interface version" is the better term for what I'm trying to
> accomplish here. Any backports would have to be done in such a way that the
> interface version is honored, but if that can't be reasonably expected...then
> I agree, versioning is unworkable.
There is no such thing as a "driver interface version", sorry, that is
not going to be workable at all.
thanks,
greg k-h
