| Message ID | 20210119045920.447-4-xieyongji@bytedance.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Introduce VDUSE - vDPA Device in Userspace | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 2021/1/19 下午12:59, Xie Yongji wrote: > With VDUSE, we should be able to support all kinds of virtio devices. > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > --- > drivers/vhost/vdpa.c | 29 +++-------------------------- > 1 file changed, 3 insertions(+), 26 deletions(-) > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > index 29ed4173f04e..448be7875b6d 100644 > --- a/drivers/vhost/vdpa.c > +++ b/drivers/vhost/vdpa.c > @@ -22,6 +22,7 @@ > #include <linux/nospec.h> > #include <linux/vhost.h> > #include <linux/virtio_net.h> > +#include <linux/virtio_blk.h> > > #include "vhost.h" > > @@ -185,26 +186,6 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) > return 0; > } > > -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, > - struct vhost_vdpa_config *c) > -{ > - long size = 0; > - > - switch (v->virtio_id) { > - case VIRTIO_ID_NET: > - size = sizeof(struct virtio_net_config); > - break; > - } > - > - if (c->len == 0) > - return -EINVAL; > - > - if (c->len > size - c->off) > - return -E2BIG; > - > - return 0; > -} I think we should use a separate patch for this. Thanks > - > static long vhost_vdpa_get_config(struct vhost_vdpa *v, > struct vhost_vdpa_config __user *c) > { > @@ -215,7 +196,7 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v, > > if (copy_from_user(&config, c, size)) > return -EFAULT; > - if (vhost_vdpa_config_validate(v, &config)) > + if (config.len == 0) > return -EINVAL; > buf = kvzalloc(config.len, GFP_KERNEL); > if (!buf) > @@ -243,7 +224,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, > > if (copy_from_user(&config, c, size)) > return -EFAULT; > - if (vhost_vdpa_config_validate(v, &config)) > + if (config.len == 0) > return -EINVAL; > buf = kvzalloc(config.len, GFP_KERNEL); > if (!buf) > @@ -1025,10 +1006,6 @@ static int vhost_vdpa_probe(struct vdpa_device *vdpa) > int minor; > int r; > > - /* Currently, we only accept the network devices. */ > - if (ops->get_device_id(vdpa) != VIRTIO_ID_NET) > - return -ENOTSUPP; > - > v = kzalloc(sizeof(*v), GFP_KERNEL | __GFP_RETRY_MAYFAIL); > if (!v) > return -ENOMEM;
On Wed, Jan 20, 2021 at 11:47 AM Jason Wang <jasowang@redhat.com> wrote: > > > On 2021/1/19 下午12:59, Xie Yongji wrote: > > With VDUSE, we should be able to support all kinds of virtio devices. > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > > --- > > drivers/vhost/vdpa.c | 29 +++-------------------------- > > 1 file changed, 3 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > > index 29ed4173f04e..448be7875b6d 100644 > > --- a/drivers/vhost/vdpa.c > > +++ b/drivers/vhost/vdpa.c > > @@ -22,6 +22,7 @@ > > #include <linux/nospec.h> > > #include <linux/vhost.h> > > #include <linux/virtio_net.h> > > +#include <linux/virtio_blk.h> > > > > #include "vhost.h" > > > > @@ -185,26 +186,6 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) > > return 0; > > } > > > > -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, > > - struct vhost_vdpa_config *c) > > -{ > > - long size = 0; > > - > > - switch (v->virtio_id) { > > - case VIRTIO_ID_NET: > > - size = sizeof(struct virtio_net_config); > > - break; > > - } > > - > > - if (c->len == 0) > > - return -EINVAL; > > - > > - if (c->len > size - c->off) > > - return -E2BIG; > > - > > - return 0; > > -} > > > I think we should use a separate patch for this. > Will do it. Thanks, Yongji
On Wed, Jan 20, 2021 at 11:46:38AM +0800, Jason Wang wrote: > >On 2021/1/19 下午12:59, Xie Yongji wrote: >>With VDUSE, we should be able to support all kinds of virtio devices. >> >>Signed-off-by: Xie Yongji <xieyongji@bytedance.com> >>--- >> drivers/vhost/vdpa.c | 29 +++-------------------------- >> 1 file changed, 3 insertions(+), 26 deletions(-) >> >>diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >>index 29ed4173f04e..448be7875b6d 100644 >>--- a/drivers/vhost/vdpa.c >>+++ b/drivers/vhost/vdpa.c >>@@ -22,6 +22,7 @@ >> #include <linux/nospec.h> >> #include <linux/vhost.h> >> #include <linux/virtio_net.h> >>+#include <linux/virtio_blk.h> >> #include "vhost.h" >>@@ -185,26 +186,6 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) >> return 0; >> } >>-static int vhost_vdpa_config_validate(struct vhost_vdpa *v, >>- struct vhost_vdpa_config *c) >>-{ >>- long size = 0; >>- >>- switch (v->virtio_id) { >>- case VIRTIO_ID_NET: >>- size = sizeof(struct virtio_net_config); >>- break; >>- } >>- >>- if (c->len == 0) >>- return -EINVAL; >>- >>- if (c->len > size - c->off) >>- return -E2BIG; >>- >>- return 0; >>-} > > >I think we should use a separate patch for this. For the vdpa-blk simulator I had the same issues and I'm adding a .get_config_size() callback to vdpa devices. Do you think make sense or is better to remove this check in vhost/vdpa, delegating the boundaries checks to get_config/set_config callbacks. Thanks, Stefano
On 2021/1/20 下午7:08, Stefano Garzarella wrote: > On Wed, Jan 20, 2021 at 11:46:38AM +0800, Jason Wang wrote: >> >> On 2021/1/19 下午12:59, Xie Yongji wrote: >>> With VDUSE, we should be able to support all kinds of virtio devices. >>> >>> Signed-off-by: Xie Yongji <xieyongji@bytedance.com> >>> --- >>> drivers/vhost/vdpa.c | 29 +++-------------------------- >>> 1 file changed, 3 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >>> index 29ed4173f04e..448be7875b6d 100644 >>> --- a/drivers/vhost/vdpa.c >>> +++ b/drivers/vhost/vdpa.c >>> @@ -22,6 +22,7 @@ >>> #include <linux/nospec.h> >>> #include <linux/vhost.h> >>> #include <linux/virtio_net.h> >>> +#include <linux/virtio_blk.h> >>> #include "vhost.h" >>> @@ -185,26 +186,6 @@ static long vhost_vdpa_set_status(struct >>> vhost_vdpa *v, u8 __user *statusp) >>> return 0; >>> } >>> -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, >>> - struct vhost_vdpa_config *c) >>> -{ >>> - long size = 0; >>> - >>> - switch (v->virtio_id) { >>> - case VIRTIO_ID_NET: >>> - size = sizeof(struct virtio_net_config); >>> - break; >>> - } >>> - >>> - if (c->len == 0) >>> - return -EINVAL; >>> - >>> - if (c->len > size - c->off) >>> - return -E2BIG; >>> - >>> - return 0; >>> -} >> >> >> I think we should use a separate patch for this. > > For the vdpa-blk simulator I had the same issues and I'm adding a > .get_config_size() callback to vdpa devices. > > Do you think make sense or is better to remove this check in > vhost/vdpa, delegating the boundaries checks to get_config/set_config > callbacks. A question here. How much value could we gain from get_config_size() consider we can let vDPA parent to validate the length in its get_config(). Thanks > > Thanks, > Stefano >
On Wed, Jan 27, 2021 at 11:33:03AM +0800, Jason Wang wrote: > >On 2021/1/20 下午7:08, Stefano Garzarella wrote: >>On Wed, Jan 20, 2021 at 11:46:38AM +0800, Jason Wang wrote: >>> >>>On 2021/1/19 下午12:59, Xie Yongji wrote: >>>>With VDUSE, we should be able to support all kinds of virtio devices. >>>> >>>>Signed-off-by: Xie Yongji <xieyongji@bytedance.com> >>>>--- >>>> drivers/vhost/vdpa.c | 29 +++-------------------------- >>>> 1 file changed, 3 insertions(+), 26 deletions(-) >>>> >>>>diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >>>>index 29ed4173f04e..448be7875b6d 100644 >>>>--- a/drivers/vhost/vdpa.c >>>>+++ b/drivers/vhost/vdpa.c >>>>@@ -22,6 +22,7 @@ >>>> #include <linux/nospec.h> >>>> #include <linux/vhost.h> >>>> #include <linux/virtio_net.h> >>>>+#include <linux/virtio_blk.h> >>>> #include "vhost.h" >>>>@@ -185,26 +186,6 @@ static long vhost_vdpa_set_status(struct >>>>vhost_vdpa *v, u8 __user *statusp) >>>> return 0; >>>> } >>>>-static int vhost_vdpa_config_validate(struct vhost_vdpa *v, >>>>- struct vhost_vdpa_config *c) >>>>-{ >>>>- long size = 0; >>>>- >>>>- switch (v->virtio_id) { >>>>- case VIRTIO_ID_NET: >>>>- size = sizeof(struct virtio_net_config); >>>>- break; >>>>- } >>>>- >>>>- if (c->len == 0) >>>>- return -EINVAL; >>>>- >>>>- if (c->len > size - c->off) >>>>- return -E2BIG; >>>>- >>>>- return 0; >>>>-} >>> >>> >>>I think we should use a separate patch for this. >> >>For the vdpa-blk simulator I had the same issues and I'm adding a >>.get_config_size() callback to vdpa devices. >> >>Do you think make sense or is better to remove this check in >>vhost/vdpa, delegating the boundaries checks to >>get_config/set_config callbacks. > > >A question here. How much value could we gain from get_config_size() >consider we can let vDPA parent to validate the length in its >get_config(). > I agree, most of the implementations already validate the length, the only gain is an error returned since get_config() is void, but eventually we can add a return value to it. Thanks, Stefano
On Tue, Jan 19, 2021 at 12:59:12PM +0800, Xie Yongji wrote: >With VDUSE, we should be able to support all kinds of virtio devices. > >Signed-off-by: Xie Yongji <xieyongji@bytedance.com> >--- > drivers/vhost/vdpa.c | 29 +++-------------------------- > 1 file changed, 3 insertions(+), 26 deletions(-) > >diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >index 29ed4173f04e..448be7875b6d 100644 >--- a/drivers/vhost/vdpa.c >+++ b/drivers/vhost/vdpa.c >@@ -22,6 +22,7 @@ > #include <linux/nospec.h> > #include <linux/vhost.h> > #include <linux/virtio_net.h> >+#include <linux/virtio_blk.h> Is this inclusion necessary? Maybe we can remove virtio_net.h as well. Thanks, Stefano > > #include "vhost.h" > >@@ -185,26 +186,6 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) > return 0; > } > >-static int vhost_vdpa_config_validate(struct vhost_vdpa *v, >- struct vhost_vdpa_config *c) >-{ >- long size = 0; >- >- switch (v->virtio_id) { >- case VIRTIO_ID_NET: >- size = sizeof(struct virtio_net_config); >- break; >- } >- >- if (c->len == 0) >- return -EINVAL; >- >- if (c->len > size - c->off) >- return -E2BIG; >- >- return 0; >-} >- > static long vhost_vdpa_get_config(struct vhost_vdpa *v, > struct vhost_vdpa_config __user *c) > { >@@ -215,7 +196,7 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v, > > if (copy_from_user(&config, c, size)) > return -EFAULT; >- if (vhost_vdpa_config_validate(v, &config)) >+ if (config.len == 0) > return -EINVAL; > buf = kvzalloc(config.len, GFP_KERNEL); > if (!buf) >@@ -243,7 +224,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, > > if (copy_from_user(&config, c, size)) > return -EFAULT; >- if (vhost_vdpa_config_validate(v, &config)) >+ if (config.len == 0) > return -EINVAL; > buf = kvzalloc(config.len, GFP_KERNEL); > if (!buf) >@@ -1025,10 +1006,6 @@ static int vhost_vdpa_probe(struct vdpa_device *vdpa) > int minor; > int r; > >- /* Currently, we only accept the network devices. */ >- if (ops->get_device_id(vdpa) != VIRTIO_ID_NET) >- return -ENOTSUPP; >- > v = kzalloc(sizeof(*v), GFP_KERNEL | __GFP_RETRY_MAYFAIL); > if (!v) > return -ENOMEM; >-- >2.11.0 >
On Wed, Jan 27, 2021 at 4:59 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Tue, Jan 19, 2021 at 12:59:12PM +0800, Xie Yongji wrote: > >With VDUSE, we should be able to support all kinds of virtio devices. > > > >Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > >--- > > drivers/vhost/vdpa.c | 29 +++-------------------------- > > 1 file changed, 3 insertions(+), 26 deletions(-) > > > >diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > >index 29ed4173f04e..448be7875b6d 100644 > >--- a/drivers/vhost/vdpa.c > >+++ b/drivers/vhost/vdpa.c > >@@ -22,6 +22,7 @@ > > #include <linux/nospec.h> > > #include <linux/vhost.h> > > #include <linux/virtio_net.h> > >+#include <linux/virtio_blk.h> > > Is this inclusion necessary? > My mistake... > Maybe we can remove virtio_net.h as well. > Agree. Thanks, Yongji
On 2021/1/27 下午4:57, Stefano Garzarella wrote: > On Wed, Jan 27, 2021 at 11:33:03AM +0800, Jason Wang wrote: >> >> On 2021/1/20 下午7:08, Stefano Garzarella wrote: >>> On Wed, Jan 20, 2021 at 11:46:38AM +0800, Jason Wang wrote: >>>> >>>> On 2021/1/19 下午12:59, Xie Yongji wrote: >>>>> With VDUSE, we should be able to support all kinds of virtio devices. >>>>> >>>>> Signed-off-by: Xie Yongji <xieyongji@bytedance.com> >>>>> --- >>>>> drivers/vhost/vdpa.c | 29 +++-------------------------- >>>>> 1 file changed, 3 insertions(+), 26 deletions(-) >>>>> >>>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >>>>> index 29ed4173f04e..448be7875b6d 100644 >>>>> --- a/drivers/vhost/vdpa.c >>>>> +++ b/drivers/vhost/vdpa.c >>>>> @@ -22,6 +22,7 @@ >>>>> #include <linux/nospec.h> >>>>> #include <linux/vhost.h> >>>>> #include <linux/virtio_net.h> >>>>> +#include <linux/virtio_blk.h> >>>>> #include "vhost.h" >>>>> @@ -185,26 +186,6 @@ static long vhost_vdpa_set_status(struct >>>>> vhost_vdpa *v, u8 __user *statusp) >>>>> return 0; >>>>> } >>>>> -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, >>>>> - struct vhost_vdpa_config *c) >>>>> -{ >>>>> - long size = 0; >>>>> - >>>>> - switch (v->virtio_id) { >>>>> - case VIRTIO_ID_NET: >>>>> - size = sizeof(struct virtio_net_config); >>>>> - break; >>>>> - } >>>>> - >>>>> - if (c->len == 0) >>>>> - return -EINVAL; >>>>> - >>>>> - if (c->len > size - c->off) >>>>> - return -E2BIG; >>>>> - >>>>> - return 0; >>>>> -} >>>> >>>> >>>> I think we should use a separate patch for this. >>> >>> For the vdpa-blk simulator I had the same issues and I'm adding a >>> .get_config_size() callback to vdpa devices. >>> >>> Do you think make sense or is better to remove this check in >>> vhost/vdpa, delegating the boundaries checks to >>> get_config/set_config callbacks. >> >> >> A question here. How much value could we gain from get_config_size() >> consider we can let vDPA parent to validate the length in its >> get_config(). >> > > I agree, most of the implementations already validate the length, the > only gain is an error returned since get_config() is void, but > eventually we can add a return value to it. Right, one problem here is that. For the virito path, its get_config() returns void. So we can not propagate error to virtio drivers. But it might not be a big issue since we trust kernel virtio driver. So I think it makes sense to change the return value in the vdpa config ops. Thanks > > Thanks, > Stefano >
On Fri, Jan 29, 2021 at 11:04 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Thu, Jan 28, 2021 at 11:11:49AM +0800, Jason Wang wrote: > > > >On 2021/1/27 下午4:57, Stefano Garzarella wrote: > >>On Wed, Jan 27, 2021 at 11:33:03AM +0800, Jason Wang wrote: > >>> > >>>On 2021/1/20 下午7:08, Stefano Garzarella wrote: > >>>>On Wed, Jan 20, 2021 at 11:46:38AM +0800, Jason Wang wrote: > >>>>> > >>>>>On 2021/1/19 下午12:59, Xie Yongji wrote: > >>>>>>With VDUSE, we should be able to support all kinds of virtio devices. > >>>>>> > >>>>>>Signed-off-by: Xie Yongji <xieyongji@bytedance.com> > >>>>>>--- > >>>>>> drivers/vhost/vdpa.c | 29 +++-------------------------- > >>>>>> 1 file changed, 3 insertions(+), 26 deletions(-) > >>>>>> > >>>>>>diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c > >>>>>>index 29ed4173f04e..448be7875b6d 100644 > >>>>>>--- a/drivers/vhost/vdpa.c > >>>>>>+++ b/drivers/vhost/vdpa.c > >>>>>>@@ -22,6 +22,7 @@ > >>>>>> #include <linux/nospec.h> > >>>>>> #include <linux/vhost.h> > >>>>>> #include <linux/virtio_net.h> > >>>>>>+#include <linux/virtio_blk.h> > >>>>>> #include "vhost.h" > >>>>>>@@ -185,26 +186,6 @@ static long > >>>>>>vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user > >>>>>>*statusp) > >>>>>> return 0; > >>>>>> } > >>>>>>-static int vhost_vdpa_config_validate(struct vhost_vdpa *v, > >>>>>>- struct vhost_vdpa_config *c) > >>>>>>-{ > >>>>>>- long size = 0; > >>>>>>- > >>>>>>- switch (v->virtio_id) { > >>>>>>- case VIRTIO_ID_NET: > >>>>>>- size = sizeof(struct virtio_net_config); > >>>>>>- break; > >>>>>>- } > >>>>>>- > >>>>>>- if (c->len == 0) > >>>>>>- return -EINVAL; > >>>>>>- > >>>>>>- if (c->len > size - c->off) > >>>>>>- return -E2BIG; > >>>>>>- > >>>>>>- return 0; > >>>>>>-} > >>>>> > >>>>> > >>>>>I think we should use a separate patch for this. > >>>> > >>>>For the vdpa-blk simulator I had the same issues and I'm adding > >>>>a .get_config_size() callback to vdpa devices. > >>>> > >>>>Do you think make sense or is better to remove this check in > >>>>vhost/vdpa, delegating the boundaries checks to > >>>>get_config/set_config callbacks. > >>> > >>> > >>>A question here. How much value could we gain from > >>>get_config_size() consider we can let vDPA parent to validate the > >>>length in its get_config(). > >>> > >> > >>I agree, most of the implementations already validate the length, > >>the only gain is an error returned since get_config() is void, but > >>eventually we can add a return value to it. > > > > > >Right, one problem here is that. For the virito path, its get_config() > >returns void. So we can not propagate error to virtio drivers. But it > >might not be a big issue since we trust kernel virtio driver. > > > >So I think it makes sense to change the return value in the vdpa config ops. > > Thanks for your feedback! > > @Xie do you plan to do this? > > Otherwise I can do in my vdpa-blk-sim series, where for now I used this > patch as is. > Hi Stefano, please do in your vdpa-blk-sim series. I have no plan for it now. Thanks, Yongji
On Sat, Jan 30, 2021 at 07:33:08PM +0800, Yongji Xie wrote: >On Fri, Jan 29, 2021 at 11:04 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> On Thu, Jan 28, 2021 at 11:11:49AM +0800, Jason Wang wrote: >> > >> >On 2021/1/27 下午4:57, Stefano Garzarella wrote: >> >>On Wed, Jan 27, 2021 at 11:33:03AM +0800, Jason Wang wrote: >> >>> >> >>>On 2021/1/20 下午7:08, Stefano Garzarella wrote: >> >>>>On Wed, Jan 20, 2021 at 11:46:38AM +0800, Jason Wang wrote: >> >>>>> >> >>>>>On 2021/1/19 下午12:59, Xie Yongji wrote: >> >>>>>>With VDUSE, we should be able to support all kinds of virtio devices. >> >>>>>> >> >>>>>>Signed-off-by: Xie Yongji <xieyongji@bytedance.com> >> >>>>>>--- >> >>>>>> drivers/vhost/vdpa.c | 29 +++-------------------------- >> >>>>>> 1 file changed, 3 insertions(+), 26 deletions(-) >> >>>>>> >> >>>>>>diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c >> >>>>>>index 29ed4173f04e..448be7875b6d 100644 >> >>>>>>--- a/drivers/vhost/vdpa.c >> >>>>>>+++ b/drivers/vhost/vdpa.c >> >>>>>>@@ -22,6 +22,7 @@ >> >>>>>> #include <linux/nospec.h> >> >>>>>> #include <linux/vhost.h> >> >>>>>> #include <linux/virtio_net.h> >> >>>>>>+#include <linux/virtio_blk.h> >> >>>>>> #include "vhost.h" >> >>>>>>@@ -185,26 +186,6 @@ static long >> >>>>>>vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user >> >>>>>>*statusp) >> >>>>>> return 0; >> >>>>>> } >> >>>>>>-static int vhost_vdpa_config_validate(struct vhost_vdpa *v, >> >>>>>>- struct vhost_vdpa_config *c) >> >>>>>>-{ >> >>>>>>- long size = 0; >> >>>>>>- >> >>>>>>- switch (v->virtio_id) { >> >>>>>>- case VIRTIO_ID_NET: >> >>>>>>- size = sizeof(struct virtio_net_config); >> >>>>>>- break; >> >>>>>>- } >> >>>>>>- >> >>>>>>- if (c->len == 0) >> >>>>>>- return -EINVAL; >> >>>>>>- >> >>>>>>- if (c->len > size - c->off) >> >>>>>>- return -E2BIG; >> >>>>>>- >> >>>>>>- return 0; >> >>>>>>-} >> >>>>> >> >>>>> >> >>>>>I think we should use a separate patch for this. >> >>>> >> >>>>For the vdpa-blk simulator I had the same issues and I'm adding >> >>>>a .get_config_size() callback to vdpa devices. >> >>>> >> >>>>Do you think make sense or is better to remove this check in >> >>>>vhost/vdpa, delegating the boundaries checks to >> >>>>get_config/set_config callbacks. >> >>> >> >>> >> >>>A question here. How much value could we gain from >> >>>get_config_size() consider we can let vDPA parent to validate the >> >>>length in its get_config(). >> >>> >> >> >> >>I agree, most of the implementations already validate the length, >> >>the only gain is an error returned since get_config() is void, but >> >>eventually we can add a return value to it. >> > >> > >> >Right, one problem here is that. For the virito path, its get_config() >> >returns void. So we can not propagate error to virtio drivers. But it >> >might not be a big issue since we trust kernel virtio driver. >> > >> >So I think it makes sense to change the return value in the vdpa config ops. >> >> Thanks for your feedback! >> >> @Xie do you plan to do this? >> >> Otherwise I can do in my vdpa-blk-sim series, where for now I used this >> patch as is. >> > >Hi Stefano, please do in your vdpa-blk-sim series. I have no plan for >it now. Okay, I'll do it. Thanks, Stefano
diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c index 29ed4173f04e..448be7875b6d 100644 --- a/drivers/vhost/vdpa.c +++ b/drivers/vhost/vdpa.c @@ -22,6 +22,7 @@ #include <linux/nospec.h> #include <linux/vhost.h> #include <linux/virtio_net.h> +#include <linux/virtio_blk.h> #include "vhost.h" @@ -185,26 +186,6 @@ static long vhost_vdpa_set_status(struct vhost_vdpa *v, u8 __user *statusp) return 0; } -static int vhost_vdpa_config_validate(struct vhost_vdpa *v, - struct vhost_vdpa_config *c) -{ - long size = 0; - - switch (v->virtio_id) { - case VIRTIO_ID_NET: - size = sizeof(struct virtio_net_config); - break; - } - - if (c->len == 0) - return -EINVAL; - - if (c->len > size - c->off) - return -E2BIG; - - return 0; -} - static long vhost_vdpa_get_config(struct vhost_vdpa *v, struct vhost_vdpa_config __user *c) { @@ -215,7 +196,7 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v, if (copy_from_user(&config, c, size)) return -EFAULT; - if (vhost_vdpa_config_validate(v, &config)) + if (config.len == 0) return -EINVAL; buf = kvzalloc(config.len, GFP_KERNEL); if (!buf) @@ -243,7 +224,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v, if (copy_from_user(&config, c, size)) return -EFAULT; - if (vhost_vdpa_config_validate(v, &config)) + if (config.len == 0) return -EINVAL; buf = kvzalloc(config.len, GFP_KERNEL); if (!buf) @@ -1025,10 +1006,6 @@ static int vhost_vdpa_probe(struct vdpa_device *vdpa) int minor; int r; - /* Currently, we only accept the network devices. */ - if (ops->get_device_id(vdpa) != VIRTIO_ID_NET) - return -ENOTSUPP; - v = kzalloc(sizeof(*v), GFP_KERNEL | __GFP_RETRY_MAYFAIL); if (!v) return -ENOMEM;
With VDUSE, we should be able to support all kinds of virtio devices. Signed-off-by: Xie Yongji <xieyongji@bytedance.com> --- drivers/vhost/vdpa.c | 29 +++-------------------------- 1 file changed, 3 insertions(+), 26 deletions(-)