[PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition

[PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition

[Gleb Fotengauer-Malinovskiy]

This ioctl actually writes to parameter too.

Fixes: 4036e387 ("KVM: s390: ioctls to get and set guest storage attributes")
Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
---
 include/uapi/linux/kvm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index c0b6dfe..ebd604c 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1351,7 +1351,7 @@ struct kvm_s390_ucas_mapping {
 /* Available with KVM_CAP_X86_SMM */
 #define KVM_SMI                   _IO(KVMIO,   0xb7)
 /* Available with KVM_CAP_S390_CMMA_MIGRATION */
-#define KVM_S390_GET_CMMA_BITS      _IOW(KVMIO, 0xb8, struct kvm_s390_cmma_log)
+#define KVM_S390_GET_CMMA_BITS      _IOWR(KVMIO, 0xb8, struct kvm_s390_cmma_log)
 #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
-- 
glebfm

Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition

[Christian Borntraeger]

On 07/10/2017 04:44 PM, Gleb Fotengauer-Malinovskiy wrote:
> This ioctl actually writes to parameter too.

Maybe rephrase that to:
The kernel does not only read struct kvm_s390_cmma_log for KVM_S390_GET_CMMA_BITS,
it also writes back a return value making this _IOWR instead of _IOW.


> Fixes: 4036e387 ("KVM: s390: ioctls to get and set guest storage attributes")
> Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>


Out of curiosity, how did you notice that? 

> ---
>  include/uapi/linux/kvm.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index c0b6dfe..ebd604c 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1351,7 +1351,7 @@ struct kvm_s390_ucas_mapping {
>  /* Available with KVM_CAP_X86_SMM */
>  #define KVM_SMI                   _IO(KVMIO,   0xb7)
>  /* Available with KVM_CAP_S390_CMMA_MIGRATION */
> -#define KVM_S390_GET_CMMA_BITS      _IOW(KVMIO, 0xb8, struct kvm_s390_cmma_log)
> +#define KVM_S390_GET_CMMA_BITS      _IOWR(KVMIO, 0xb8, struct kvm_s390_cmma_log)
>  #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
> 
>  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
> 

[PATCH v2] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition

[Gleb Fotengauer-Malinovskiy]

In case of KVM_S390_GET_CMMA_BITS, the kernel does not only read struct
kvm_s390_cmma_log passed from userspace (which constitutes _IOC_WRITE),
it also writes back a return value (which constitutes _IOC_READ) making
this an _IOWR ioctl instead of _IOW.

Fixes: 4036e387 ("KVM: s390: ioctls to get and set guest storage attributes")
Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 include/uapi/linux/kvm.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index c0b6dfe..ebd604c 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1351,7 +1351,7 @@ struct kvm_s390_ucas_mapping {
 /* Available with KVM_CAP_X86_SMM */
 #define KVM_SMI                   _IO(KVMIO,   0xb7)
 /* Available with KVM_CAP_S390_CMMA_MIGRATION */
-#define KVM_S390_GET_CMMA_BITS      _IOW(KVMIO, 0xb8, struct kvm_s390_cmma_log)
+#define KVM_S390_GET_CMMA_BITS      _IOWR(KVMIO, 0xb8, struct kvm_s390_cmma_log)
 #define KVM_S390_SET_CMMA_BITS      _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)

-- 
glebfm

Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition

[Gleb Fotengauer-Malinovskiy]

On Mon, Jul 10, 2017 at 08:43:12PM +0200, Christian Borntraeger wrote:
> On 07/10/2017 04:44 PM, Gleb Fotengauer-Malinovskiy wrote:
> > This ioctl actually writes to parameter too.
> 
> Maybe rephrase that to:
> The kernel does not only read struct kvm_s390_cmma_log for KVM_S390_GET_CMMA_BITS,
> it also writes back a return value making this _IOWR instead of _IOW.

Ok, see v2.

> > Fixes: 4036e387 ("KVM: s390: ioctls to get and set guest storage attributes")
> > Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 
> Out of curiosity, how did you notice that? 

I regenerated strace's ioctl lists.  It was obvious from the diff that
*GET* and *SET* could not be both _IOC_WRITE.

-- 
glebfm

Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition

[Christian Borntraeger]

On 07/10/2017 11:23 PM, Gleb Fotengauer-Malinovskiy wrote:
> On Mon, Jul 10, 2017 at 08:43:12PM +0200, Christian Borntraeger wrote:
>> On 07/10/2017 04:44 PM, Gleb Fotengauer-Malinovskiy wrote:
>>> This ioctl actually writes to parameter too.
>>
>> Maybe rephrase that to:
>> The kernel does not only read struct kvm_s390_cmma_log for KVM_S390_GET_CMMA_BITS,
>> it also writes back a return value making this _IOWR instead of _IOW.
> 
> Ok, see v2.
> 
>>> Fixes: 4036e387 ("KVM: s390: ioctls to get and set guest storage attributes")
>>> Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
>> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>
>>
>> Out of curiosity, how did you notice that? 
> 
> I regenerated strace's ioctl lists.  It was obvious from the diff that
> *GET* and *SET* could not be both _IOC_WRITE.
> 

In fact we do have multiple GET/SET ioctls in KVM, where both provide a control
block that is _IOC_WRITE only. That control block then has an address that will 
be read/written to depending on get/set. 
E.g. look at 
#define KVM_SET_DEVICE_ATTR       _IOW(KVMIO,  0xe1, struct kvm_device_attr)
#define KVM_GET_DEVICE_ATTR       _IOW(KVMIO,  0xe2, struct kvm_device_attr)

but as far as I understand, the direction hints only qualify the referenced
struct and not the side effects. So for KVM_*_DEVICE_ATTR it is correct to have
IOW for both cases.

But for GET_CMMA we do indeed write back data. 

Paolo, Radim,

if we want to fix the direction hint, it would be good to merge this in as soon
as possible. The new interface was added during this merge window.

Re: [PATCH] KVM: s390: Fix KVM_S390_GET_CMMA_BITS ioctl definition

[Radim Krčmář]

2017-07-11 10:21+0200, Christian Borntraeger:
> On 07/10/2017 11:23 PM, Gleb Fotengauer-Malinovskiy wrote:
> > On Mon, Jul 10, 2017 at 08:43:12PM +0200, Christian Borntraeger wrote:
> >> On 07/10/2017 04:44 PM, Gleb Fotengauer-Malinovskiy wrote:
> >>> This ioctl actually writes to parameter too.
> >>
> >> Maybe rephrase that to:
> >> The kernel does not only read struct kvm_s390_cmma_log for KVM_S390_GET_CMMA_BITS,
> >> it also writes back a return value making this _IOWR instead of _IOW.
> > 
> > Ok, see v2.
> > 
> >>> Fixes: 4036e387 ("KVM: s390: ioctls to get and set guest storage attributes")
> >>> Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
> >> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> >>
> >>
> >> Out of curiosity, how did you notice that? 
> > 
> > I regenerated strace's ioctl lists.  It was obvious from the diff that
> > *GET* and *SET* could not be both _IOC_WRITE.
> > 
> 
> In fact we do have multiple GET/SET ioctls in KVM, where both provide a control
> block that is _IOC_WRITE only. That control block then has an address that will 
> be read/written to depending on get/set. 
> E.g. look at 
> #define KVM_SET_DEVICE_ATTR       _IOW(KVMIO,  0xe1, struct kvm_device_attr)
> #define KVM_GET_DEVICE_ATTR       _IOW(KVMIO,  0xe2, struct kvm_device_attr)
> 
> but as far as I understand, the direction hints only qualify the referenced
> struct and not the side effects. So for KVM_*_DEVICE_ATTR it is correct to have
> IOW for both cases.
> 
> But for GET_CMMA we do indeed write back data. 
> 
> Paolo, Radim,
> 
> if we want to fix the direction hint, it would be good to merge this in as soon
> as possible. The new interface was added during this merge window.

Having correct hints would allow us to have one common
copy_from_user/copy_to_user and I think it's not too late to rename it
with the real behavior.  Applied for the second merge-window pull
request,

thanks.