DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ilya Maximets <i.maximets@samsung.com>
To: Tiwei Bie <tiwei.bie@intel.com>
Cc: dev@dpdk.org, Maxime Coquelin <maxime.coquelin@redhat.com>,
	Zhihong Wang <zhihong.wang@intel.com>,
	stable@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] vhost: fix crash if set vring num handling failed
Date: Mon, 3 Sep 2018 10:07:18 +0300	[thread overview]
Message-ID: <20180903070552eucas1p2fb3bfba7191883bb84f2038a057c82fc~Q0fu924sK1503615036eucas1p21@eucas1p2.samsung.com> (raw)
In-Reply-To: <20180903022617.GA11310@fbsd.sh.intel.com>

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

  reply	other threads:[~2018-09-03  7:05 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20180817113255eucas1p2042d22241265dafa241c2a6a2c949244@eucas1p2.samsung.com>
2018-08-17 11:33 ` Ilya Maximets
2018-08-29 14:00   ` Ilya Maximets
2018-08-30  6:06     ` Tiwei Bie
2018-08-30  6:54       ` Ilya Maximets
2018-09-03  2:26         ` Tiwei Bie
2018-09-03  7:07           ` Ilya Maximets [this message]
     [not found]   ` <CGME20180903101059eucas1p2030d1e9077a03bc5d0613ba0311e1eab@eucas1p2.samsung.com>
2018-09-03 10:12     ` [dpdk-dev] [PATCH v2] vhost-user: drop connection on message handling failures Ilya Maximets
2018-09-10 13:51       ` Maxime Coquelin
2018-09-11  9:25       ` Maxime Coquelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='20180903070552eucas1p2fb3bfba7191883bb84f2038a057c82fc~Q0fu924sK1503615036eucas1p21@eucas1p2.samsung.com' \
    --to=i.maximets@samsung.com \
    --cc=dev@dpdk.org \
    --cc=maxime.coquelin@redhat.com \
    --cc=stable@dpdk.org \
    --cc=tiwei.bie@intel.com \
    --cc=zhihong.wang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).