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

[Christian Schoenebeck via Qemu-devel <qemu-devel@nongnu.org>]

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 <qemu-devel@nongnu.org> 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 <antonios.motakis@huawei.com>
> 
> You should mention here the changes you made to the original patch.

Got it. Will do for the other cases as well of course.

> > -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.

> > @@ -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.

Best regards,
Christian Schoenebeck