Re: [PATCH 3/5] 9pfs: fix wrong encoding of rdev field in Rgetattr on macOS

[Christian Schoenebeck]

On Donnerstag, 21. April 2022 09:30:56 CEST Greg Kurz wrote:
> On Tue, 19 Apr 2022 13:41:15 +0200
> 
> Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:
> > The 'rdev' field in 9p reponse 'Rgetattr' is of type dev_t,
> > which is actually a system dependant type and therefore both the
> > size and encoding of dev_t differ between macOS and Linux.
> > 
> > So far we have sent 'rdev' to guest in host's dev_t format as-is,
> > which caused devices to appear with wrong device numbers on
> > guests running on macOS hosts, eventually leading to various
> > misbehaviours on guest in conjunction with device files.
> > 
> > This patch fixes this issue by converting the device number from
> > host's dev_t format to Linux dev_t format. As 9p request
> > 'Tgettattr' is exclusive to protocol version 9p2000.L, it should
> > be fair to assume that 'rdev' field is assumed to be in Linux dev_t
> > format by client as well.
> 
> For the sake of accuracy : this is converting the host's dev_t to glibc's
> format (MMMM_Mmmm_mmmM_MMmm, 64 bits) actually, which is compatible with
> linux's format (mmmM_MMmm, 32 bits), as described in <bits/sysmacros.h>.

You want me to put this to the commit log?

> > Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> > ---
> > 
> >  hw/9pfs/9p.c | 37 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> > index 225f31fc31..d953035e1c 100644
> > --- a/hw/9pfs/9p.c
> > +++ b/hw/9pfs/9p.c
> > @@ -1318,6 +1318,41 @@ static int32_t stat_to_iounit(const V9fsPDU *pdu,
> > const struct stat *stbuf)> 
> >      return blksize_to_iounit(pdu, stbuf->st_blksize);
> >  
> >  }
> > 
> > +#if !defined(CONFIG_LINUX)
> > +
> > +/*
> > + * Generates a Linux device number (a.k.a. dev_t) for given device major
> > + * and minor numbers.
> > + */
> > +static uint64_t makedev_dotl(uint32_t dev_major, uint32_t dev_minor)
> > +{
> > +    uint64_t dev;
> > +
> > +    // from glibc sysmacros.h:
> > +    dev  = (((uint64_t) (dev_major & 0x00000fffu)) <<  8);
> > +    dev |= (((uint64_t) (dev_major & 0xfffff000u)) << 32);
> > +    dev |= (((uint64_t) (dev_minor & 0x000000ffu)) <<  0);
> > +    dev |= (((uint64_t) (dev_minor & 0xffffff00u)) << 12);
> > +    return dev;
> > +}
> > +
> > +#endif
> > +
> > +/*
> > + * Converts given device number from host's device number format to Linux
> > + * device number format. As both the size of type dev_t and encoding of
> > + * dev_t is system dependant, we have to convert them for Linux guests if
> > + * host is not running Linux.
> > + */
> > +static uint64_t host_dev_to_dotl_dev(dev_t dev)
> > +{
> > +#ifdef CONFIG_LINUX
> > +    return dev;
> > +#else
> > +    return makedev_dotl(major(dev), minor(dev));
> > +#endif
> > +}
> > +
> 
> It is a bit unfortunate to inflate 9p.c, which is large enough, with
> glue code. Even if they are only used here, I'd personally put them
> in 9p-util.h. No big deal.

Makes sense, I'll move it.

> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks!

> >  static int stat_to_v9stat_dotl(V9fsPDU *pdu, const struct stat *stbuf,
> >  
> >                                  V9fsStatDotl *v9lstat)
> >  
> >  {
> > 
> > @@ -1327,7 +1362,7 @@ static int stat_to_v9stat_dotl(V9fsPDU *pdu, const
> > struct stat *stbuf,> 
> >      v9lstat->st_nlink = stbuf->st_nlink;
> >      v9lstat->st_uid = stbuf->st_uid;
> >      v9lstat->st_gid = stbuf->st_gid;
> > 
> > -    v9lstat->st_rdev = stbuf->st_rdev;
> > +    v9lstat->st_rdev = host_dev_to_dotl_dev(stbuf->st_rdev);
> > 
> >      v9lstat->st_size = stbuf->st_size;
> >      v9lstat->st_blksize = stat_to_iounit(pdu, stbuf);
> >      v9lstat->st_blocks = stbuf->st_blocks;