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