[V3,04/12] misc: xilinx_sdfec: Add open, close and ioctl

Commit Message

Dragan Cvetic April 27, 2019, 10:04 p.m. UTC

Add char device interface per DT node present and support
file operations:
- open(),
- close(),
- unlocked_ioctl(),
- compat_ioctl().

Tested-by: Dragan Cvetic <[e-mail]>
Signed-off-by: Derek Kiernan <[e-mail]>
Signed-off-by: Dragan Cvetic <[e-mail]>
---
 drivers/misc/xilinx_sdfec.c      | 78 ++++++++++++++++++++++++++++++++++++++++
 include/uapi/misc/xilinx_sdfec.h |  4 +++
 2 files changed, 82 insertions(+)

Comments

Greg Kroah-Hartman May 2, 2019, 5:23 p.m. UTC | #1

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

Greg Kroah-Hartman May 2, 2019, 5:23 p.m. UTC | #2

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

Dragan Cvetic May 3, 2019, 4:44 p.m. UTC | #3

Hi Greg,

Please find inline comments below.

Regards
Dragan

> -----Original Message-----
> From: Greg KH [mailto:[e-mail]]
> Sent: Thursday 2 May 2019 18:23
> To: Dragan Cvetic <[e-mail]>
> Cc: [e-mail]; Michal Simek <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; Derek Kiernan <[e-mail]>
> 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

Dragan Cvetic May 3, 2019, 4:46 p.m. UTC | #4

> -----Original Message-----
> From: Greg KH [mailto:[e-mail]]
> Sent: Thursday 2 May 2019 18:24
> To: Dragan Cvetic <[e-mail]>
> Cc: [e-mail]; Michal Simek <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; Derek Kiernan <[e-mail]>
> 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

Greg Kroah-Hartman May 4, 2019, 9:01 a.m. UTC | #5

On Fri, May 03, 2019 at 04:46:12PM +0000, Dragan Cvetic wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:[e-mail]]
> > Sent: Thursday 2 May 2019 18:24
> > To: Dragan Cvetic <[e-mail]>
> > Cc: [e-mail]; Michal Simek <[e-mail]>; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; Derek Kiernan <[e-mail]>
> > 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

Greg Kroah-Hartman May 4, 2019, 9:02 a.m. UTC | #6

On Fri, May 03, 2019 at 04:44:57PM +0000, Dragan Cvetic wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:[e-mail]]
> > Sent: Thursday 2 May 2019 18:23
> > To: Dragan Cvetic <[e-mail]>
> > Cc: [e-mail]; Michal Simek <[e-mail]>; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; Derek Kiernan <[e-mail]>
> > 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

Arnd Bergmann May 4, 2019, 2:35 p.m. UTC | #7

On Thu, May 2, 2019 at 1:23 PM Greg KH <[e-mail]> 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

Greg Kroah-Hartman May 4, 2019, 2:41 p.m. UTC | #8

On Sat, May 04, 2019 at 10:35:02AM -0400, Arnd Bergmann wrote:
> On Thu, May 2, 2019 at 1:23 PM Greg KH <[e-mail]> 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

Patch

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__ */