On Sun, Aug 29, 2021 at 10:09:19PM +0100, Peter Maydell wrote: > On Thu, 5 Aug 2021 at 21:34, johannst <johannes.sto...@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.sto...@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
