diff mbox series

vhost: fix crash if set vring num handling failed

Message ID 20180817113354.16140-1-i.maximets@samsung.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: fix crash if set vring num handling failed |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Ilya Maximets Aug. 17, 2018, 11:33 a.m. UTC 
  Allocation failures of shadow used ring and batched copy array
are not recoverable and leads to the segmentation faults like
this on the receiving/transmission path:

  Program received signal SIGSEGV, Segmentation fault.
  [Switching to Thread 0x7f913fecf0 (LWP 43625)]
  in copy_desc_to_mbuf () at /lib/librte_vhost/virtio_net.c:760
  760       batch_copy[vq->batch_copy_nb_elems].dst =

This could be easily reproduced in case of low memory or big
number of vhost-user ports. Fix that by propagating error to
the upper layer which will end up with disconnection.

Fixes: f689586bc060 ("vhost: shadow used ring update")
Cc: stable@dpdk.org

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 lib/librte_vhost/vhost_user.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Ilya Maximets Aug. 29, 2018, 2 p.m. UTC | #1 
Any thoughts on this?

Best regards, Ilya Maximets.

On 17.08.2018 14:33, Ilya Maximets wrote:
> Allocation failures of shadow used ring and batched copy array
> are not recoverable and leads to the segmentation faults like
> this on the receiving/transmission path:
> 
>   Program received signal SIGSEGV, Segmentation fault.
>   [Switching to Thread 0x7f913fecf0 (LWP 43625)]
>   in copy_desc_to_mbuf () at /lib/librte_vhost/virtio_net.c:760
>   760       batch_copy[vq->batch_copy_nb_elems].dst =
> 
> This could be easily reproduced in case of low memory or big
> number of vhost-user ports. Fix that by propagating error to
> the upper layer which will end up with disconnection.
> 
> Fixes: f689586bc060 ("vhost: shadow used ring update")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>  lib/librte_vhost/vhost_user.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 9aa1ce118..4c7fd57fb 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1693,7 +1693,9 @@ vhost_user_msg_handler(int vid, int fd)
>  		break;
>  
>  	case VHOST_USER_SET_VRING_NUM:
> -		vhost_user_set_vring_num(dev, &msg);
> +		ret = vhost_user_set_vring_num(dev, &msg);
> +		if (ret)
> +			return -1;
>  		break;
>  	case VHOST_USER_SET_VRING_ADDR:
>  		vhost_user_set_vring_addr(&dev, &msg);
>
Tiwei Bie Aug. 30, 2018, 6:06 a.m. UTC | #2 
On Wed, Aug 29, 2018 at 05:00:47PM +0300, Ilya Maximets wrote:
> Any thoughts on this?

Hi Ilya,

Currently, the errors happened during messages handling
aren't handled properly. E.g. in vhost_user_set_mem_table(),
similar issues [1] may happen too. We might want to find
a way to fix this once and for all.

[1]
https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L826
https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L837
https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L887

Thanks

> 
> Best regards, Ilya Maximets.
> 
> On 17.08.2018 14:33, Ilya Maximets wrote:
> > Allocation failures of shadow used ring and batched copy array
> > are not recoverable and leads to the segmentation faults like
> > this on the receiving/transmission path:
> > 
> >   Program received signal SIGSEGV, Segmentation fault.
> >   [Switching to Thread 0x7f913fecf0 (LWP 43625)]
> >   in copy_desc_to_mbuf () at /lib/librte_vhost/virtio_net.c:760
> >   760       batch_copy[vq->batch_copy_nb_elems].dst =
> > 
> > This could be easily reproduced in case of low memory or big
> > number of vhost-user ports. Fix that by propagating error to
> > the upper layer which will end up with disconnection.
> > 
> > Fixes: f689586bc060 ("vhost: shadow used ring update")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > ---
> >  lib/librte_vhost/vhost_user.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index 9aa1ce118..4c7fd57fb 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -1693,7 +1693,9 @@ vhost_user_msg_handler(int vid, int fd)
> >  		break;
> >  
> >  	case VHOST_USER_SET_VRING_NUM:
> > -		vhost_user_set_vring_num(dev, &msg);
> > +		ret = vhost_user_set_vring_num(dev, &msg);
> > +		if (ret)
> > +			return -1;
> >  		break;
> >  	case VHOST_USER_SET_VRING_ADDR:
> >  		vhost_user_set_vring_addr(&dev, &msg);
> >
Ilya Maximets Aug. 30, 2018, 6:54 a.m. UTC | #3 
On 30.08.2018 09:06, Tiwei Bie wrote:
> On Wed, Aug 29, 2018 at 05:00:47PM +0300, Ilya Maximets wrote:
>> Any thoughts on this?
> 
> Hi Ilya,
> 
> Currently, the errors happened during messages handling
> aren't handled properly. E.g. in vhost_user_set_mem_table(),
> similar issues [1] may happen too. We might want to find
> a way to fix this once and for all.
> 
> [1]
> https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L826
> https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L837
> https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L887
> 
> Thanks

Hi. I understand your position position and, actually, I looked at
other functions while preparing this fix. There is one difference
between vhost_user_set_mem_table() and vhost_user_set_vring_num().
The first one is able to reply bad status.
But yes, you're right that we should handle all the errors.
What about following patch (compile tested only):
----
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 9aa1ce118..63d145b2d 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1014,7 +1014,7 @@ vhost_user_set_vring_call(struct virtio_net *dev, struct VhostUserMsg *pmsg)
 	vq->callfd = file.fd;
 }
 
-static void
+static int
 vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
 {
 	struct vhost_vring_file file;
@@ -1032,7 +1032,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
 	/* Interpret ring addresses only when ring is started. */
 	dev = translate_ring_addresses(dev, file.index);
 	if (!dev)
-		return;
+		return -1;
 
 	*pdev = dev;
 
@@ -1049,6 +1049,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
 	if (vq->kickfd >= 0)
 		close(vq->kickfd);
 	vq->kickfd = file.fd;
+	return 0;
 }
 
 static void
@@ -1172,14 +1173,19 @@ vhost_user_get_protocol_features(struct virtio_net *dev,
 	msg->size = sizeof(msg->payload.u64);
 }
 
-static void
+static int
 vhost_user_set_protocol_features(struct virtio_net *dev,
 				 uint64_t protocol_features)
 {
-	if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES)
-		return;
+	if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"(%d) received invalid protocol features.\n",
+			dev->vid);
+		return -1;
+	}
 
 	dev->protocol_features = protocol_features;
+	return 0;
 }
 
 static int
@@ -1657,8 +1663,6 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 	case VHOST_USER_SET_FEATURES:
 		ret = vhost_user_set_features(dev, msg.payload.u64);
-		if (ret)
-			return -1;
 		break;
 
 	case VHOST_USER_GET_PROTOCOL_FEATURES:
@@ -1666,14 +1670,14 @@ vhost_user_msg_handler(int vid, int fd)
 		send_vhost_reply(fd, &msg);
 		break;
 	case VHOST_USER_SET_PROTOCOL_FEATURES:
-		vhost_user_set_protocol_features(dev, msg.payload.u64);
+		ret = vhost_user_set_protocol_features(dev, msg.payload.u64);
 		break;
 
 	case VHOST_USER_SET_OWNER:
-		vhost_user_set_owner();
+		ret = vhost_user_set_owner();
 		break;
 	case VHOST_USER_RESET_OWNER:
-		vhost_user_reset_owner(dev);
+		ret = vhost_user_reset_owner(dev);
 		break;
 
 	case VHOST_USER_SET_MEM_TABLE:
@@ -1681,8 +1685,9 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_LOG_BASE:
-		vhost_user_set_log_base(dev, &msg);
-
+		ret = vhost_user_set_log_base(dev, &msg);
+		if (ret)
+			goto skip_to_reply;
 		/* it needs a reply */
 		msg.size = sizeof(msg.payload.u64);
 		send_vhost_reply(fd, &msg);
@@ -1693,23 +1698,25 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_NUM:
-		vhost_user_set_vring_num(dev, &msg);
+		ret = vhost_user_set_vring_num(dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_ADDR:
-		vhost_user_set_vring_addr(&dev, &msg);
+		ret = vhost_user_set_vring_addr(&dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_BASE:
-		vhost_user_set_vring_base(dev, &msg);
+		ret = vhost_user_set_vring_base(dev, &msg);
 		break;
 
 	case VHOST_USER_GET_VRING_BASE:
-		vhost_user_get_vring_base(dev, &msg);
+		ret = vhost_user_get_vring_base(dev, &msg);
+		if (ret)
+			goto skip_to_reply;
 		msg.size = sizeof(msg.payload.state);
 		send_vhost_reply(fd, &msg);
 		break;
 
 	case VHOST_USER_SET_VRING_KICK:
-		vhost_user_set_vring_kick(&dev, &msg);
+		ret = vhost_user_set_vring_kick(&dev, &msg);
 		break;
 	case VHOST_USER_SET_VRING_CALL:
 		vhost_user_set_vring_call(dev, &msg);
@@ -1728,10 +1735,10 @@ vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_ENABLE:
-		vhost_user_set_vring_enable(dev, &msg);
+		ret = vhost_user_set_vring_enable(dev, &msg);
 		break;
 	case VHOST_USER_SEND_RARP:
-		vhost_user_send_rarp(dev, &msg);
+		ret = vhost_user_send_rarp(dev, &msg);
 		break;
 
 	case VHOST_USER_NET_SET_MTU:
@@ -1752,7 +1759,7 @@ vhost_user_msg_handler(int vid, int fd)
 	}
 
 skip_to_post_handle:
-	if (dev->extern_ops.post_msg_handle) {
+	if (!ret && dev->extern_ops.post_msg_handle) {
 		uint32_t need_reply;
 
 		ret = (*dev->extern_ops.post_msg_handle)(
@@ -1772,6 +1779,10 @@ vhost_user_msg_handler(int vid, int fd)
 		msg.payload.u64 = !!ret;
 		msg.size = sizeof(msg.payload.u64);
 		send_vhost_reply(fd, &msg);
+	} else if (ret) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"vhost message handling failed.\n");
+		return -1;
 	}
 
 	if (!(dev->flags & VIRTIO_DEV_RUNNING) && virtio_is_ready(dev)) {

----

Best regards, Ilya Maximets.
Tiwei Bie Sept. 3, 2018, 2:26 a.m. UTC | #4 
On Thu, Aug 30, 2018 at 09:54:50AM +0300, Ilya Maximets wrote:
> On 30.08.2018 09:06, Tiwei Bie wrote:
> > On Wed, Aug 29, 2018 at 05:00:47PM +0300, Ilya Maximets wrote:
> >> Any thoughts on this?
> > 
> > Hi Ilya,
> > 
> > Currently, the errors happened during messages handling
> > aren't handled properly. E.g. in vhost_user_set_mem_table(),
> > similar issues [1] may happen too. We might want to find
> > a way to fix this once and for all.
> > 
> > [1]
> > https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L826
> > https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L837
> > https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L887
> > 
> > Thanks
> 
> Hi. I understand your position position and, actually, I looked at
> other functions while preparing this fix. There is one difference
> between vhost_user_set_mem_table() and vhost_user_set_vring_num().
> The first one is able to reply bad status.
> But yes, you're right that we should handle all the errors.
> What about following patch (compile tested only):
> ----
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 9aa1ce118..63d145b2d 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1014,7 +1014,7 @@ vhost_user_set_vring_call(struct virtio_net *dev, struct VhostUserMsg *pmsg)
>  	vq->callfd = file.fd;
>  }
>  
> -static void
> +static int
>  vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
>  {
>  	struct vhost_vring_file file;
> @@ -1032,7 +1032,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
>  	/* Interpret ring addresses only when ring is started. */
>  	dev = translate_ring_addresses(dev, file.index);
>  	if (!dev)
> -		return;
> +		return -1;
>  
>  	*pdev = dev;
>  
> @@ -1049,6 +1049,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
>  	if (vq->kickfd >= 0)
>  		close(vq->kickfd);
>  	vq->kickfd = file.fd;
> +	return 0;
>  }
>  
>  static void
> @@ -1172,14 +1173,19 @@ vhost_user_get_protocol_features(struct virtio_net *dev,
>  	msg->size = sizeof(msg->payload.u64);
>  }
>  
> -static void
> +static int
>  vhost_user_set_protocol_features(struct virtio_net *dev,
>  				 uint64_t protocol_features)
>  {
> -	if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES)
> -		return;
> +	if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"(%d) received invalid protocol features.\n",
> +			dev->vid);
> +		return -1;
> +	}
>  
>  	dev->protocol_features = protocol_features;
> +	return 0;
>  }
>  
>  static int
> @@ -1657,8 +1663,6 @@ vhost_user_msg_handler(int vid, int fd)
>  		break;
>  	case VHOST_USER_SET_FEATURES:
>  		ret = vhost_user_set_features(dev, msg.payload.u64);
> -		if (ret)
> -			return -1;
>  		break;
>  
>  	case VHOST_USER_GET_PROTOCOL_FEATURES:
> @@ -1666,14 +1670,14 @@ vhost_user_msg_handler(int vid, int fd)
>  		send_vhost_reply(fd, &msg);
>  		break;
>  	case VHOST_USER_SET_PROTOCOL_FEATURES:
> -		vhost_user_set_protocol_features(dev, msg.payload.u64);
> +		ret = vhost_user_set_protocol_features(dev, msg.payload.u64);
>  		break;
>  
>  	case VHOST_USER_SET_OWNER:
> -		vhost_user_set_owner();
> +		ret = vhost_user_set_owner();
>  		break;
>  	case VHOST_USER_RESET_OWNER:
> -		vhost_user_reset_owner(dev);
> +		ret = vhost_user_reset_owner(dev);
>  		break;
>  
>  	case VHOST_USER_SET_MEM_TABLE:
> @@ -1681,8 +1685,9 @@ vhost_user_msg_handler(int vid, int fd)
>  		break;
>  
>  	case VHOST_USER_SET_LOG_BASE:
> -		vhost_user_set_log_base(dev, &msg);
> -
> +		ret = vhost_user_set_log_base(dev, &msg);
> +		if (ret)
> +			goto skip_to_reply;
>  		/* it needs a reply */
>  		msg.size = sizeof(msg.payload.u64);
>  		send_vhost_reply(fd, &msg);
> @@ -1693,23 +1698,25 @@ vhost_user_msg_handler(int vid, int fd)
>  		break;
>  
>  	case VHOST_USER_SET_VRING_NUM:
> -		vhost_user_set_vring_num(dev, &msg);
> +		ret = vhost_user_set_vring_num(dev, &msg);
>  		break;
>  	case VHOST_USER_SET_VRING_ADDR:
> -		vhost_user_set_vring_addr(&dev, &msg);
> +		ret = vhost_user_set_vring_addr(&dev, &msg);
>  		break;
>  	case VHOST_USER_SET_VRING_BASE:
> -		vhost_user_set_vring_base(dev, &msg);
> +		ret = vhost_user_set_vring_base(dev, &msg);
>  		break;
>  
>  	case VHOST_USER_GET_VRING_BASE:
> -		vhost_user_get_vring_base(dev, &msg);
> +		ret = vhost_user_get_vring_base(dev, &msg);
> +		if (ret)
> +			goto skip_to_reply;
>  		msg.size = sizeof(msg.payload.state);
>  		send_vhost_reply(fd, &msg);
>  		break;
>  
>  	case VHOST_USER_SET_VRING_KICK:
> -		vhost_user_set_vring_kick(&dev, &msg);
> +		ret = vhost_user_set_vring_kick(&dev, &msg);
>  		break;
>  	case VHOST_USER_SET_VRING_CALL:
>  		vhost_user_set_vring_call(dev, &msg);
> @@ -1728,10 +1735,10 @@ vhost_user_msg_handler(int vid, int fd)
>  		break;
>  
>  	case VHOST_USER_SET_VRING_ENABLE:
> -		vhost_user_set_vring_enable(dev, &msg);
> +		ret = vhost_user_set_vring_enable(dev, &msg);
>  		break;
>  	case VHOST_USER_SEND_RARP:
> -		vhost_user_send_rarp(dev, &msg);
> +		ret = vhost_user_send_rarp(dev, &msg);
>  		break;
>  
>  	case VHOST_USER_NET_SET_MTU:
> @@ -1752,7 +1759,7 @@ vhost_user_msg_handler(int vid, int fd)
>  	}
>  
>  skip_to_post_handle:
> -	if (dev->extern_ops.post_msg_handle) {
> +	if (!ret && dev->extern_ops.post_msg_handle) {
>  		uint32_t need_reply;
>  
>  		ret = (*dev->extern_ops.post_msg_handle)(
> @@ -1772,6 +1779,10 @@ vhost_user_msg_handler(int vid, int fd)
>  		msg.payload.u64 = !!ret;
>  		msg.size = sizeof(msg.payload.u64);
>  		send_vhost_reply(fd, &msg);
> +	} else if (ret) {
> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"vhost message handling failed.\n");
> +		return -1;
>  	}

The basic idea behind above code is to drop the connection
if we can't report the error to QEMU when error happens. It
looks good to me.

Thanks

>  
>  	if (!(dev->flags & VIRTIO_DEV_RUNNING) && virtio_is_ready(dev)) {
> 
> ----
> 
> Best regards, Ilya Maximets.
Ilya Maximets Sept. 3, 2018, 7:07 a.m. UTC | #5 
On 03.09.2018 05:26, Tiwei Bie wrote:
> On Thu, Aug 30, 2018 at 09:54:50AM +0300, Ilya Maximets wrote:
>> On 30.08.2018 09:06, Tiwei Bie wrote:
>>> On Wed, Aug 29, 2018 at 05:00:47PM +0300, Ilya Maximets wrote:
>>>> Any thoughts on this?
>>>
>>> Hi Ilya,
>>>
>>> Currently, the errors happened during messages handling
>>> aren't handled properly. E.g. in vhost_user_set_mem_table(),
>>> similar issues [1] may happen too. We might want to find
>>> a way to fix this once and for all.
>>>
>>> [1]
>>> https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L826
>>> https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L837
>>> https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L887
>>>
>>> Thanks
>>
>> Hi. I understand your position position and, actually, I looked at
>> other functions while preparing this fix. There is one difference
>> between vhost_user_set_mem_table() and vhost_user_set_vring_num().
>> The first one is able to reply bad status.
>> But yes, you're right that we should handle all the errors.
>> What about following patch (compile tested only):
>> ----
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index 9aa1ce118..63d145b2d 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -1014,7 +1014,7 @@ vhost_user_set_vring_call(struct virtio_net *dev, struct VhostUserMsg *pmsg)
>>  	vq->callfd = file.fd;
>>  }
>>  
>> -static void
>> +static int
>>  vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
>>  {
>>  	struct vhost_vring_file file;
>> @@ -1032,7 +1032,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
>>  	/* Interpret ring addresses only when ring is started. */
>>  	dev = translate_ring_addresses(dev, file.index);
>>  	if (!dev)
>> -		return;
>> +		return -1;
>>  
>>  	*pdev = dev;
>>  
>> @@ -1049,6 +1049,7 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
>>  	if (vq->kickfd >= 0)
>>  		close(vq->kickfd);
>>  	vq->kickfd = file.fd;
>> +	return 0;
>>  }
>>  
>>  static void
>> @@ -1172,14 +1173,19 @@ vhost_user_get_protocol_features(struct virtio_net *dev,
>>  	msg->size = sizeof(msg->payload.u64);
>>  }
>>  
>> -static void
>> +static int
>>  vhost_user_set_protocol_features(struct virtio_net *dev,
>>  				 uint64_t protocol_features)
>>  {
>> -	if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES)
>> -		return;
>> +	if (protocol_features & ~VHOST_USER_PROTOCOL_FEATURES) {
>> +		RTE_LOG(ERR, VHOST_CONFIG,
>> +			"(%d) received invalid protocol features.\n",
>> +			dev->vid);
>> +		return -1;
>> +	}
>>  
>>  	dev->protocol_features = protocol_features;
>> +	return 0;
>>  }
>>  
>>  static int
>> @@ -1657,8 +1663,6 @@ vhost_user_msg_handler(int vid, int fd)
>>  		break;
>>  	case VHOST_USER_SET_FEATURES:
>>  		ret = vhost_user_set_features(dev, msg.payload.u64);
>> -		if (ret)
>> -			return -1;
>>  		break;
>>  
>>  	case VHOST_USER_GET_PROTOCOL_FEATURES:
>> @@ -1666,14 +1670,14 @@ vhost_user_msg_handler(int vid, int fd)
>>  		send_vhost_reply(fd, &msg);
>>  		break;
>>  	case VHOST_USER_SET_PROTOCOL_FEATURES:
>> -		vhost_user_set_protocol_features(dev, msg.payload.u64);
>> +		ret = vhost_user_set_protocol_features(dev, msg.payload.u64);
>>  		break;
>>  
>>  	case VHOST_USER_SET_OWNER:
>> -		vhost_user_set_owner();
>> +		ret = vhost_user_set_owner();
>>  		break;
>>  	case VHOST_USER_RESET_OWNER:
>> -		vhost_user_reset_owner(dev);
>> +		ret = vhost_user_reset_owner(dev);
>>  		break;
>>  
>>  	case VHOST_USER_SET_MEM_TABLE:
>> @@ -1681,8 +1685,9 @@ vhost_user_msg_handler(int vid, int fd)
>>  		break;
>>  
>>  	case VHOST_USER_SET_LOG_BASE:
>> -		vhost_user_set_log_base(dev, &msg);
>> -
>> +		ret = vhost_user_set_log_base(dev, &msg);
>> +		if (ret)
>> +			goto skip_to_reply;
>>  		/* it needs a reply */
>>  		msg.size = sizeof(msg.payload.u64);
>>  		send_vhost_reply(fd, &msg);
>> @@ -1693,23 +1698,25 @@ vhost_user_msg_handler(int vid, int fd)
>>  		break;
>>  
>>  	case VHOST_USER_SET_VRING_NUM:
>> -		vhost_user_set_vring_num(dev, &msg);
>> +		ret = vhost_user_set_vring_num(dev, &msg);
>>  		break;
>>  	case VHOST_USER_SET_VRING_ADDR:
>> -		vhost_user_set_vring_addr(&dev, &msg);
>> +		ret = vhost_user_set_vring_addr(&dev, &msg);
>>  		break;
>>  	case VHOST_USER_SET_VRING_BASE:
>> -		vhost_user_set_vring_base(dev, &msg);
>> +		ret = vhost_user_set_vring_base(dev, &msg);
>>  		break;
>>  
>>  	case VHOST_USER_GET_VRING_BASE:
>> -		vhost_user_get_vring_base(dev, &msg);
>> +		ret = vhost_user_get_vring_base(dev, &msg);
>> +		if (ret)
>> +			goto skip_to_reply;
>>  		msg.size = sizeof(msg.payload.state);
>>  		send_vhost_reply(fd, &msg);
>>  		break;
>>  
>>  	case VHOST_USER_SET_VRING_KICK:
>> -		vhost_user_set_vring_kick(&dev, &msg);
>> +		ret = vhost_user_set_vring_kick(&dev, &msg);
>>  		break;
>>  	case VHOST_USER_SET_VRING_CALL:
>>  		vhost_user_set_vring_call(dev, &msg);
>> @@ -1728,10 +1735,10 @@ vhost_user_msg_handler(int vid, int fd)
>>  		break;
>>  
>>  	case VHOST_USER_SET_VRING_ENABLE:
>> -		vhost_user_set_vring_enable(dev, &msg);
>> +		ret = vhost_user_set_vring_enable(dev, &msg);
>>  		break;
>>  	case VHOST_USER_SEND_RARP:
>> -		vhost_user_send_rarp(dev, &msg);
>> +		ret = vhost_user_send_rarp(dev, &msg);
>>  		break;
>>  
>>  	case VHOST_USER_NET_SET_MTU:
>> @@ -1752,7 +1759,7 @@ vhost_user_msg_handler(int vid, int fd)
>>  	}
>>  
>>  skip_to_post_handle:
>> -	if (dev->extern_ops.post_msg_handle) {
>> +	if (!ret && dev->extern_ops.post_msg_handle) {
>>  		uint32_t need_reply;
>>  
>>  		ret = (*dev->extern_ops.post_msg_handle)(
>> @@ -1772,6 +1779,10 @@ vhost_user_msg_handler(int vid, int fd)
>>  		msg.payload.u64 = !!ret;
>>  		msg.size = sizeof(msg.payload.u64);
>>  		send_vhost_reply(fd, &msg);
>> +	} else if (ret) {
>> +		RTE_LOG(ERR, VHOST_CONFIG,
>> +			"vhost message handling failed.\n");
>> +		return -1;
>>  	}
> 
> The basic idea behind above code is to drop the connection
> if we can't report the error to QEMU when error happens. It
> looks good to me.
> 
> Thanks

OK. Good.
I'll prepare a proper patch.

Best regards, Ilya Maximets.

>>  
>>  	if (!(dev->flags & VIRTIO_DEV_RUNNING) && virtio_is_ready(dev)) {
>>
>> ----
>>
>> Best regards, Ilya Maximets.
> 
>
diff mbox series

Patch

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 9aa1ce118..4c7fd57fb 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1693,7 +1693,9 @@  vhost_user_msg_handler(int vid, int fd)
 		break;
 
 	case VHOST_USER_SET_VRING_NUM:
-		vhost_user_set_vring_num(dev, &msg);
+		ret = vhost_user_set_vring_num(dev, &msg);
+		if (ret)
+			return -1;
 		break;
 	case VHOST_USER_SET_VRING_ADDR:
 		vhost_user_set_vring_addr(&dev, &msg);