Re: [Qemu-devel] [PATCH v4 2/5] 9p: Treat multiple devices on one export as an error

[Greg Kurz]

On Fri, 28 Jun 2019 14:36:41 +0200
Christian Schoenebeck <address@hidden> wrote:

> On Donnerstag, 27. Juni 2019 19:26:22 CEST Greg Kurz wrote:
> > On Wed, 26 Jun 2019 20:30:41 +0200
> > 
> > Christian Schoenebeck via Qemu-devel <address@hidden> wrote:
> > > The QID path should uniquely identify a file. However, the
> > > inode of a file is currently used as the QID path, which
> > > on its own only uniquely identifies wiles within a device.
> > 
> > s/wile/files
> 
> Ah right. :)
> 
> > > Here we track the device hosting the 9pfs share, in order
> > > to prevent security issues with QID path collisions from
> > > other devices.
> > > 
> > > Signed-off-by: Antonios Motakis <address@hidden>
> > 
> > You should mention here the changes you made to the original patch.
> 
> Got it. Will do for the other cases as well of course.
> 

Cool.

> > > -static void stat_to_qid(const struct stat *stbuf, V9fsQID *qidp)
> > > +static int stat_to_qid(V9fsPDU *pdu, const struct stat *stbuf, V9fsQID
> > > *qidp)> 
> > >  {
> > >  
> > >      size_t size;
> > > 
> > > +    if (pdu->s->dev_id == 0) {
> > > +        pdu->s->dev_id = stbuf->st_dev;
> > 
> > st_dev should be captured in v9fs_device_realize_common() since we
> > lstat() the root there, instead of every request doing the check.
> 
> Ok.
> 
> > > +    } else if (pdu->s->dev_id != stbuf->st_dev) {
> > > +        error_report_once(
> > > +            "9p: Multiple devices detected in same VirtFS export. "
> > > +            "You must use a separate export for each device."
> > > +        );
> > > +        return -ENOSYS;
> > 
> > This error is likely to end up as the return value of a
> > syscall in the guest and -ENOSYS usually means the syscall
> > isn't implemented, which is obviously not the case. Maybe
> > return -EPERM instead ?
> 
> I would rather suggest -ENODEV. The entire device of the requested file/dir 
> is 
> not available on guest.
> 
> -EPERM IMO rather motivates users looking for file system permission settings 
> on individual files intead, and probably not checking the host's logs for the 
> detailled error message.
> 

-ENODEV is ok with me then.

> > > @@ -3633,6 +3674,8 @@ int v9fs_device_realize_common(V9fsState *s, const
> > > V9fsTransport *t,> 
> > >          goto out;
> > >      
> > >      }
> > > 
> > > +    s->dev_id = 0;
> > > +
> > 
> > Set it to stat->st_dev after lstat() was called later in this function.
> 
> I guesst you mean "earlier" not "later". The lstat() call is just before that 
> dev_id initalization line. But in general your suggestion makes sense of 
> course.
> 

Oops... "earlier" indeed :)

> Best regards,
> Christian Schoenebeck