* [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).