[PATCH v2 0/1] misc: hpilo: switch .{read,write} ops to .{read,write}_iter

[PATCH v2 0/1] misc: hpilo: switch .{read,write} ops to .{read,write}_iter

[matt.hsiao]

From: Matt Hsiao <matt.hsiao@hpe.com>

This patch changes .{read,write} ops to their iter variants for
dependent drivers to use hpilo through kernel_{read,write}.

Changes since v1:
- Fix the patch format so it could be applied cleanly.

Matt Hsiao (1):
  misc: hpilo: switch .{read,write} ops to .{read,write}_iter

 drivers/misc/hpilo.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

-- 
2.16.6

[PATCH v2 1/1] misc: hpilo: switch .{read,write} ops to .{read,write}_iter

[matt.hsiao]

From: Matt Hsiao <matt.hsiao@hpe.com>

Commit 4d03e3cc59828c82ee89 ("fs: don't allow kernel reads and writes
without iter ops") requested exclusive .{read,write}_iter ops for
kernel_{read,write}. To support dependent drivers to access hpilo by
kernel_{read,write}, switch .{read,write} ops to their iter variants.

Signed-off-by: Matt Hsiao <matt.hsiao@hpe.com>
---
 drivers/misc/hpilo.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
index 8d00df9243c4..5d431a56b7eb 100644
--- a/drivers/misc/hpilo.c
+++ b/drivers/misc/hpilo.c
@@ -23,6 +23,7 @@
 #include <linux/wait.h>
 #include <linux/poll.h>
 #include <linux/slab.h>
+#include <linux/uio.h>
 #include "hpilo.h"
 
 static struct class *ilo_class;
@@ -435,14 +436,14 @@ static void ilo_set_reset(struct ilo_hwinfo *hw)
 	}
 }
 
-static ssize_t ilo_read(struct file *fp, char __user *buf,
-			size_t len, loff_t *off)
+static ssize_t ilo_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
-	int err, found, cnt, pkt_id, pkt_len;
-	struct ccb_data *data = fp->private_data;
+	int err = 0, found, cnt, pkt_id, pkt_len;
+	struct ccb_data *data = iocb->ki_filp->private_data;
 	struct ccb *driver_ccb = &data->driver_ccb;
 	struct ilo_hwinfo *hw = data->ilo_hw;
 	void *pkt;
+	size_t len = iov_iter_count(to), copied;
 
 	if (is_channel_reset(driver_ccb)) {
 		/*
@@ -477,7 +478,9 @@ static ssize_t ilo_read(struct file *fp, char __user *buf,
 	if (pkt_len < len)
 		len = pkt_len;
 
-	err = copy_to_user(buf, pkt, len);
+	copied = copy_to_iter(pkt, len, to);
+	if (unlikely(copied != len))
+		err = -EFAULT;
 
 	/* return the received packet to the queue */
 	ilo_pkt_enqueue(hw, driver_ccb, RECVQ, pkt_id, desc_mem_sz(1));
@@ -485,14 +488,14 @@ static ssize_t ilo_read(struct file *fp, char __user *buf,
 	return err ? -EFAULT : len;
 }
 
-static ssize_t ilo_write(struct file *fp, const char __user *buf,
-			 size_t len, loff_t *off)
+static ssize_t ilo_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
-	int err, pkt_id, pkt_len;
-	struct ccb_data *data = fp->private_data;
+	int err = 0, pkt_id, pkt_len;
+	struct ccb_data *data = iocb->ki_filp->private_data;
 	struct ccb *driver_ccb = &data->driver_ccb;
 	struct ilo_hwinfo *hw = data->ilo_hw;
 	void *pkt;
+	size_t len = iov_iter_count(from), copied;
 
 	if (is_channel_reset(driver_ccb))
 		return -ENODEV;
@@ -506,9 +509,11 @@ static ssize_t ilo_write(struct file *fp, const char __user *buf,
 		len = pkt_len;
 
 	/* on failure, set the len to 0 to return empty packet to the device */
-	err = copy_from_user(pkt, buf, len);
-	if (err)
+	copied = copy_from_iter(pkt, len, from);
+	if (unlikely(copied != len)) {
 		len = 0;
+		err = -EFAULT;
+	}
 
 	/* send the packet */
 	ilo_pkt_enqueue(hw, driver_ccb, SENDQ, pkt_id, len);
@@ -639,8 +644,8 @@ static int ilo_open(struct inode *ip, struct file *fp)
 
 static const struct file_operations ilo_fops = {
 	.owner		= THIS_MODULE,
-	.read		= ilo_read,
-	.write		= ilo_write,
+	.read_iter	= ilo_read_iter,
+	.write_iter	= ilo_write_iter,
 	.poll		= ilo_poll,
 	.open 		= ilo_open,
 	.release 	= ilo_close,
-- 
2.16.6

Re: [PATCH v2 1/1] misc: hpilo: switch .{read,write} ops to .{read,write}_iter

[Greg KH]

On Thu, Jul 14, 2022 at 01:54:52AM +0800, matt.hsiao@hpe.com wrote:
> From: Matt Hsiao <matt.hsiao@hpe.com>
> 
> Commit 4d03e3cc59828c82ee89 ("fs: don't allow kernel reads and writes
> without iter ops") requested exclusive .{read,write}_iter ops for
> kernel_{read,write}. To support dependent drivers to access hpilo by
> kernel_{read,write}, switch .{read,write} ops to their iter variants.
> 
> Signed-off-by: Matt Hsiao <matt.hsiao@hpe.com>

So this fixes a bug?  What commit does this fix?

Should it go to stable branches?  If so, which ones?

But my main question is I have no idea what the changelog means here.
What is a "dependent driver"?  What does "exclusive" mean here?  What is
a iter variant?



> ---
>  drivers/misc/hpilo.c | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
> index 8d00df9243c4..5d431a56b7eb 100644
> --- a/drivers/misc/hpilo.c
> +++ b/drivers/misc/hpilo.c
> @@ -23,6 +23,7 @@
>  #include <linux/wait.h>
>  #include <linux/poll.h>
>  #include <linux/slab.h>
> +#include <linux/uio.h>
>  #include "hpilo.h"
>  
>  static struct class *ilo_class;
> @@ -435,14 +436,14 @@ static void ilo_set_reset(struct ilo_hwinfo *hw)
>  	}
>  }
>  
> -static ssize_t ilo_read(struct file *fp, char __user *buf,
> -			size_t len, loff_t *off)
> +static ssize_t ilo_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  {
> -	int err, found, cnt, pkt_id, pkt_len;
> -	struct ccb_data *data = fp->private_data;
> +	int err = 0, found, cnt, pkt_id, pkt_len;
> +	struct ccb_data *data = iocb->ki_filp->private_data;
>  	struct ccb *driver_ccb = &data->driver_ccb;
>  	struct ilo_hwinfo *hw = data->ilo_hw;
>  	void *pkt;
> +	size_t len = iov_iter_count(to), copied;
>  
>  	if (is_channel_reset(driver_ccb)) {
>  		/*
> @@ -477,7 +478,9 @@ static ssize_t ilo_read(struct file *fp, char __user *buf,
>  	if (pkt_len < len)
>  		len = pkt_len;
>  
> -	err = copy_to_user(buf, pkt, len);
> +	copied = copy_to_iter(pkt, len, to);
> +	if (unlikely(copied != len))

Why unlikely?  If you can prove it is needed in benchmarks, great,
otherwise never add likely/unlikely as they are almost always wrong and
the compiler and cpu can do it better.


> +		err = -EFAULT;
>  
>  	/* return the received packet to the queue */
>  	ilo_pkt_enqueue(hw, driver_ccb, RECVQ, pkt_id, desc_mem_sz(1));
> @@ -485,14 +488,14 @@ static ssize_t ilo_read(struct file *fp, char __user *buf,
>  	return err ? -EFAULT : len;
>  }
>  
> -static ssize_t ilo_write(struct file *fp, const char __user *buf,
> -			 size_t len, loff_t *off)
> +static ssize_t ilo_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  {
> -	int err, pkt_id, pkt_len;
> -	struct ccb_data *data = fp->private_data;
> +	int err = 0, pkt_id, pkt_len;
> +	struct ccb_data *data = iocb->ki_filp->private_data;
>  	struct ccb *driver_ccb = &data->driver_ccb;
>  	struct ilo_hwinfo *hw = data->ilo_hw;
>  	void *pkt;
> +	size_t len = iov_iter_count(from), copied;
>  
>  	if (is_channel_reset(driver_ccb))
>  		return -ENODEV;
> @@ -506,9 +509,11 @@ static ssize_t ilo_write(struct file *fp, const char __user *buf,
>  		len = pkt_len;
>  
>  	/* on failure, set the len to 0 to return empty packet to the device */
> -	err = copy_from_user(pkt, buf, len);
> -	if (err)
> +	copied = copy_from_iter(pkt, len, from);
> +	if (unlikely(copied != len)) {

Same here.

thanks,

greg k-h

Re: [PATCH v2 1/1] misc: hpilo: switch .{read,write} ops to .{read,write}_iter

[Matt Hsiao]

On Wed, Jul 13, 2022 at 08:28:24PM +0200, Greg KH wrote:
> On Thu, Jul 14, 2022 at 01:54:52AM +0800, matt.hsiao@hpe.com wrote:
> > From: Matt Hsiao <matt.hsiao@hpe.com>
> > 
> > Commit 4d03e3cc59828c82ee89 ("fs: don't allow kernel reads and writes
> > without iter ops") requested exclusive .{read,write}_iter ops for
> > kernel_{read,write}. To support dependent drivers to access hpilo by
> > kernel_{read,write}, switch .{read,write} ops to their iter variants.
> > 
> > Signed-off-by: Matt Hsiao <matt.hsiao@hpe.com>
> 
> So this fixes a bug?  What commit does this fix?

No, this is not a bug fix. Please see my explanation for your main question below. 

> 
> Should it go to stable branches?  If so, which ones?

No, it does not need to.

> 
> But my main question is I have no idea what the changelog means here.
> What is a "dependent driver"?  What does "exclusive" mean here?  What is
> a iter variant?

There is an out-of-box driver which is not in the upstream kernel yet
that uses kernel_{read,write} to access the hpilo driver for talking
to the iLO ASIC. Before commit 4d03e3cc59828c82ee89 ("fs: don't allow kernel
reads and writes without iter ops"), kernel_{read,write} would call the
.{read,write} file ops that hpilo already implemented, so there was no problem;
But after that commit, kernel_{read,write} would only allow the .{read,write}_iter
file ops, and disallowed the coexistence of .{read,write} file ops. Accessing
hpilo now fails since it does not have the .{read,write}_iter file ops. To make it
work, this patch implements the .{read,write}_iter file ops and removed the
.{read,write} ones.

> 
> 
> 
> > ---
> >  drivers/misc/hpilo.c | 31 ++++++++++++++++++-------------
> >  1 file changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/misc/hpilo.c b/drivers/misc/hpilo.c
> > index 8d00df9243c4..5d431a56b7eb 100644
> > --- a/drivers/misc/hpilo.c
> > +++ b/drivers/misc/hpilo.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/wait.h>
> >  #include <linux/poll.h>
> >  #include <linux/slab.h>
> > +#include <linux/uio.h>
> >  #include "hpilo.h"
> >  
> >  static struct class *ilo_class;
> > @@ -435,14 +436,14 @@ static void ilo_set_reset(struct ilo_hwinfo *hw)
> >  	}
> >  }
> >  
> > -static ssize_t ilo_read(struct file *fp, char __user *buf,
> > -			size_t len, loff_t *off)
> > +static ssize_t ilo_read_iter(struct kiocb *iocb, struct iov_iter *to)
> >  {
> > -	int err, found, cnt, pkt_id, pkt_len;
> > -	struct ccb_data *data = fp->private_data;
> > +	int err = 0, found, cnt, pkt_id, pkt_len;
> > +	struct ccb_data *data = iocb->ki_filp->private_data;
> >  	struct ccb *driver_ccb = &data->driver_ccb;
> >  	struct ilo_hwinfo *hw = data->ilo_hw;
> >  	void *pkt;
> > +	size_t len = iov_iter_count(to), copied;
> >  
> >  	if (is_channel_reset(driver_ccb)) {
> >  		/*
> > @@ -477,7 +478,9 @@ static ssize_t ilo_read(struct file *fp, char __user *buf,
> >  	if (pkt_len < len)
> >  		len = pkt_len;
> >  
> > -	err = copy_to_user(buf, pkt, len);
> > +	copied = copy_to_iter(pkt, len, to);
> > +	if (unlikely(copied != len))
> 
> Why unlikely?  If you can prove it is needed in benchmarks, great,
> otherwise never add likely/unlikely as they are almost always wrong and
> the compiler and cpu can do it better.

Will remove it in the next verion of patch.

> 
> 
> > +		err = -EFAULT;
> >  
> >  	/* return the received packet to the queue */
> >  	ilo_pkt_enqueue(hw, driver_ccb, RECVQ, pkt_id, desc_mem_sz(1));
> > @@ -485,14 +488,14 @@ static ssize_t ilo_read(struct file *fp, char __user *buf,
> >  	return err ? -EFAULT : len;
> >  }
> >  
> > -static ssize_t ilo_write(struct file *fp, const char __user *buf,
> > -			 size_t len, loff_t *off)
> > +static ssize_t ilo_write_iter(struct kiocb *iocb, struct iov_iter *from)
> >  {
> > -	int err, pkt_id, pkt_len;
> > -	struct ccb_data *data = fp->private_data;
> > +	int err = 0, pkt_id, pkt_len;
> > +	struct ccb_data *data = iocb->ki_filp->private_data;
> >  	struct ccb *driver_ccb = &data->driver_ccb;
> >  	struct ilo_hwinfo *hw = data->ilo_hw;
> >  	void *pkt;
> > +	size_t len = iov_iter_count(from), copied;
> >  
> >  	if (is_channel_reset(driver_ccb))
> >  		return -ENODEV;
> > @@ -506,9 +509,11 @@ static ssize_t ilo_write(struct file *fp, const char __user *buf,
> >  		len = pkt_len;
> >  
> >  	/* on failure, set the len to 0 to return empty packet to the device */
> > -	err = copy_from_user(pkt, buf, len);
> > -	if (err)
> > +	copied = copy_from_iter(pkt, len, from);
> > +	if (unlikely(copied != len)) {
> 
> Same here.

Will remove it in the next verion of patch.

> 
> thanks,
> 
> greg k-h

Re: [PATCH v2 1/1] misc: hpilo: switch .{read,write} ops to .{read,write}_iter

[Greg KH]

On Fri, Jul 15, 2022 at 04:55:57PM +0800, Matt Hsiao wrote:
> On Wed, Jul 13, 2022 at 08:28:24PM +0200, Greg KH wrote:
> > But my main question is I have no idea what the changelog means here.
> > What is a "dependent driver"?  What does "exclusive" mean here?  What is
> > a iter variant?
> 
> There is an out-of-box driver which is not in the upstream kernel yet
> that uses kernel_{read,write} to access the hpilo driver for talking
> to the iLO ASIC. Before commit 4d03e3cc59828c82ee89 ("fs: don't allow kernel
> reads and writes without iter ops"), kernel_{read,write} would call the
> .{read,write} file ops that hpilo already implemented, so there was no problem;
> But after that commit, kernel_{read,write} would only allow the .{read,write}_iter
> file ops, and disallowed the coexistence of .{read,write} file ops. Accessing
> hpilo now fails since it does not have the .{read,write}_iter file ops. To make it
> work, this patch implements the .{read,write}_iter file ops and removed the
> .{read,write} ones.

For obvious reasons, we can not take any changes for any out-of-tree
code.  Nor would you ever want me to do so.

So we can't take this, sorry.  Please work with your management to get
the drivers upstream and then we will be glad to take these types of
changes, as they well know the price they have to pay to have
out-of-tree code.

Also, your management knows we can't take changes for out-of-tree code.
They know better than to put you into this uncomfortable position.  This
is on them, not you, to resolve.  I suggest you have a stern talking to
with them as they are at fault here.

good luck!

greg k-h