Re: [Qemu-devel] [PATCH v2] linux-user: fix to handle variably sized SIOCGSTAMP with new kernels

[Daniel P . Berrangé]

On Thu, Jul 11, 2019 at 10:02:01AM +0200, Christian Ehrhardt wrote:
> On Mon, Jun 17, 2019 at 5:35 PM Laurent Vivier <address@hidden> wrote:
> >
> > Le 17/06/2019 à 15:11, Daniel P. Berrangé a écrit :
> > > The SIOCGSTAMP symbol was previously defined in the
> > > asm-generic/sockios.h header file. QEMU sees that header
> > > indirectly via sys/socket.h
> > >
> > > In linux kernel commit 0768e17073dc527ccd18ed5f96ce85f9985e9115
> > > the asm-generic/sockios.h header no longer defines SIOCGSTAMP.
> > > Instead it provides only SIOCGSTAMP_OLD, which only uses a
> > > 32-bit time_t on 32-bit architectures.
> > >
> > > The linux/sockios.h header then defines SIOCGSTAMP using
> > > either SIOCGSTAMP_OLD or SIOCGSTAMP_NEW as appropriate. If
> > > SIOCGSTAMP_NEW is used, then the tv_sec field is 64-bit even
> > > on 32-bit architectures
> > >
> > > To cope with this we must now define two separate syscalls,
> > > with corresponding old and new sizes, as well as including
> > > the new linux/sockios.h header.
> > >
> > > Signed-off-by: Daniel P. Berrangé <address@hidden>
> > > ---
> > >  linux-user/ioctls.h        | 15 +++++++++++++++
> > >  linux-user/syscall.c       |  1 +
> > >  linux-user/syscall_defs.h  |  5 +++++
> > >  linux-user/syscall_types.h |  4 ++++
> > >  4 files changed, 25 insertions(+)
> > >
> > > diff --git a/linux-user/ioctls.h b/linux-user/ioctls.h
> > > index 5e84dc7c3a..5a6d6def7e 100644
> > > --- a/linux-user/ioctls.h
> > > +++ b/linux-user/ioctls.h
> > > @@ -222,8 +222,23 @@
> > >    IOCTL(SIOCGIWNAME, IOC_W | IOC_R, MK_PTR(MK_STRUCT(STRUCT_char_ifreq)))
> > >    IOCTL(SIOCSPGRP, IOC_W, MK_PTR(TYPE_INT)) /* pid_t */
> > >    IOCTL(SIOCGPGRP, IOC_R, MK_PTR(TYPE_INT)) /* pid_t */
> > > +
> > > +#ifdef SIOCGSTAMP_OLD
> > > +  IOCTL(SIOCGSTAMP_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval)))
> > > +#else
> > >    IOCTL(SIOCGSTAMP, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval)))
> > > +#endif
> > > +#ifdef SIOCGSTAMPNS_OLD
> > > +  IOCTL(SIOCGSTAMPNS_OLD, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec)))
> > > +#else
> > >    IOCTL(SIOCGSTAMPNS, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec)))
> > > +#endif
> > > +#ifdef SIOCGSTAMP_NEW
> > > +  IOCTL(SIOCGSTAMP_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timeval64)))
> > > +#endif
> > > +#ifdef SIOCGSTAMPNS_NEW
> > > +  IOCTL(SIOCGSTAMPNS_NEW, IOC_R, MK_PTR(MK_STRUCT(STRUCT_timespec64)))
> > > +#endif
> > >
> > >    IOCTL(RNDGETENTCNT, IOC_R, MK_PTR(TYPE_INT))
> > >    IOCTL(RNDADDTOENTCNT, IOC_W, MK_PTR(TYPE_INT))
> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > > index b187c1281d..f13e260b02 100644
> > > --- a/linux-user/syscall.c
> > > +++ b/linux-user/syscall.c
> > > @@ -37,6 +37,7 @@
> > >  #include <sched.h>
> > >  #include <sys/timex.h>
> > >  #include <sys/socket.h>
> > > +#include <linux/sockios.h>
> > >  #include <sys/un.h>
> > >  #include <sys/uio.h>
> > >  #include <poll.h>
> > > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> > > index 7f141f699c..7830b600e7 100644
> > > --- a/linux-user/syscall_defs.h
> > > +++ b/linux-user/syscall_defs.h
> > > @@ -750,6 +750,11 @@ struct target_pollfd {
> > >
> > >  #define TARGET_SIOCGSTAMP      0x8906          /* Get stamp (timeval) */
> > >  #define TARGET_SIOCGSTAMPNS    0x8907          /* Get stamp (timespec) */
> > > +#define TARGET_SIOCGSTAMP_OLD   0x8906          /* Get stamp (timeval) */
> > > +#define TARGET_SIOCGSTAMPNS_OLD 0x8907          /* Get stamp (timespec) 
> > > */
> > > +#define TARGET_SIOCGSTAMP_NEW   TARGET_IOC(TARGET_IOC_READ, 's', 6, 
> > > sizeof(long long) + sizeof(long)) /* Get stamp (timeval64) */
> > > +#define TARGET_SIOCGSTAMPNS_NEW TARGET_IOC(TARGET_IOC_READ, 's', 7, 
> > > sizeof(long long) + sizeof(long)) /* Get stamp (timespec64) */
> > kernel defines:
> >
> > #define SIOCGSTAMP_NEW   _IOR(SOCK_IOC_TYPE, 0x06, long long[2])
> > #define SIOCGSTAMPNS_NEW _IOR(SOCK_IOC_TYPE, 0x07, long long[2])
> >
> > So it should be TARGET_IOR(0x89, 0x6, abi_llong[2])
> >
> > Their codes are 0x80108906 and 80108907.
> 
> Hi,
> I found the discussion around this topic being almost a month old.
> And related to this fedora bug [1] was closed by adding [2] which
> matches [3] that was nacked in the discussion here.
> 
> Since I found nothing later (neither qemu commits nor further
> discussions) I wonder if it has fallen through the cracks OR if there
> was a kernel fix/change to resolve it (if that is the case a pointer
> to the related kernel change would be nice)?

I didn't have time to address the feedback to this v2, and since the
immediate problem for Fedora has a workaround, its been lower priority
especially since my understanding of linux-iser is limited.

IOW, If anyone wants to take over my patch proposal here feel free. It
would obviously be nice to fix for this 4.1 release if practical.

> 
> [1]: https://bugzilla.redhat.com/show_bug.cgi?id=1718926
> [2]: 
> https://src.fedoraproject.org/rpms/qemu/blob/master/f/0005-NOT-UPSTREAM-Build-fix-with-latest-kernel.patch
> [3]: https://patchew.org/QEMU/address@hidden/
> 
> > Thanks,
> > Laurent
> >
> 
> 
> -- 
> Christian Ehrhardt
> Software Engineer, Ubuntu Server
> Canonical Ltd
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|