diff mbox series

[RFC,v3,03/11] vdpa: Remove the restriction that only supports virtio-net devices

Message ID 20210119045920.447-4-xieyongji@bytedance.com (mailing list archive)
State New
Headers show
Series Introduce VDUSE - vDPA Device in Userspace | expand

Commit Message

Xie Yongji Jan. 19, 2021, 4:59 a.m. UTC
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(-)

Comments

Jason Wang Jan. 20, 2021, 3:46 a.m. UTC | #1
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;
Xie Yongji Jan. 20, 2021, 6:46 a.m. UTC | #2
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
Stefano Garzarella Jan. 20, 2021, 11:08 a.m. UTC | #3
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
Jason Wang Jan. 27, 2021, 3:33 a.m. UTC | #4
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
>
Stefano Garzarella Jan. 27, 2021, 8:57 a.m. UTC | #5
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
Stefano Garzarella Jan. 27, 2021, 8:59 a.m. UTC | #6
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
>
Xie Yongji Jan. 27, 2021, 9:05 a.m. UTC | #7
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
Jason Wang Jan. 28, 2021, 3:11 a.m. UTC | #8
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
>
Xie Yongji Jan. 30, 2021, 11:33 a.m. UTC | #9
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
Stefano Garzarella Feb. 1, 2021, 11:05 a.m. UTC | #10
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 mbox series

Patch

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;