[PATCH v2] vfio-ccw: Do not read region ret_code after write

Eric Farman posted 1 patch 3 years, 2 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210303160739.2179378-1-farman@linux.ibm.com
Maintainers: Eric Farman <farman@linux.ibm.com>, Cornelia Huck <cohuck@redhat.com>, Thomas Huth <thuth@redhat.com>, Alex Williamson <alex.williamson@redhat.com>
hw/vfio/ccw.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
[PATCH v2] vfio-ccw: Do not read region ret_code after write
Posted by Eric Farman 3 years, 2 months ago
A pwrite() call returns the number of bytes written (or -1 on error),
and vfio-ccw compares this number with the size of the region to
determine if an error had occurred or not.

If they are not equal, this is a failure and the errno is used to
determine exactly how things failed. An errno of zero is possible
(though unlikely) in this situation and would be translated to a
successful operation.

If they ARE equal, the ret_code field is read from the region to
determine how to proceed. While the kernel sets the ret_code field
as necessary, the region and thus this field is not "written back"
to the user. So the value can only be what it was initialized to,
which is zero.

So, let's convert an unexpected length with errno of zero to a
return code of -EFAULT, and explicitly set an expected length to
a return code of zero. This will be a little safer and clearer.

Suggested-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---

Notes:
    Changes v1->v2:
     - Add code to handle an unexpected length being returned,
       but errno is not set. [CH]
    
    v1: https://lore.kernel.org/qemu-devel/cc440ee8-28c5-3208-19db-ebb48ea60e9d@linux.ibm.com/

 hw/vfio/ccw.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index bc78a0ad76..b2df708e4b 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -104,9 +104,9 @@ again:
             goto again;
         }
         error_report("vfio-ccw: write I/O region failed with errno=%d", errno);
-        ret = -errno;
+        ret = errno ? -errno : -EFAULT;
     } else {
-        ret = region->ret_code;
+        ret = 0;
     }
     switch (ret) {
     case 0:
@@ -192,9 +192,9 @@ again:
             goto again;
         }
         error_report("vfio-ccw: write cmd region failed with errno=%d", errno);
-        ret = -errno;
+        ret = errno ? -errno : -EFAULT;
     } else {
-        ret = region->ret_code;
+        ret = 0;
     }
     switch (ret) {
     case 0:
@@ -232,9 +232,9 @@ again:
             goto again;
         }
         error_report("vfio-ccw: write cmd region failed with errno=%d", errno);
-        ret = -errno;
+        ret = errno ? -errno : -EFAULT;
     } else {
-        ret = region->ret_code;
+        ret = 0;
     }
     switch (ret) {
     case 0:
-- 
2.25.1


Re: [PATCH v2] vfio-ccw: Do not read region ret_code after write
Posted by Cornelia Huck 3 years, 2 months ago
On Wed,  3 Mar 2021 17:07:39 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> A pwrite() call returns the number of bytes written (or -1 on error),
> and vfio-ccw compares this number with the size of the region to
> determine if an error had occurred or not.
> 
> If they are not equal, this is a failure and the errno is used to
> determine exactly how things failed. An errno of zero is possible
> (though unlikely) in this situation and would be translated to a
> successful operation.
> 
> If they ARE equal, the ret_code field is read from the region to
> determine how to proceed. While the kernel sets the ret_code field
> as necessary, the region and thus this field is not "written back"
> to the user. So the value can only be what it was initialized to,
> which is zero.
> 
> So, let's convert an unexpected length with errno of zero to a
> return code of -EFAULT, and explicitly set an expected length to
> a return code of zero. This will be a little safer and clearer.
> 
> Suggested-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
> 
> Notes:
>     Changes v1->v2:
>      - Add code to handle an unexpected length being returned,
>        but errno is not set. [CH]
>     
>     v1: https://lore.kernel.org/qemu-devel/cc440ee8-28c5-3208-19db-ebb48ea60e9d@linux.ibm.com/
> 
>  hw/vfio/ccw.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

Thanks, applied.