Message ID | 1556402706-176271-5-git-send-email-dragan.cvetic@xilinx.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | misc: xilinx sd-fec drive | expand |
On Sat, Apr 27, 2019 at 11:04:58PM +0100, Dragan Cvetic wrote: > +static int xsdfec_dev_open(struct inode *iptr, struct file *fptr) > +{ > + struct xsdfec_dev *xsdfec; > + > + xsdfec = container_of(iptr->i_cdev, struct xsdfec_dev, xsdfec_cdev); > + > + if (!atomic_dec_and_test(&xsdfec->open_count)) { Why do you care about this? And do you really think it matters? What are you trying to protect from here? > + atomic_inc(&xsdfec->open_count); > + return -EBUSY; > + } > + > + fptr->private_data = xsdfec; > + return 0; > +} > + > +static int xsdfec_dev_release(struct inode *iptr, struct file *fptr) > +{ > + struct xsdfec_dev *xsdfec; > + > + xsdfec = container_of(iptr->i_cdev, struct xsdfec_dev, xsdfec_cdev); > + > + atomic_inc(&xsdfec->open_count); You increment a number when the device is closed? You are trying to make it hard to maintain this code over time :) > + return 0; > +} > + > +static long xsdfec_dev_ioctl(struct file *fptr, unsigned int cmd, > + unsigned long data) > +{ > + struct xsdfec_dev *xsdfec = fptr->private_data; > + void __user *arg = NULL; > + int rval = -EINVAL; > + int err = 0; > + > + if (!xsdfec) > + return rval; > + > + if (_IOC_TYPE(cmd) != XSDFEC_MAGIC) > + return -ENOTTY; > + > + /* check if ioctl argument is present and valid */ > + if (_IOC_DIR(cmd) != _IOC_NONE) { > + arg = (void __user *)data; > + if (!arg) { > + dev_err(xsdfec->dev, > + "xilinx sdfec ioctl argument is NULL Pointer"); You just created a way for userspace to spam the kernel log, please do not do that :( > + return rval; > + } > + } > + > + if (err) { > + dev_err(xsdfec->dev, "Invalid xilinx sdfec ioctl argument"); > + return -EFAULT; Wrong error, you did not have a memory fault. Again, you just created a way to spam the kernel log by a user :( thanks, greg k-h
On Sat, Apr 27, 2019 at 11:04:58PM +0100, Dragan Cvetic wrote: > Add char device interface per DT node present and support > file operations: > - open(), > - close(), > - unlocked_ioctl(), > - compat_ioctl(). Why do you need compat_ioctl() at all? Any "new" driver should never need it. Just create your structures properly. thanks, greg k-h
Hi Greg, Please find inline comments below. Regards Dragan > -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Thursday 2 May 2019 18:23 > To: Dragan Cvetic <draganc@xilinx.com> > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > Subject: Re: [PATCH V3 04/12] misc: xilinx_sdfec: Add open, close and ioctl > > On Sat, Apr 27, 2019 at 11:04:58PM +0100, Dragan Cvetic wrote: > > +static int xsdfec_dev_open(struct inode *iptr, struct file *fptr) > > +{ > > + struct xsdfec_dev *xsdfec; > > + > > + xsdfec = container_of(iptr->i_cdev, struct xsdfec_dev, xsdfec_cdev); > > + > > + if (!atomic_dec_and_test(&xsdfec->open_count)) { > > Why do you care about this? > > And do you really think it matters? What are you trying to protect from > here? There is a request to increase the driver security. It is acceptable for us for now, even with non-perfections (will not be protected if opened twice with dup() or fork()). This is covered in the documentation. > > > + atomic_inc(&xsdfec->open_count); > > + return -EBUSY; > > + } > > + > > + fptr->private_data = xsdfec; > > + return 0; > > +} > > + > > +static int xsdfec_dev_release(struct inode *iptr, struct file *fptr) > > +{ > > + struct xsdfec_dev *xsdfec; > > + > > + xsdfec = container_of(iptr->i_cdev, struct xsdfec_dev, xsdfec_cdev); > > + > > + atomic_inc(&xsdfec->open_count); > > You increment a number when the device is closed? > > You are trying to make it hard to maintain this code over time :) > > > > + return 0; > > +} > > + > > +static long xsdfec_dev_ioctl(struct file *fptr, unsigned int cmd, > > + unsigned long data) > > +{ > > + struct xsdfec_dev *xsdfec = fptr->private_data; > > + void __user *arg = NULL; > > + int rval = -EINVAL; > > + int err = 0; > > + > > + if (!xsdfec) > > + return rval; > > + > > + if (_IOC_TYPE(cmd) != XSDFEC_MAGIC) > > + return -ENOTTY; > > + > > + /* check if ioctl argument is present and valid */ > > + if (_IOC_DIR(cmd) != _IOC_NONE) { > > + arg = (void __user *)data; > > + if (!arg) { > > + dev_err(xsdfec->dev, > > + "xilinx sdfec ioctl argument is NULL Pointer"); > > You just created a way for userspace to spam the kernel log, please do > not do that :( Will be removed. > > > > > + return rval; > > + } > > + } > > + > > + if (err) { > > + dev_err(xsdfec->dev, "Invalid xilinx sdfec ioctl argument"); > > + return -EFAULT; > > Wrong error, you did not have a memory fault. Absolutely useless code. Will be removed. Thanks. > > Again, you just created a way to spam the kernel log by a user :( > > thanks, > > greg k-h
> -----Original Message----- > From: Greg KH [mailto:gregkh@linuxfoundation.org] > Sent: Thursday 2 May 2019 18:24 > To: Dragan Cvetic <draganc@xilinx.com> > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > Subject: Re: [PATCH V3 04/12] misc: xilinx_sdfec: Add open, close and ioctl > > On Sat, Apr 27, 2019 at 11:04:58PM +0100, Dragan Cvetic wrote: > > Add char device interface per DT node present and support > > file operations: > > - open(), > > - close(), > > - unlocked_ioctl(), > > - compat_ioctl(). > > Why do you need compat_ioctl() at all? Any "new" driver should never > need it. Just create your structures properly. This was a comment from Arnd, see https://lkml.org/lkml/2019/3/19/377. Please advise. > > thanks, > > greg k-h
On Fri, May 03, 2019 at 04:46:12PM +0000, Dragan Cvetic wrote: > > > > -----Original Message----- > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > Sent: Thursday 2 May 2019 18:24 > > To: Dragan Cvetic <draganc@xilinx.com> > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > Subject: Re: [PATCH V3 04/12] misc: xilinx_sdfec: Add open, close and ioctl > > > > On Sat, Apr 27, 2019 at 11:04:58PM +0100, Dragan Cvetic wrote: > > > Add char device interface per DT node present and support > > > file operations: > > > - open(), > > > - close(), > > > - unlocked_ioctl(), > > > - compat_ioctl(). > > > > Why do you need compat_ioctl() at all? Any "new" driver should never > > need it. Just create your structures properly. > > This was a comment from Arnd, see https://lkml.org/lkml/2019/3/19/377. > Please advise. Why do you need a compat_ioctl callback when there is nothing to fix up? thanks, greg k-h
On Fri, May 03, 2019 at 04:44:57PM +0000, Dragan Cvetic wrote: > > -----Original Message----- > > From: Greg KH [mailto:gregkh@linuxfoundation.org] > > Sent: Thursday 2 May 2019 18:23 > > To: Dragan Cvetic <draganc@xilinx.com> > > Cc: arnd@arndb.de; Michal Simek <michals@xilinx.com>; linux-arm-kernel@lists.infradead.org; robh+dt@kernel.org; > > mark.rutland@arm.com; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Derek Kiernan <dkiernan@xilinx.com> > > Subject: Re: [PATCH V3 04/12] misc: xilinx_sdfec: Add open, close and ioctl > > > > On Sat, Apr 27, 2019 at 11:04:58PM +0100, Dragan Cvetic wrote: > > > +static int xsdfec_dev_open(struct inode *iptr, struct file *fptr) > > > +{ > > > + struct xsdfec_dev *xsdfec; > > > + > > > + xsdfec = container_of(iptr->i_cdev, struct xsdfec_dev, xsdfec_cdev); > > > + > > > + if (!atomic_dec_and_test(&xsdfec->open_count)) { > > > > Why do you care about this? > > > > And do you really think it matters? What are you trying to protect from > > here? > > There is a request to increase the driver security. How does this affect "security" in any way? > It is acceptable for us for now, even with non-perfections (will not > be protected if opened twice with dup() or fork()). This is covered > in the documentation. As this really "does nothing", no need to bother the kernel with trying to keep this logic working properly. So please just drop it. thanks, greg k-h
On Thu, May 2, 2019 at 1:23 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Sat, Apr 27, 2019 at 11:04:58PM +0100, Dragan Cvetic wrote: > > Add char device interface per DT node present and support > > file operations: > > - open(), > > - close(), > > - unlocked_ioctl(), > > - compat_ioctl(). > > Why do you need compat_ioctl() at all? Any "new" driver should never > need it. Just create your structures properly. The function he added was the version that is needed when the structures are compatible. I submitted a series to add a generic 'compat_ptr_ioctl' implementation that would save a few lines here doing the same thing, but it's not merged yet. Generally speaking, every driver that has a .ioctl() function should also have a .compat_ioctl(), and ideally it should be exactly this trivial version. Arnd
On Sat, May 04, 2019 at 10:35:02AM -0400, Arnd Bergmann wrote: > On Thu, May 2, 2019 at 1:23 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Sat, Apr 27, 2019 at 11:04:58PM +0100, Dragan Cvetic wrote: > > > Add char device interface per DT node present and support > > > file operations: > > > - open(), > > > - close(), > > > - unlocked_ioctl(), > > > - compat_ioctl(). > > > > Why do you need compat_ioctl() at all? Any "new" driver should never > > need it. Just create your structures properly. > > The function he added was the version that is needed when the structures > are compatible. I submitted a series to add a generic 'compat_ptr_ioctl' > implementation that would save a few lines here doing the same thing, > but it's not merged yet. > > Generally speaking, every driver that has a .ioctl() function should also > have a .compat_ioctl(), and ideally it should be exactly this trivial > version. Ok, for some reason I thought if there was no need for a compat ioctl (i.e. no pointer mess), then no need for a callback at all. thanks, greg k-h
diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c index a52a5c6..30879ae 100644 --- a/drivers/misc/xilinx_sdfec.c +++ b/drivers/misc/xilinx_sdfec.c @@ -25,6 +25,7 @@ #include <linux/uaccess.h> #include <linux/spinlock.h> #include <linux/clk.h> +#include <linux/compat.h> #include <uapi/misc/xilinx_sdfec.h> @@ -81,8 +82,85 @@ struct xsdfec_dev { struct xsdfec_clks clks; }; +static int xsdfec_dev_open(struct inode *iptr, struct file *fptr) +{ + struct xsdfec_dev *xsdfec; + + xsdfec = container_of(iptr->i_cdev, struct xsdfec_dev, xsdfec_cdev); + + if (!atomic_dec_and_test(&xsdfec->open_count)) { + atomic_inc(&xsdfec->open_count); + return -EBUSY; + } + + fptr->private_data = xsdfec; + return 0; +} + +static int xsdfec_dev_release(struct inode *iptr, struct file *fptr) +{ + struct xsdfec_dev *xsdfec; + + xsdfec = container_of(iptr->i_cdev, struct xsdfec_dev, xsdfec_cdev); + + atomic_inc(&xsdfec->open_count); + return 0; +} + +static long xsdfec_dev_ioctl(struct file *fptr, unsigned int cmd, + unsigned long data) +{ + struct xsdfec_dev *xsdfec = fptr->private_data; + void __user *arg = NULL; + int rval = -EINVAL; + int err = 0; + + if (!xsdfec) + return rval; + + if (_IOC_TYPE(cmd) != XSDFEC_MAGIC) + return -ENOTTY; + + /* check if ioctl argument is present and valid */ + if (_IOC_DIR(cmd) != _IOC_NONE) { + arg = (void __user *)data; + if (!arg) { + dev_err(xsdfec->dev, + "xilinx sdfec ioctl argument is NULL Pointer"); + return rval; + } + } + + if (err) { + dev_err(xsdfec->dev, "Invalid xilinx sdfec ioctl argument"); + return -EFAULT; + } + + switch (cmd) { + default: + /* Should not get here */ + dev_err(xsdfec->dev, "Undefined SDFEC IOCTL"); + break; + } + return rval; +} + +#ifdef CONFIG_COMPAT +static long xsdfec_dev_compat_ioctl(struct file *file, unsigned int cmd, + unsigned long data) +{ + return xsdfec_dev_ioctl(file, cmd, (unsigned long)compat_ptr(data)); +} +#endif + static const struct file_operations xsdfec_fops = { .owner = THIS_MODULE, + .open = xsdfec_dev_open, + .release = xsdfec_dev_release, + .unlocked_ioctl = xsdfec_dev_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = xsdfec_dev_compat_ioctl, +#endif }; static int xsdfec_clk_init(struct platform_device *pdev, diff --git a/include/uapi/misc/xilinx_sdfec.h b/include/uapi/misc/xilinx_sdfec.h index ba577fa..9709759 100644 --- a/include/uapi/misc/xilinx_sdfec.h +++ b/include/uapi/misc/xilinx_sdfec.h @@ -39,4 +39,8 @@ struct xsdfec_config { __s32 fec_id; }; +/* + * XSDFEC IOCTL List + */ +#define XSDFEC_MAGIC 'f' #endif /* __XILINX_SDFEC_H__ */