[PATCH] vduse: add virtio_fs to allowed dev id

[PATCH] vduse: add virtio_fs to allowed dev id

[Eugenio Pérez]

A VDUSE device that implements virtiofs device works fine just by
adding the device id to the whitelist.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 7ae99691efdf..6a9a37351310 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -144,6 +144,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
 static u32 allowed_device_id[] = {
 	VIRTIO_ID_BLOCK,
 	VIRTIO_ID_NET,
+	VIRTIO_ID_FS,
 };

 static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
--
2.48.1

Re: [PATCH] vduse: add virtio_fs to allowed dev id

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 418 bytes --]

On Tue, Jan 21, 2025 at 11:33:46AM +0100, Eugenio Pérez wrote:
> A VDUSE device that implements virtiofs device works fine just by
> adding the device id to the whitelist.

The virtiofs DAX Window feature might require extra work. It is not
widely enabled though, so must virtiofs devices will work fine with just
virtqueue and configuration space support.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] vduse: add virtio_fs to allowed dev id

[Jason Wang]

On Wed, Jan 22, 2025 at 11:49 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Tue, Jan 21, 2025 at 11:33:46AM +0100, Eugenio Pérez wrote:
> > A VDUSE device that implements virtiofs device works fine just by
> > adding the device id to the whitelist.
>
> The virtiofs DAX Window feature might require extra work. It is not
> widely enabled though, so must virtiofs devices will work fine with just
> virtqueue and configuration space support.

+1.

The DAX Window may help to reduce the bounce buffer copying and fit
for the case where an AI application just accesses one single large
file.

We need to consider implementing this via VDUSE.

Thanks


>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [PATCH] vduse: add virtio_fs to allowed dev id

[Eugenio Perez Martin]

On Thu, Jan 23, 2025 at 2:50 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Jan 22, 2025 at 11:49 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Tue, Jan 21, 2025 at 11:33:46AM +0100, Eugenio Pérez wrote:
> > > A VDUSE device that implements virtiofs device works fine just by
> > > adding the device id to the whitelist.
> >
> > The virtiofs DAX Window feature might require extra work. It is not
> > widely enabled though, so must virtiofs devices will work fine with just
> > virtqueue and configuration space support.
>
> +1.
>
> The DAX Window may help to reduce the bounce buffer copying and fit
> for the case where an AI application just accesses one single large
> file.
>
> We need to consider implementing this via VDUSE.
>
> Thanks
>
>

I thought DAX can only be applied in the VM case. Is it possible to
apply it using virtio_vdpa?

Is it possible to detect that the device is trying to use DAX so it
fails the negotiation?

Re: [PATCH] vduse: add virtio_fs to allowed dev id

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1660 bytes --]

On Thu, Jan 23, 2025 at 08:26:57AM +0100, Eugenio Perez Martin wrote:
> On Thu, Jan 23, 2025 at 2:50 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Jan 22, 2025 at 11:49 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > On Tue, Jan 21, 2025 at 11:33:46AM +0100, Eugenio Pérez wrote:
> > > > A VDUSE device that implements virtiofs device works fine just by
> > > > adding the device id to the whitelist.
> > >
> > > The virtiofs DAX Window feature might require extra work. It is not
> > > widely enabled though, so must virtiofs devices will work fine with just
> > > virtqueue and configuration space support.
> >
> > +1.
> >
> > The DAX Window may help to reduce the bounce buffer copying and fit
> > for the case where an AI application just accesses one single large
> > file.
> >
> > We need to consider implementing this via VDUSE.
> >
> > Thanks
> >
> >
> 
> I thought DAX can only be applied in the VM case. Is it possible to
> apply it using virtio_vdpa?

Hardware could implement the DAX Window feature. In virtio-pci it's
implemented as a range within a BAR on the PCI device.

> Is it possible to detect that the device is trying to use DAX so it
> fails the negotiation?

Yes, DAX is optional. It's only available when the device reports a
Shared Memory Region with ID 0:
https://github.com/oasis-tcs/virtio-spec/blob/master/device-types/fs/description.tex#L233

Just to let you know, the QEMU vhost-user implementation of the DAX
Window is not complete. In the past David Gilbert had out-of-tree
patches to enable it in QEMU.

The guest driver support is in Linux.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] vduse: add virtio_fs to allowed dev id

[Michael S. Tsirkin]

On Tue, Jan 21, 2025 at 11:33:46AM +0100, Eugenio Pérez wrote:
> A VDUSE device that implements virtiofs device works fine just by
> adding the device id to the whitelist.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>


OK, but the commit log really should say why
you are doing this. And also why is it safe.

> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 7ae99691efdf..6a9a37351310 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -144,6 +144,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
>  static u32 allowed_device_id[] = {
>  	VIRTIO_ID_BLOCK,
>  	VIRTIO_ID_NET,
> +	VIRTIO_ID_FS,
>  };
> 
>  static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
> --
> 2.48.1

Re: [PATCH] vduse: add virtio_fs to allowed dev id

[Eugenio Perez Martin]

On Mon, Feb 24, 2025 at 10:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Jan 21, 2025 at 11:33:46AM +0100, Eugenio Pérez wrote:
> > A VDUSE device that implements virtiofs device works fine just by
> > adding the device id to the whitelist.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
>
> OK, but the commit log really should say why
> you are doing this.

Sure I can expand on the motivation.

Something like "Allowing VDUSE FS type allows to build filesystems
that run in userspace and can be presented transparently to the host
and the guest. After modifying userland's libfuse, this allows to
expose a good amount to already available userland FS through vDPA."

I'd add using the high performance virtio protocol but I still need to
do more tests for this TBH.

> And also why is it safe.
>

Can you expand on the scenarios you think this is insecure? While I
understand it's security sensitive, you already need root to perform
vdpa device operations. Is FS different from net or block?

Thanks!

> > ---
> >  drivers/vdpa/vdpa_user/vduse_dev.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index 7ae99691efdf..6a9a37351310 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -144,6 +144,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
> >  static u32 allowed_device_id[] = {
> >       VIRTIO_ID_BLOCK,
> >       VIRTIO_ID_NET,
> > +     VIRTIO_ID_FS,
> >  };
> >
> >  static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
> > --
> > 2.48.1
>

Re: [PATCH] vduse: add virtio_fs to allowed dev id

[Michael S. Tsirkin]

On Tue, Feb 25, 2025 at 01:17:02PM +0100, Eugenio Perez Martin wrote:
> On Mon, Feb 24, 2025 at 10:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Jan 21, 2025 at 11:33:46AM +0100, Eugenio Pérez wrote:
> > > A VDUSE device that implements virtiofs device works fine just by
> > > adding the device id to the whitelist.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >
> >
> > OK, but the commit log really should say why
> > you are doing this.
> 
> Sure I can expand on the motivation.
> 
> Something like "Allowing VDUSE FS type allows to build filesystems
> that run in userspace and can be presented transparently to the host
> and the guest. After modifying userland's libfuse, this allows to
> expose a good amount to already available userland FS through vDPA."
> 
> I'd add using the high performance virtio protocol but I still need to
> do more tests for this TBH.
> 
> > And also why is it safe.
> >
> 
> Can you expand on the scenarios you think this is insecure? While I
> understand it's security sensitive, you already need root to perform
> vdpa device operations. Is FS different from net or block?
> 
> Thanks!

I did not say it was insecure, just that you need to explain the
security considerations in the commit log.
The issue is that when one gave access to vdpa user device previously
it would only allow mounting blk now a filesystem.

Net is different, it is gated by CAP_NET_ADMIN.

When net was introduced, selinux was there initially then it
was deferred and never surfaced.
Maybe we should revive it so it is possible to control which
devices can be created in a granular way.

> > > ---
> > >  drivers/vdpa/vdpa_user/vduse_dev.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index 7ae99691efdf..6a9a37351310 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -144,6 +144,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
> > >  static u32 allowed_device_id[] = {
> > >       VIRTIO_ID_BLOCK,
> > >       VIRTIO_ID_NET,
> > > +     VIRTIO_ID_FS,
> > >  };
> > >
> > >  static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
> > > --
> > > 2.48.1
> >

Re: [PATCH] vduse: add virtio_fs to allowed dev id

[Jason Wang]

On Tue, Feb 25, 2025 at 8:31 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Feb 25, 2025 at 01:17:02PM +0100, Eugenio Perez Martin wrote:
> > On Mon, Feb 24, 2025 at 10:51 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Jan 21, 2025 at 11:33:46AM +0100, Eugenio Pérez wrote:
> > > > A VDUSE device that implements virtiofs device works fine just by
> > > > adding the device id to the whitelist.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > >
> > >
> > > OK, but the commit log really should say why
> > > you are doing this.
> >
> > Sure I can expand on the motivation.
> >
> > Something like "Allowing VDUSE FS type allows to build filesystems
> > that run in userspace and can be presented transparently to the host
> > and the guest. After modifying userland's libfuse, this allows to
> > expose a good amount to already available userland FS through vDPA."
> >
> > I'd add using the high performance virtio protocol but I still need to
> > do more tests for this TBH.
> >
> > > And also why is it safe.
> > >
> >
> > Can you expand on the scenarios you think this is insecure? While I
> > understand it's security sensitive, you already need root to perform
> > vdpa device operations. Is FS different from net or block?
> >
> > Thanks!
>
> I did not say it was insecure, just that you need to explain the
> security considerations in the commit log.
> The issue is that when one gave access to vdpa user device previously
> it would only allow mounting blk now a filesystem.
>
> Net is different, it is gated by CAP_NET_ADMIN.
>
> When net was introduced, selinux was there initially then it
> was deferred and never surfaced.
> Maybe we should revive it so it is possible to control which
> devices can be created in a granular way.

Can we simply start from CAP_SYS_ADMIN?

Thanks

>
> > > > ---
> > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > index 7ae99691efdf..6a9a37351310 100644
> > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > @@ -144,6 +144,7 @@ static struct workqueue_struct *vduse_irq_bound_wq;
> > > >  static u32 allowed_device_id[] = {
> > > >       VIRTIO_ID_BLOCK,
> > > >       VIRTIO_ID_NET,
> > > > +     VIRTIO_ID_FS,
> > > >  };
> > > >
> > > >  static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
> > > > --
> > > > 2.48.1
> > >
>