* [PATCH] fsi: fix bogos error returns from cfam_read and cfam_write
@ 2019-11-22 23:31 Colin King
2019-11-28 22:32 ` Andrew Jeffery
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Colin King @ 2019-11-22 23:31 UTC (permalink / raw)
To: Jeremy Kerr, Joel Stanley, Alistar Popple, Eddie James,
Benjamin Herrenschmidt, linux-fsi
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
In the case where errors occur in functions cfam_read and cfam_write
the error return code in rc is not returned and a bogus non-error
count size is returned instead. Fix this by returning the correct
error code when an error occurs or the count size if the functions
worked correctly.
Addresses-Coverity: ("Unused value")
Fixes: d1dcd6782576 ("fsi: Add cfam char devices")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/fsi/fsi-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 8244da8a7241..c3885b138ead 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -718,7 +718,7 @@ static ssize_t cfam_read(struct file *filep, char __user *buf, size_t count,
rc = count;
fail:
*offset = off;
- return count;
+ return rc;
}
static ssize_t cfam_write(struct file *filep, const char __user *buf,
@@ -755,7 +755,7 @@ static ssize_t cfam_write(struct file *filep, const char __user *buf,
rc = count;
fail:
*offset = off;
- return count;
+ return rc;
}
static loff_t cfam_llseek(struct file *file, loff_t offset, int whence)
--
2.24.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] fsi: fix bogos error returns from cfam_read and cfam_write
2019-11-22 23:31 [PATCH] fsi: fix bogos error returns from cfam_read and cfam_write Colin King
@ 2019-11-28 22:32 ` Andrew Jeffery
2019-11-28 23:20 ` Joel Stanley
2019-11-29 3:24 ` [PATCH] [PATCH v2] fsi: fix bogus " Jeremy Kerr
2 siblings, 0 replies; 5+ messages in thread
From: Andrew Jeffery @ 2019-11-28 22:32 UTC (permalink / raw)
To: Colin King, Jeremy Kerr, Joel Stanley, Alistair Popple,
Eddie James, Benjamin Herrenschmidt, linux-fsi
Cc: kernel-janitors, linux-kernel
On Sat, 23 Nov 2019, at 10:01, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> In the case where errors occur in functions cfam_read and cfam_write
> the error return code in rc is not returned and a bogus non-error
> count size is returned instead. Fix this by returning the correct
> error code when an error occurs or the count size if the functions
> worked correctly.
>
> Addresses-Coverity: ("Unused value")
> Fixes: d1dcd6782576 ("fsi: Add cfam char devices")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reviewed-by: Andrew Jeffery <andrew@aj.id.au>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fsi: fix bogos error returns from cfam_read and cfam_write
2019-11-22 23:31 [PATCH] fsi: fix bogos error returns from cfam_read and cfam_write Colin King
2019-11-28 22:32 ` Andrew Jeffery
@ 2019-11-28 23:20 ` Joel Stanley
2019-11-29 3:24 ` [PATCH] [PATCH v2] fsi: fix bogus " Jeremy Kerr
2 siblings, 0 replies; 5+ messages in thread
From: Joel Stanley @ 2019-11-28 23:20 UTC (permalink / raw)
To: Colin King
Cc: Jeremy Kerr, Alistar Popple, Eddie James, Benjamin Herrenschmidt,
linux-fsi, kernel-janitors, Linux Kernel Mailing List
Hi Colin,
On Fri, 22 Nov 2019 at 23:31, Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> In the case where errors occur in functions cfam_read and cfam_write
> the error return code in rc is not returned and a bogus non-error
> count size is returned instead. Fix this by returning the correct
> error code when an error occurs or the count size if the functions
> worked correctly.
You're correct that if there's an error we need to return an error.
However the other case is when there's a partial read that completed.
We already advance the file offset, but I think we should also return
the number of bytes successfully read.
Cheers,
Joel
>
> Addresses-Coverity: ("Unused value")
> Fixes: d1dcd6782576 ("fsi: Add cfam char devices")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/fsi/fsi-core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index 8244da8a7241..c3885b138ead 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -718,7 +718,7 @@ static ssize_t cfam_read(struct file *filep, char __user *buf, size_t count,
> rc = count;
> fail:
> *offset = off;
> - return count;
> + return rc;
> }
>
> static ssize_t cfam_write(struct file *filep, const char __user *buf,
> @@ -755,7 +755,7 @@ static ssize_t cfam_write(struct file *filep, const char __user *buf,
> rc = count;
> fail:
> *offset = off;
> - return count;
> + return rc;
> }
>
> static loff_t cfam_llseek(struct file *file, loff_t offset, int whence)
> --
> 2.24.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] [PATCH v2] fsi: fix bogus error returns from cfam_read and cfam_write
2019-11-22 23:31 [PATCH] fsi: fix bogos error returns from cfam_read and cfam_write Colin King
2019-11-28 22:32 ` Andrew Jeffery
2019-11-28 23:20 ` Joel Stanley
@ 2019-11-29 3:24 ` Jeremy Kerr
2019-11-29 9:03 ` Colin Ian King
2 siblings, 1 reply; 5+ messages in thread
From: Jeremy Kerr @ 2019-11-29 3:24 UTC (permalink / raw)
To: Joel Stanley, Alistar Popple, Eddie James,
Benjamin Herrenschmidt, Colin King, linux-fsi
Cc: kernel-janitors, linux-kernel
Based on a static analysis report and original patch from Colin Ian King
<colin.king@canonical.com>.
Currently, we may drop error values from cfam_read and cfam_write. This
change returns the actual error on failure, but a partial read/write will
take precedence.
Addresses-Coverity: ("Unused value")
Fixes: d1dcd6782576 ("fsi: Add cfam char devices")
Reported-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
Colin: thanks for the report and patch. I think this is a more complete
fix, as we want to preseve any partial read/write status if a failure
happens mid-way through an operation. Let me know if you (or the
coverity analysis) have any feedback.
---
drivers/fsi/fsi-core.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 71c6f9fef648..3158a78c2e94 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -699,6 +699,8 @@ static ssize_t cfam_read(struct file *filep, char __user *buf, size_t count,
if (off > 0xffffffff || count > 0xffffffff || off + count > 0xffffffff)
return -EINVAL;
+ rc = 0;
+
for (total_len = 0; total_len < count; total_len += read_len) {
__be32 data;
@@ -707,18 +709,22 @@ static ssize_t cfam_read(struct file *filep, char __user *buf, size_t count,
rc = fsi_slave_read(slave, off, &data, read_len);
if (rc)
- goto fail;
+ break;
rc = copy_to_user(buf + total_len, &data, read_len);
if (rc) {
rc = -EFAULT;
- goto fail;
+ break;
}
off += read_len;
}
- rc = count;
- fail:
+
+ /* if we've read any data, we want that to be returned in
+ * preference to an error state */
+ if (total_len)
+ rc = total_len;
+
*offset = off;
- return count;
+ return rc;
}
static ssize_t cfam_write(struct file *filep, const char __user *buf,
@@ -736,6 +742,8 @@ static ssize_t cfam_write(struct file *filep, const char __user *buf,
if (off > 0xffffffff || count > 0xffffffff || off + count > 0xffffffff)
return -EINVAL;
+ rc = 0;
+
for (total_len = 0; total_len < count; total_len += write_len) {
__be32 data;
@@ -745,17 +753,21 @@ static ssize_t cfam_write(struct file *filep, const char __user *buf,
rc = copy_from_user(&data, buf + total_len, write_len);
if (rc) {
rc = -EFAULT;
- goto fail;
+ break;
}
rc = fsi_slave_write(slave, off, &data, write_len);
if (rc)
- goto fail;
+ break;
off += write_len;
}
- rc = count;
- fail:
+
+ /* if we've written any data, we want to indicate that partial write
+ * instead of any mid-stream error */
+ if (total_len)
+ rc = total_len;
+
*offset = off;
- return count;
+ return rc;
}
static loff_t cfam_llseek(struct file *file, loff_t offset, int whence)
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] [PATCH v2] fsi: fix bogus error returns from cfam_read and cfam_write
2019-11-29 3:24 ` [PATCH] [PATCH v2] fsi: fix bogus " Jeremy Kerr
@ 2019-11-29 9:03 ` Colin Ian King
0 siblings, 0 replies; 5+ messages in thread
From: Colin Ian King @ 2019-11-29 9:03 UTC (permalink / raw)
To: Jeremy Kerr, Joel Stanley, Alistar Popple, Eddie James,
Benjamin Herrenschmidt, linux-fsi
Cc: kernel-janitors, linux-kernel
On 29/11/2019 03:24, Jeremy Kerr wrote:
> Based on a static analysis report and original patch from Colin Ian King
> <colin.king@canonical.com>.
>
> Currently, we may drop error values from cfam_read and cfam_write. This
> change returns the actual error on failure, but a partial read/write will
> take precedence.
>
> Addresses-Coverity: ("Unused value")
> Fixes: d1dcd6782576 ("fsi: Add cfam char devices")
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
>
> ---
> Colin: thanks for the report and patch. I think this is a more complete
> fix, as we want to preseve any partial read/write status if a failure
> happens mid-way through an operation. Let me know if you (or the
> coverity analysis) have any feedback.
Looks good to me. Thanks Jeremy.
>
> ---
>
> drivers/fsi/fsi-core.c | 32 ++++++++++++++++++++++----------
> 1 file changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index 71c6f9fef648..3158a78c2e94 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -699,6 +699,8 @@ static ssize_t cfam_read(struct file *filep, char __user *buf, size_t count,
> if (off > 0xffffffff || count > 0xffffffff || off + count > 0xffffffff)
> return -EINVAL;
>
> + rc = 0;
> +
> for (total_len = 0; total_len < count; total_len += read_len) {
> __be32 data;
>
> @@ -707,18 +709,22 @@ static ssize_t cfam_read(struct file *filep, char __user *buf, size_t count,
>
> rc = fsi_slave_read(slave, off, &data, read_len);
> if (rc)
> - goto fail;
> + break;
> rc = copy_to_user(buf + total_len, &data, read_len);
> if (rc) {
> rc = -EFAULT;
> - goto fail;
> + break;
> }
> off += read_len;
> }
> - rc = count;
> - fail:
> +
> + /* if we've read any data, we want that to be returned in
> + * preference to an error state */
> + if (total_len)
> + rc = total_len;
> +
> *offset = off;
> - return count;
> + return rc;
> }
>
> static ssize_t cfam_write(struct file *filep, const char __user *buf,
> @@ -736,6 +742,8 @@ static ssize_t cfam_write(struct file *filep, const char __user *buf,
> if (off > 0xffffffff || count > 0xffffffff || off + count > 0xffffffff)
> return -EINVAL;
>
> + rc = 0;
> +
> for (total_len = 0; total_len < count; total_len += write_len) {
> __be32 data;
>
> @@ -745,17 +753,21 @@ static ssize_t cfam_write(struct file *filep, const char __user *buf,
> rc = copy_from_user(&data, buf + total_len, write_len);
> if (rc) {
> rc = -EFAULT;
> - goto fail;
> + break;
> }
> rc = fsi_slave_write(slave, off, &data, write_len);
> if (rc)
> - goto fail;
> + break;
> off += write_len;
> }
> - rc = count;
> - fail:
> +
> + /* if we've written any data, we want to indicate that partial write
> + * instead of any mid-stream error */
> + if (total_len)
> + rc = total_len;
> +
> *offset = off;
> - return count;
> + return rc;
> }
>
> static loff_t cfam_llseek(struct file *file, loff_t offset, int whence)
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-11-29 9:04 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-22 23:31 [PATCH] fsi: fix bogos error returns from cfam_read and cfam_write Colin King
2019-11-28 22:32 ` Andrew Jeffery
2019-11-28 23:20 ` Joel Stanley
2019-11-29 3:24 ` [PATCH] [PATCH v2] fsi: fix bogus " Jeremy Kerr
2019-11-29 9:03 ` Colin Ian King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).