Messages in this thread | | | From | "Eads, Gage" <> | Subject | RE: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls | Date | Fri, 17 Jul 2020 20:05:08 +0000 |
| |
> -----Original Message----- > From: Arnd Bergmann <arnd@arndb.de> > Sent: Friday, July 17, 2020 1:57 PM > To: Eads, Gage <gage.eads@intel.com> > Cc: linux-kernel@vger.kernel.org; gregkh <gregkh@linuxfoundation.org>; > Karlsson, Magnus <magnus.karlsson@intel.com>; Topel, Bjorn > <bjorn.topel@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.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. >
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.
> 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. >
Fair enough. I'll remove it.
Thanks, Gage
> Arnd
| |