| Message ID | 1552997064-432700-5-git-send-email-dragan.cvetic@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | misc: xilinx sd-fec driver | expand |
On Tue, Mar 19, 2019 at 1:05 PM Dragan Cvetic <dragan.cvetic@xilinx.com> wrote: > > Add char device interface per DT node present and support > file operations: > - open(), which keeps only one open per device at a time, > - close(), which release the open for this device, > - ioctl(), which provides infrastructure for a specific driver > control. > drivers/misc/xilinx_sdfec.c | 79 ++++++++++++++++++++++++++++++++++++++++ > include/uapi/misc/xilinx_sdfec.h | 4 ++ > 2 files changed, 83 insertions(+) > > diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c > index a52a5c6..3407de4 100644 > --- a/drivers/misc/xilinx_sdfec.c > +++ b/drivers/misc/xilinx_sdfec.c > @@ -81,8 +81,87 @@ 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 (!xsdfec) > + return -EAGAIN; The result of container_of() will not be NULL here. Did you mean to check i_cdev? That probably also won't be NULL, but that check would be more reasonable. > + /* Only one open per device at a time */ > + if (!atomic_dec_and_test(&xsdfec->open_count)) { > + atomic_inc(&xsdfec->open_count); > + return -EBUSY; > + } What is that limitation for? Is it worse to open it twice than to dup() or fork()? Note that the test is not really atomic either: if three processes try to open the file at the same time, it gets decremented from 1 to -2, so only the second one sees 0 and increments it back to -1 afterwards... > +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) { > + dev_err(xsdfec->dev, "Not a xilinx sdfec ioctl"); > + return -ENOTTY; > + } remove the error messages here as well. > + /* Access check of the argument if present */ > + if (_IOC_DIR(cmd) & _IOC_READ) > + err = !access_ok((void *)arg, _IOC_SIZE(cmd)); > + else if (_IOC_DIR(cmd) & _IOC_WRITE) > + err = !access_ok((void *)arg, _IOC_SIZE(cmd)); This seems odd. Why two separate checks, and why the access_ok() check when you do a copy_from_user() that contains the same check later? If you want to get fancy here, you could just copy the data in the main ioctl handler based on _IOC_SIZE, and pass around normal kernel pointers from there. > static const struct file_operations xsdfec_fops = { > .owner = THIS_MODULE, > + .open = xsdfec_dev_open, > + .release = xsdfec_dev_release, > + .unlocked_ioctl = xsdfec_dev_ioctl, > }; This lacks a .compat_ioctl pointer. Arnd
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Tuesday 19 March 2019 13:18 > To: Dragan Cvetic <draganc@xilinx.com> > Cc: gregkh <gregkh@linuxfoundation.org>; Michal Simek <michals@xilinx.com>; Linux ARM <linux-arm-kernel@lists.infradead.org>; > Derek Kiernan <dkiernan@xilinx.com>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH 04/12] misc: xilinx_sdfec: Add open, close and ioctl > > On Tue, Mar 19, 2019 at 1:05 PM Dragan Cvetic <dragan.cvetic@xilinx.com> wrote: > > > > Add char device interface per DT node present and support > > file operations: > > - open(), which keeps only one open per device at a time, > > - close(), which release the open for this device, > > - ioctl(), which provides infrastructure for a specific driver > > control. > > > drivers/misc/xilinx_sdfec.c | 79 ++++++++++++++++++++++++++++++++++++++++ > > include/uapi/misc/xilinx_sdfec.h | 4 ++ > > 2 files changed, 83 insertions(+) > > > > diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c > > index a52a5c6..3407de4 100644 > > --- a/drivers/misc/xilinx_sdfec.c > > +++ b/drivers/misc/xilinx_sdfec.c > > @@ -81,8 +81,87 @@ 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 (!xsdfec) > > + return -EAGAIN; > > The result of container_of() will not be NULL here. > Did you mean to check i_cdev? That probably also won't > be NULL, but that check would be more reasonable. Will be either removed fully or changed with i_cdev check > > > + /* Only one open per device at a time */ > > + if (!atomic_dec_and_test(&xsdfec->open_count)) { > > + atomic_inc(&xsdfec->open_count); > > + return -EBUSY; > > + } > > What is that limitation for? Is it worse to open it twice than > to dup() or fork()? > The device can be opened only once. > Note that the test is not really atomic either: if three processes > try to open the file at the same time, it gets decremented from > 1 to -2, so only the second one sees 0 and increments it back > to -1 afterwards... It looks you are right. Will fix this. Thank you for the catch. > > > +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) { > > + dev_err(xsdfec->dev, "Not a xilinx sdfec ioctl"); > > + return -ENOTTY; > > + } > > remove the error messages here as well. > > > + /* Access check of the argument if present */ > > + if (_IOC_DIR(cmd) & _IOC_READ) > > + err = !access_ok((void *)arg, _IOC_SIZE(cmd)); > > + else if (_IOC_DIR(cmd) & _IOC_WRITE) > > + err = !access_ok((void *)arg, _IOC_SIZE(cmd)); > > This seems odd. Why two separate checks, and why the access_ok() > check when you do a copy_from_user() that contains the same check > later? Accepted, will remove it. > > If you want to get fancy here, you could just copy the data in the main > ioctl handler based on _IOC_SIZE, and pass around normal kernel > pointers from there. Will not be fancy. Thank you for the advice. > > > static const struct file_operations xsdfec_fops = { > > .owner = THIS_MODULE, > > + .open = xsdfec_dev_open, > > + .release = xsdfec_dev_release, > > + .unlocked_ioctl = xsdfec_dev_ioctl, > > }; > > This lacks a .compat_ioctl pointer. This is new for me, I have to investigate more and propose a solution. Thank you for suggestion. > > Arnd
On Tue, Mar 19, 2019 at 3:59 PM Dragan Cvetic <draganc@xilinx.com> wrote: > > > > > + /* Only one open per device at a time */ > > > + if (!atomic_dec_and_test(&xsdfec->open_count)) { > > > + atomic_inc(&xsdfec->open_count); > > > + return -EBUSY; > > > + } > > > > What is that limitation for? Is it worse to open it twice than > > to dup() or fork()? > > > The device can be opened only once. What I mean here is that preventing the double open() is a fairly weak protection: it means you cannot have multiple 'struct file' pointers attached to the same inode, but you can still have the same 'struct file' being available to multiple processes. Arnd
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Tuesday 19 March 2019 15:36 > To: Dragan Cvetic <draganc@xilinx.com> > Cc: gregkh <gregkh@linuxfoundation.org>; Michal Simek <michals@xilinx.com>; Linux ARM <linux-arm-kernel@lists.infradead.org>; > Derek Kiernan <dkiernan@xilinx.com>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH 04/12] misc: xilinx_sdfec: Add open, close and ioctl > > On Tue, Mar 19, 2019 at 3:59 PM Dragan Cvetic <draganc@xilinx.com> wrote: > > > > > > > + /* Only one open per device at a time */ > > > > + if (!atomic_dec_and_test(&xsdfec->open_count)) { > > > > + atomic_inc(&xsdfec->open_count); > > > > + return -EBUSY; > > > > + } > > > > > > What is that limitation for? Is it worse to open it twice than > > > to dup() or fork()? > > > > > The device can be opened only once. > > What I mean here is that preventing the double open() is > a fairly weak protection: it means you cannot have multiple > 'struct file' pointers attached to the same inode, but you > can still have the same 'struct file' being available to > multiple processes. > Could you please suggest the solution? My intention was to prevent more than one process access the same device. > Arnd Thanks Dragan
On Tue, Mar 19, 2019 at 7:10 PM Dragan Cvetic <draganc@xilinx.com> wrote: > > -----Original Message----- > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > Sent: Tuesday 19 March 2019 15:36 > > To: Dragan Cvetic <draganc@xilinx.com> > > Cc: gregkh <gregkh@linuxfoundation.org>; Michal Simek <michals@xilinx.com>; Linux ARM <linux-arm-kernel@lists.infradead.org>; > > Derek Kiernan <dkiernan@xilinx.com>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > > Subject: Re: [PATCH 04/12] misc: xilinx_sdfec: Add open, close and ioctl > > > > On Tue, Mar 19, 2019 at 3:59 PM Dragan Cvetic <draganc@xilinx.com> wrote: > > > > > > > > > + /* Only one open per device at a time */ > > > > > + if (!atomic_dec_and_test(&xsdfec->open_count)) { > > > > > + atomic_inc(&xsdfec->open_count); > > > > > + return -EBUSY; > > > > > + } > > > > > > > > What is that limitation for? Is it worse to open it twice than > > > > to dup() or fork()? > > > > > > > The device can be opened only once. > > > > What I mean here is that preventing the double open() is > > a fairly weak protection: it means you cannot have multiple > > 'struct file' pointers attached to the same inode, but you > > can still have the same 'struct file' being available to > > multiple processes. > > > Could you please suggest the solution? > My intention was to prevent more than one process access the same device. Generally speaking, you can't prevent it, but you should make sure that if two processes attempt to use the same device, nothing bad happens. Usually it's enough to have appropriate locking. Arnd
> -----Original Message----- > From: Arnd Bergmann [mailto:arnd@arndb.de] > Sent: Tuesday 19 March 2019 19:46 > To: Dragan Cvetic <draganc@xilinx.com> > Cc: gregkh <gregkh@linuxfoundation.org>; Michal Simek <michals@xilinx.com>; Derek Kiernan <dkiernan@xilinx.com>; Linux ARM > <linux-arm-kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH 04/12] misc: xilinx_sdfec: Add open, close and ioctl > > On Tue, Mar 19, 2019 at 7:10 PM Dragan Cvetic <draganc@xilinx.com> wrote: > > > -----Original Message----- > > > From: Arnd Bergmann [mailto:arnd@arndb.de] > > > Sent: Tuesday 19 March 2019 15:36 > > > To: Dragan Cvetic <draganc@xilinx.com> > > > Cc: gregkh <gregkh@linuxfoundation.org>; Michal Simek <michals@xilinx.com>; Linux ARM <linux-arm- > kernel@lists.infradead.org>; > > > Derek Kiernan <dkiernan@xilinx.com>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > > > Subject: Re: [PATCH 04/12] misc: xilinx_sdfec: Add open, close and ioctl > > > > > > On Tue, Mar 19, 2019 at 3:59 PM Dragan Cvetic <draganc@xilinx.com> wrote: > > > > > > > > > > > + /* Only one open per device at a time */ > > > > > > + if (!atomic_dec_and_test(&xsdfec->open_count)) { > > > > > > + atomic_inc(&xsdfec->open_count); > > > > > > + return -EBUSY; > > > > > > + } > > > > > > > > > > What is that limitation for? Is it worse to open it twice than > > > > > to dup() or fork()? > > > > > > > > > The device can be opened only once. > > > > > > What I mean here is that preventing the double open() is > > > a fairly weak protection: it means you cannot have multiple > > > 'struct file' pointers attached to the same inode, but you > > > can still have the same 'struct file' being available to > > > multiple processes. > > > > > Could you please suggest the solution? > > My intention was to prevent more than one process access the same device. > > Generally speaking, you can't prevent it, but you should make sure that > if two processes attempt to use the same device, nothing bad happens. > Usually it's enough to have appropriate locking. > There is a need to increase the driver security, even the proposed is not perfect, it is acceptable for us for now. > Arnd
diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c index a52a5c6..3407de4 100644 --- a/drivers/misc/xilinx_sdfec.c +++ b/drivers/misc/xilinx_sdfec.c @@ -81,8 +81,87 @@ 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 (!xsdfec) + return -EAGAIN; + + /* Only one open per device at a time */ + 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); + if (!xsdfec) + return -EAGAIN; + + 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) { + dev_err(xsdfec->dev, "Not a xilinx sdfec ioctl"); + 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; + } + } + + /* Access check of the argument if present */ + if (_IOC_DIR(cmd) & _IOC_READ) + err = !access_ok((void *)arg, _IOC_SIZE(cmd)); + else if (_IOC_DIR(cmd) & _IOC_WRITE) + err = !access_ok((void *)arg, _IOC_SIZE(cmd)); + + 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; +} + static const struct file_operations xsdfec_fops = { .owner = THIS_MODULE, + .open = xsdfec_dev_open, + .release = xsdfec_dev_release, + .unlocked_ioctl = xsdfec_dev_ioctl, }; 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 5543163..5de0add 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__ */