[patch 2/2] ACPI / debugger: copy_to_user doesn't return errors

[patch 2/2] ACPI / debugger: copy_to_user doesn't return errors

[Dan Carpenter]

The copy_to/from_user() functions don't return error codes, they return
the number of bytes remaining.  We had intended to return -EFUALT here.
We actually have already checked access_ok() in an earlier function so
I don't think these functions will fail but let's fix it anyway.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
index f2c92ab..2a1777b 100644
--- a/drivers/acpi/acpi_dbg.c
+++ b/drivers/acpi/acpi_dbg.c
@@ -592,9 +592,10 @@ static int acpi_aml_read_user(char __user *buf, int len)
 	smp_rmb();
 	p = &crc->buf[crc->tail];
 	n = min(len, circ_count_to_end(crc));
-	ret = copy_to_user(buf, p, n);
-	if (IS_ERR_VALUE(ret))
+	if (copy_to_user(buf, p, n)) {
+		ret = -EFAULT;
 		goto out;
+	}
 	/* sync tail after removing logs */
 	smp_mb();
 	crc->tail = (crc->tail + n) & (ACPI_AML_BUF_SIZE - 1);
@@ -661,9 +662,10 @@ static int acpi_aml_write_user(const char __user *buf, int len)
 	smp_mb();
 	p = &crc->buf[crc->head];
 	n = min(len, circ_space_to_end(crc));
-	ret = copy_from_user(p, buf, n);
-	if (IS_ERR_VALUE(ret))
+	if (copy_from_user(p, buf, n)) {
+		ret = -EFAULT;
 		goto out;
+	}
 	/* sync head after inserting cmds */
 	smp_wmb();
 	crc->head = (crc->head + n) & (ACPI_AML_BUF_SIZE - 1);

[patch 2/2] ACPI / debugger: copy_to_user doesn't return errors

[Dan Carpenter]

The copy_to/from_user() functions don't return error codes, they return
the number of bytes remaining.  We had intended to return -EFUALT here.
We actually have already checked access_ok() in an earlier function so
I don't think these functions will fail but let's fix it anyway.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
index f2c92ab..2a1777b 100644
--- a/drivers/acpi/acpi_dbg.c
+++ b/drivers/acpi/acpi_dbg.c
@@ -592,9 +592,10 @@ static int acpi_aml_read_user(char __user *buf, int len)
 	smp_rmb();
 	p = &crc->buf[crc->tail];
 	n = min(len, circ_count_to_end(crc));
-	ret = copy_to_user(buf, p, n);
-	if (IS_ERR_VALUE(ret))
+	if (copy_to_user(buf, p, n)) {
+		ret = -EFAULT;
 		goto out;
+	}
 	/* sync tail after removing logs */
 	smp_mb();
 	crc->tail = (crc->tail + n) & (ACPI_AML_BUF_SIZE - 1);
@@ -661,9 +662,10 @@ static int acpi_aml_write_user(const char __user *buf, int len)
 	smp_mb();
 	p = &crc->buf[crc->head];
 	n = min(len, circ_space_to_end(crc));
-	ret = copy_from_user(p, buf, n);
-	if (IS_ERR_VALUE(ret))
+	if (copy_from_user(p, buf, n)) {
+		ret = -EFAULT;
 		goto out;
+	}
 	/* sync head after inserting cmds */
 	smp_wmb();
 	crc->head = (crc->head + n) & (ACPI_AML_BUF_SIZE - 1);

Re: [patch 2/2] ACPI / debugger: copy_to_user doesn't return errors

[Julia Lawall]

On Thu, 24 Dec 2015, Dan Carpenter wrote:

> The copy_to/from_user() functions don't return error codes, they return
> the number of bytes remaining.  We had intended to return -EFUALT here.
> We actually have already checked access_ok() in an earlier function so
> I don't think these functions will fail but let's fix it anyway.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
> index f2c92ab..2a1777b 100644
> --- a/drivers/acpi/acpi_dbg.c
> +++ b/drivers/acpi/acpi_dbg.c
> @@ -592,9 +592,10 @@ static int acpi_aml_read_user(char __user *buf, int len)
>  	smp_rmb();
>  	p = &crc->buf[crc->tail];
>  	n = min(len, circ_count_to_end(crc));
> -	ret = copy_to_user(buf, p, n);
> -	if (IS_ERR_VALUE(ret))

I'm not familiar with IS_ERR_VALUE.  Is it to allow functions that return 
unsigned values to also return negative error codes?

thanks,
julia

> +	if (copy_to_user(buf, p, n)) {
> +		ret = -EFAULT;
>  		goto out;
> +	}
>  	/* sync tail after removing logs */
>  	smp_mb();
>  	crc->tail = (crc->tail + n) & (ACPI_AML_BUF_SIZE - 1);
> @@ -661,9 +662,10 @@ static int acpi_aml_write_user(const char __user *buf, int len)
>  	smp_mb();
>  	p = &crc->buf[crc->head];
>  	n = min(len, circ_space_to_end(crc));
> -	ret = copy_from_user(p, buf, n);
> -	if (IS_ERR_VALUE(ret))
> +	if (copy_from_user(p, buf, n)) {
> +		ret = -EFAULT;
>  		goto out;
> +	}
>  	/* sync head after inserting cmds */
>  	smp_wmb();
>  	crc->head = (crc->head + n) & (ACPI_AML_BUF_SIZE - 1);
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch 2/2] ACPI / debugger: copy_to_user doesn't return errors

[Julia Lawall]

On Thu, 24 Dec 2015, Dan Carpenter wrote:

> The copy_to/from_user() functions don't return error codes, they return
> the number of bytes remaining.  We had intended to return -EFUALT here.
> We actually have already checked access_ok() in an earlier function so
> I don't think these functions will fail but let's fix it anyway.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
> index f2c92ab..2a1777b 100644
> --- a/drivers/acpi/acpi_dbg.c
> +++ b/drivers/acpi/acpi_dbg.c
> @@ -592,9 +592,10 @@ static int acpi_aml_read_user(char __user *buf, int len)
>  	smp_rmb();
>  	p = &crc->buf[crc->tail];
>  	n = min(len, circ_count_to_end(crc));
> -	ret = copy_to_user(buf, p, n);
> -	if (IS_ERR_VALUE(ret))

I'm not familiar with IS_ERR_VALUE.  Is it to allow functions that return 
unsigned values to also return negative error codes?

thanks,
julia

> +	if (copy_to_user(buf, p, n)) {
> +		ret = -EFAULT;
>  		goto out;
> +	}
>  	/* sync tail after removing logs */
>  	smp_mb();
>  	crc->tail = (crc->tail + n) & (ACPI_AML_BUF_SIZE - 1);
> @@ -661,9 +662,10 @@ static int acpi_aml_write_user(const char __user *buf, int len)
>  	smp_mb();
>  	p = &crc->buf[crc->head];
>  	n = min(len, circ_space_to_end(crc));
> -	ret = copy_from_user(p, buf, n);
> -	if (IS_ERR_VALUE(ret))
> +	if (copy_from_user(p, buf, n)) {
> +		ret = -EFAULT;
>  		goto out;
> +	}
>  	/* sync head after inserting cmds */
>  	smp_wmb();
>  	crc->head = (crc->head + n) & (ACPI_AML_BUF_SIZE - 1);
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [patch 2/2] ACPI / debugger: copy_to_user doesn't return errors

[Dan Carpenter]

On Thu, Dec 24, 2015 at 08:39:06AM +0100, Julia Lawall wrote:
> On Thu, 24 Dec 2015, Dan Carpenter wrote:
> 
> > The copy_to/from_user() functions don't return error codes, they return
> > the number of bytes remaining.  We had intended to return -EFUALT here.
> > We actually have already checked access_ok() in an earlier function so
> > I don't think these functions will fail but let's fix it anyway.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
> > index f2c92ab..2a1777b 100644
> > --- a/drivers/acpi/acpi_dbg.c
> > +++ b/drivers/acpi/acpi_dbg.c
> > @@ -592,9 +592,10 @@ static int acpi_aml_read_user(char __user *buf, int len)
> >  	smp_rmb();
> >  	p = &crc->buf[crc->tail];
> >  	n = min(len, circ_count_to_end(crc));
> > -	ret = copy_to_user(buf, p, n);
> > -	if (IS_ERR_VALUE(ret))
> 
> I'm not familiar with IS_ERR_VALUE.  Is it to allow functions that
> return unsigned values to also return negative error codes?

Yes, but here it is used as a substitue for checking if (ret < 0) or if
(ret >= 0).  I would have prefered to open code it.  Also I prefer
if (ret) for when the value is either zero or negative error codes but
here we have standardized on IS_ERR_VALUE(ret) for everything.

regards,
dan carpenter

Re: [patch 2/2] ACPI / debugger: copy_to_user doesn't return errors

[Dan Carpenter]

On Thu, Dec 24, 2015 at 08:39:06AM +0100, Julia Lawall wrote:
> On Thu, 24 Dec 2015, Dan Carpenter wrote:
> 
> > The copy_to/from_user() functions don't return error codes, they return
> > the number of bytes remaining.  We had intended to return -EFUALT here.
> > We actually have already checked access_ok() in an earlier function so
> > I don't think these functions will fail but let's fix it anyway.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
> > index f2c92ab..2a1777b 100644
> > --- a/drivers/acpi/acpi_dbg.c
> > +++ b/drivers/acpi/acpi_dbg.c
> > @@ -592,9 +592,10 @@ static int acpi_aml_read_user(char __user *buf, int len)
> >  	smp_rmb();
> >  	p = &crc->buf[crc->tail];
> >  	n = min(len, circ_count_to_end(crc));
> > -	ret = copy_to_user(buf, p, n);
> > -	if (IS_ERR_VALUE(ret))
> 
> I'm not familiar with IS_ERR_VALUE.  Is it to allow functions that
> return unsigned values to also return negative error codes?

Yes, but here it is used as a substitue for checking if (ret < 0) or if
(ret >= 0).  I would have prefered to open code it.  Also I prefer
if (ret) for when the value is either zero or negative error codes but
here we have standardized on IS_ERR_VALUE(ret) for everything.

regards,
dan carpenter

RE: [patch 2/2] ACPI / debugger: copy_to_user doesn't return errors

[Zheng, Lv]

Hi,

> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, December 25, 2015 3:16 AM
> Subject: Re: [patch 2/2] ACPI / debugger: copy_to_user doesn't return errors
> 
> On Thu, Dec 24, 2015 at 08:39:06AM +0100, Julia Lawall wrote:
> > On Thu, 24 Dec 2015, Dan Carpenter wrote:
> >
> > > The copy_to/from_user() functions don't return error codes, they return
> > > the number of bytes remaining.  We had intended to return -EFUALT here.
> > > We actually have already checked access_ok() in an earlier function so
> > > I don't think these functions will fail but let's fix it anyway.
> > >
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> > >
> > > diff --git a/drivers/acpi/acpi_dbg.c b/drivers/acpi/acpi_dbg.c
> > > index f2c92ab..2a1777b 100644
> > > --- a/drivers/acpi/acpi_dbg.c
> > > +++ b/drivers/acpi/acpi_dbg.c
> > > @@ -592,9 +592,10 @@ static int acpi_aml_read_user(char __user *buf,
> int len)
> > >  	smp_rmb();
> > >  	p = &crc->buf[crc->tail];
> > >  	n = min(len, circ_count_to_end(crc));
> > > -	ret = copy_to_user(buf, p, n);
> > > -	if (IS_ERR_VALUE(ret))
> >
> > I'm not familiar with IS_ERR_VALUE.  Is it to allow functions that
> > return unsigned values to also return negative error codes?
> 
> Yes, but here it is used as a substitue for checking if (ret < 0) or if
> (ret >= 0).  I would have prefered to open code it.  Also I prefer
> if (ret) for when the value is either zero or negative error codes but
> here we have standardized on IS_ERR_VALUE(ret) for everything.
[Lv Zheng] 
This patch looks correct.
It's good for me to know the rule for copy_to/from_user().
Thanks for pointing out.

Thanks and best regards
-Lv

> 
> regards,
> dan carpenter