[PATCH v0] kvm: unsigned datatype in ioctl wrapper

[PATCH v0] kvm: unsigned datatype in ioctl wrapper

[johannst]

Dear all,

in my opinion the `type` argument in the kvm ioctl wrappers should be of
type unsigned. Please correct me if I am wrong.

-

Due to the same reason as explained in the comment on the
`irq_set_ioctl` field in `struct KVMState` (accel/kvm/kvm-all.c),
the kvm ioctl wrapper should take `type` as an unsigned.

Signed-off-by: johannst <johannes.stoelp@gmail.com>
---
 accel/kvm/kvm-all.c    | 8 ++++----
 accel/kvm/trace-events | 8 ++++----
 include/sysemu/kvm.h   | 8 ++++----
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 0125c17edb..45cd6edce3 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2967,7 +2967,7 @@ int kvm_cpu_exec(CPUState *cpu)
     return ret;
 }
 
-int kvm_ioctl(KVMState *s, int type, ...)
+int kvm_ioctl(KVMState *s, unsigned type, ...)
 {
     int ret;
     void *arg;
@@ -2985,7 +2985,7 @@ int kvm_ioctl(KVMState *s, int type, ...)
     return ret;
 }
 
-int kvm_vm_ioctl(KVMState *s, int type, ...)
+int kvm_vm_ioctl(KVMState *s, unsigned type, ...)
 {
     int ret;
     void *arg;
@@ -3003,7 +3003,7 @@ int kvm_vm_ioctl(KVMState *s, int type, ...)
     return ret;
 }
 
-int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
+int kvm_vcpu_ioctl(CPUState *cpu, unsigned type, ...)
 {
     int ret;
     void *arg;
@@ -3021,7 +3021,7 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...)
     return ret;
 }
 
-int kvm_device_ioctl(int fd, int type, ...)
+int kvm_device_ioctl(int fd, unsigned type, ...)
 {
     int ret;
     void *arg;
diff --git a/accel/kvm/trace-events b/accel/kvm/trace-events
index 399aaeb0ec..d78c2eff31 100644
--- a/accel/kvm/trace-events
+++ b/accel/kvm/trace-events
@@ -1,11 +1,11 @@
 # See docs/devel/tracing.rst for syntax documentation.
 
 # kvm-all.c
-kvm_ioctl(int type, void *arg) "type 0x%x, arg %p"
-kvm_vm_ioctl(int type, void *arg) "type 0x%x, arg %p"
-kvm_vcpu_ioctl(int cpu_index, int type, void *arg) "cpu_index %d, type 0x%x, arg %p"
+kvm_ioctl(unsigned type, void *arg) "type 0x%x, arg %p"
+kvm_vm_ioctl(unsigned type, void *arg) "type 0x%x, arg %p"
+kvm_vcpu_ioctl(int cpu_index, unsigned type, void *arg) "cpu_index %d, type 0x%x, arg %p"
 kvm_run_exit(int cpu_index, uint32_t reason) "cpu_index %d, reason %d"
-kvm_device_ioctl(int fd, int type, void *arg) "dev fd %d, type 0x%x, arg %p"
+kvm_device_ioctl(int fd, unsigned type, void *arg) "dev fd %d, type 0x%x, arg %p"
 kvm_failed_reg_get(uint64_t id, const char *msg) "Warning: Unable to retrieve ONEREG %" PRIu64 " from KVM: %s"
 kvm_failed_reg_set(uint64_t id, const char *msg) "Warning: Unable to set ONEREG %" PRIu64 " to KVM: %s"
 kvm_init_vcpu(int cpu_index, unsigned long arch_cpu_id) "index: %d id: %lu"
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index a1ab1ee12d..6d6ed4acf9 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -251,11 +251,11 @@ int kvm_on_sigbus(int code, void *addr);
 
 /* internal API */
 
-int kvm_ioctl(KVMState *s, int type, ...);
+int kvm_ioctl(KVMState *s, unsigned type, ...);
 
-int kvm_vm_ioctl(KVMState *s, int type, ...);
+int kvm_vm_ioctl(KVMState *s, unsigned type, ...);
 
-int kvm_vcpu_ioctl(CPUState *cpu, int type, ...);
+int kvm_vcpu_ioctl(CPUState *cpu, unsigned type, ...);
 
 /**
  * kvm_device_ioctl - call an ioctl on a kvm device
@@ -264,7 +264,7 @@ int kvm_vcpu_ioctl(CPUState *cpu, int type, ...);
  *
  * Returns: -errno on error, nonnegative on success
  */
-int kvm_device_ioctl(int fd, int type, ...);
+int kvm_device_ioctl(int fd, unsigned type, ...);
 
 /**
  * kvm_vm_check_attr - check for existence of a specific vm attribute
-- 
2.32.0

[PATCH v0] kvm: unsigned datatype in ioctl wrapper

[Johannes Stoelp]

From: johannst <johannes.stoelp@gmail.com>

Ping.

https://patchew.org/QEMU/20210805193950.514357-1-johannes.stoelp@gmail.com/
https://lore.kernel.org/qemu-devel/20210805193950.514357-1-johannes.stoelp@gmail.com/

Thanks and best,
Johannes

[PATCH v0] kvm: unsigned datatype in ioctl wrapper

[Johannes Stoelp]

From: johannst <johannes.stoelp@gmail.com>

Ping.

https://patchew.org/QEMU/20210805193950.514357-1-johannes.stoelp@gmail.com/
https://lore.kernel.org/qemu-devel/20210805193950.514357-1-johannes.stoelp@gmail.com/

Thanks and best,
Johannes

Re: [PATCH v0] kvm: unsigned datatype in ioctl wrapper

[Peter Maydell]

On Thu, 5 Aug 2021 at 21:34, johannst <johannes.stoelp@googlemail.com> wrote:
>
> Dear all,
>
> in my opinion the `type` argument in the kvm ioctl wrappers should be of
> type unsigned. Please correct me if I am wrong.

(Ccing Eric as our resident POSIX expert.)

> Due to the same reason as explained in the comment on the
> `irq_set_ioctl` field in `struct KVMState` (accel/kvm/kvm-all.c),
> the kvm ioctl wrapper should take `type` as an unsigned.

The reason in that comment:
    /* The man page (and posix) say ioctl numbers are signed int, but
     * they're not.  Linux, glibc and *BSD all treat ioctl numbers as
     * unsigned, and treating them as signed here can break things */

It would be more helpful to readers to state the reason directly
in the commit message, rather than requiring them to go and look
up a comment in some other file.

(That comment, incidentally, seems to be no longer completely
true: on my system the ioctl manpage says 'unsigned long', though
the glibc info docs say 'int', in contradiction to the ioctl.h
glibc actually ships...)

Of the various KVM_* ioctls we use via these functions, do
any actually have values that would result in invalid sign
extension here ? That is, is this fixing an existing bug, or is
it merely avoiding a potential future bug?

> Signed-off-by: johannst <johannes.stoelp@gmail.com>
> ---
>  accel/kvm/kvm-all.c    | 8 ++++----
>  accel/kvm/trace-events | 8 ++++----
>  include/sysemu/kvm.h   | 8 ++++----
>  3 files changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 0125c17edb..45cd6edce3 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2967,7 +2967,7 @@ int kvm_cpu_exec(CPUState *cpu)
>      return ret;
>  }
>
> -int kvm_ioctl(KVMState *s, int type, ...)
> +int kvm_ioctl(KVMState *s, unsigned type, ...)

The underlying ioctl() prototype (at least in my Linux /usr/include/sys/ioctl.h
and as documented in the ioctl(2) manpage) uses "unsigned long" for the
request argument; should we do the same ?

-- PMM

Re: [PATCH v0] kvm: unsigned datatype in ioctl wrapper

[Eric Blake]

On Sun, Aug 29, 2021 at 10:09:19PM +0100, Peter Maydell wrote:
> On Thu, 5 Aug 2021 at 21:34, johannst <johannes.stoelp@googlemail.com> wrote:
> >
> > Dear all,
> >
> > in my opinion the `type` argument in the kvm ioctl wrappers should be of
> > type unsigned. Please correct me if I am wrong.
> 
> (Ccing Eric as our resident POSIX expert.)
> 
> > Due to the same reason as explained in the comment on the
> > `irq_set_ioctl` field in `struct KVMState` (accel/kvm/kvm-all.c),
> > the kvm ioctl wrapper should take `type` as an unsigned.
> 
> The reason in that comment:
>     /* The man page (and posix) say ioctl numbers are signed int, but
>      * they're not.  Linux, glibc and *BSD all treat ioctl numbers as
>      * unsigned, and treating them as signed here can break things */
> 
> It would be more helpful to readers to state the reason directly
> in the commit message, rather than requiring them to go and look
> up a comment in some other file.
> 
> (That comment, incidentally, seems to be no longer completely
> true: on my system the ioctl manpage says 'unsigned long', though
> the glibc info docs say 'int', in contradiction to the ioctl.h
> glibc actually ships...)

POSIX says of ioctl:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/ioctl.html
 int ioctl(int fildes, int request, ... /* arg */);

But the standardization of ioctl() is extremely limited: POSIX only
uses it for the now-deprecated STREAMS option (basically it was
supposed to be the next-generation pty interface, but it never really
caught on; Solaris supported it, but I don't think Linux ever has).
And qemu doesn't really care about the STREAMS option; so our real
source of authority for how ioctl behaves is not POSIX, but the
kernel.

The fact that glibc uses unsigned long rather than int for the second
argument is a strong argument in favor of using an unsigned type (on
64-bit platforms, the kernel really is looking at 64 bits, even though
POSIX says we are only passing in 32, and sign-extension is wrong),
but on the other hand, I don't know if any ioctl requests CAN be sign
extended (ideally, no ioctl request has bit 0x80000000 set, so that it
doesn't matter if the userspace code was calling via a signed or
unsigned type, or via the 32-bit POSIX signature instead of the actual
kernel 'unsigned long' signature).

> 
> Of the various KVM_* ioctls we use via these functions, do
> any actually have values that would result in invalid sign
> extension here ? That is, is this fixing an existing bug, or is
> it merely avoiding a potential future bug?

My question as well.  If there is such a bug, calling it out in the
commit message is essential; if the bug is just theoretical,
mentioning that is still useful.

> 
> > Signed-off-by: johannst <johannes.stoelp@gmail.com>
> > ---
> >  accel/kvm/kvm-all.c    | 8 ++++----
> >  accel/kvm/trace-events | 8 ++++----
> >  include/sysemu/kvm.h   | 8 ++++----
> >  3 files changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index 0125c17edb..45cd6edce3 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -2967,7 +2967,7 @@ int kvm_cpu_exec(CPUState *cpu)
> >      return ret;
> >  }
> >
> > -int kvm_ioctl(KVMState *s, int type, ...)
> > +int kvm_ioctl(KVMState *s, unsigned type, ...)
> 
> The underlying ioctl() prototype (at least in my Linux /usr/include/sys/ioctl.h
> and as documented in the ioctl(2) manpage) uses "unsigned long" for the
> request argument; should we do the same ?

Either we should match POSIX ('int') or Linux ('unsigned long'),
'unsigned' matches neither.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH v0] kvm: unsigned datatype in ioctl wrapper

[Peter Maydell]

On Mon, 30 Aug 2021 at 16:47, Eric Blake <eblake@redhat.com> wrote:
>
> On Sun, Aug 29, 2021 at 10:09:19PM +0100, Peter Maydell wrote:
> > Of the various KVM_* ioctls we use via these functions, do
> > any actually have values that would result in invalid sign
> > extension here ? That is, is this fixing an existing bug, or is
> > it merely avoiding a potential future bug?
>
> My question as well.  If there is such a bug, calling it out in the
> commit message is essential; if the bug is just theoretical,
> mentioning that is still useful.

I found this glibc bug from 2012, filed by some random guy named
Linus Torvalds, and still open:
https://sourceware.org/bugzilla/show_bug.cgi?id=14362
where among other things he claims
# As noted, this does not actually cause problems on Linux, because
# unlike FreeBSD, Linux knows what the f*ck it is doing, and just
# ignores the upper bits exactly because of possible sign confusion.

Whether that's still true a decade later I have no idea :-)

-- PMM

Re: [PATCH v0] kvm: unsigned datatype in ioctl wrapper

[Ed Maste]

On Mon, 30 Aug 2021 at 13:34, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> # As noted, this does not actually cause problems on Linux, because
> # unlike FreeBSD, Linux knows what the f*ck it is doing, and just
> # ignores the upper bits exactly because of possible sign confusion.
>
> Whether that's still true a decade later I have no idea :-)

It wasn't even true a decade ago -- FreeBSD has ignored the upper bits
since 2005[1]. That change included a warning if upper bits were set,
but the warning is now hidden behind a diagnostic option.

[1] URL: https://cgit.FreeBSD.org/src/commit/?id=9fc6aa0618a174f436fb1a26867c99cff4125b40

Re: [PATCH v0] kvm: unsigned datatype in ioctl wrapper

[Johannes S]

On Sun, Aug 29, 2021 at 11:10 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> It would be more helpful to readers to state the reason directly
> in the commit message, rather than requiring them to go and look
> up a comment in some other file.

Thanks for the tip, that makes sense. I will follow it next time.

> > Of the various KVM_* ioctls we use via these functions, do
> > any actually have values that would result in invalid sign
> > extension here ? That is, is this fixing an existing bug, or is
> > it merely avoiding a potential future bug?
>
> My question as well.  If there is such a bug, calling it out in the
> commit message is essential; if the bug is just theoretical,
> mentioning that is still useful.

I agree, more details would have been helpful here.

The background is, that I observed some sign-extension for certain kvm
ioctl requests. However that was in an artificial environment which
included an ioctl wrapper that was defined as
    int ioctl(int fd, unsigned long type, ...);

One example for such an ioctl request is the 'KVM_IRQ_LINE_STATUS'.

So this is not fixing a actual bug, but a theoretical one.

On Mon, Aug 30, 2021 at 5:47 PM Eric Blake <eblake@redhat.com> wrote:
> The fact that glibc uses unsigned long rather than int for the second
> argument is a strong argument in favor of using an unsigned type (on
> 64-bit platforms, the kernel really is looking at 64 bits, even though
> POSIX says we are only passing in 32, and sign-extension is wrong),
> but on the other hand, I don't know if any ioctl requests CAN be sign
> extended (ideally, no ioctl request has bit 0x80000000 set, so that it
> doesn't matter if the userspace code was calling via a signed or
> unsigned type, or via the 32-bit POSIX signature instead of the actual
> kernel 'unsigned long' signature).

On my machine (64bit) I found some ioctl requests with the 0x8000_0000 bit
set, eg
- KVM_IRQ_LINE_STATUS
- KVM_GET_MSR_INDEX_LIST
- KVM_SET_IRQCHIP

I was curious and looked into the macro definitions and it turns out on my
machine, all requests with the '_IOC_READ' direction seem to have the
0x8000_0000 bit set.
_IOC_READ = 2
_IOC_DIRSHIFT = 30
_IOC_READ << _IOC_DIRSHIFT = 0x80000000

#define KVM_SET_IRQCHIP     _IOR(KVMIO,  0x63, struct kvm_irqchip)
#define _IOR(type,nr,size)  _IOC(_IOC_READ,(type),(nr),(_IOC_TYPECHECK(size)))
#define _IOC(dir,type,nr,size) \
    (((dir)  << _IOC_DIRSHIFT) | \
     ((type) << _IOC_TYPESHIFT) | \
     ((nr)   << _IOC_NRSHIFT) | \
     ((size) << _IOC_SIZESHIFT))

On Mon, Aug 30, 2021 at 7:33 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> I found this glibc bug from 2012, filed by some random guy named
> Linus Torvalds, and still open:
> https://sourceware.org/bugzilla/show_bug.cgi?id=14362
> where among other things he claims
> # As noted, this does not actually cause problems on Linux, because
> # unlike FreeBSD, Linux knows what the f*ck it is doing, and just
> # ignores the upper bits exactly because of possible sign confusion.

Sounds like we are saved be the Linux Kernel here :)

In my opinion we should use 'unsigned' data types here for the ioctl
request in the ioctl wrappers or would you prefer to keep the ioctl
wrapper definition as is today? What is you opinion?

Thanks and best,
Johannes

Re: [PATCH v0] kvm: unsigned datatype in ioctl wrapper

[Peter Maydell]

On Mon, 30 Aug 2021 at 20:37, Johannes S <johannes.stoelp@googlemail.com> wrote:
> In my opinion we should use 'unsigned' data types here for the ioctl
> request in the ioctl wrappers or would you prefer to keep the ioctl
> wrapper definition as is today? What is you opinion?

I think I would vote for following the type used by the ioctl()
function as declared in the headers for both Linux and the BSDs,
and using 'unsigned long'.
(We should change KVMState::irq_set_ioctl too, to match.)

-- PMM

Re: [PATCH v0] kvm: unsigned datatype in ioctl wrapper

[Johannes S]

On Mon, Aug 30, 2021 at 10:15 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> I think I would vote for following the type used by the ioctl()
> function as declared in the headers for both Linux and the BSDs,
> and using 'unsigned long'.
> (We should change KVMState::irq_set_ioctl too, to match.)

I would agree to 'unsigned long', in that case I can generate a new patch.

And in theory if all libc implementations specify the ioctl request as
'int' we could go back to 'int'.

Johannes