[dpdk-dev] [PATCH v2] vhost: fix vring message handling broken in some case

[dpdk-dev] [PATCH v2] vhost: fix vring message handling broken in some case

[Zhike Wang]

When VHOST_USER_VRING_NOFD_MASK is set, the fd_num is 0,
so validate_msg_fds() will return error. In this case,
the negotiation of vring message between vhost user front end and
back end would fail, and as a result, vhost user link could NOT be up.

How to reproduce:
1.Run dpdk testpmd insides VM, which locates at host with ovs+dpdk.
2.Notice that inside ovs there are endless logs regarding failure to
handle VHOST_USER_SET_VRING_CALL, and link of vm could NOT be up.

Fixes: bf47225 ("vhost: fix possible denial of service by leaking FDs")
Signed-off-by: Zhike Wang <wangzk320@163.com>
---
 lib/librte_vhost/vhost_user.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 90ecee1..0cfb8b7 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1563,8 +1563,10 @@
 	struct virtio_net *dev = *pdev;
 	struct vhost_vring_file file;
 	struct vhost_virtqueue *vq;
+	int expected_fds;
 
-	if (validate_msg_fds(msg, 1) != 0)
+	expected_fds = (msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK) ? 0 : 1;
+	if (validate_msg_fds(msg, expected_fds) != 0)
 		return RTE_VHOST_MSG_RESULT_ERR;
 
 	file.index = msg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
@@ -1588,7 +1590,10 @@ static int vhost_user_set_vring_err(struct virtio_net **pdev __rte_unused,
 			struct VhostUserMsg *msg,
 			int main_fd __rte_unused)
 {
-	if (validate_msg_fds(msg, 1) != 0)
+	int expected_fds;
+
+	expected_fds = (msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK) ? 0 : 1;
+	if (validate_msg_fds(msg, expected_fds) != 0)
 		return RTE_VHOST_MSG_RESULT_ERR;
 
 	if (!(msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK))
@@ -1790,8 +1795,10 @@ static int vhost_user_set_vring_err(struct virtio_net **pdev __rte_unused,
 	struct virtio_net *dev = *pdev;
 	struct vhost_vring_file file;
 	struct vhost_virtqueue *vq;
+	int expected_fds;
 
-	if (validate_msg_fds(msg, 1) != 0)
+	expected_fds = (msg->payload.u64 & VHOST_USER_VRING_NOFD_MASK) ? 0 : 1;
+	if (validate_msg_fds(msg, expected_fds) != 0)
 		return RTE_VHOST_MSG_RESULT_ERR;
 
 	file.index = msg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] vhost: fix vring message handling broken in some case

[Maxime Coquelin]

Maybe change the title to something like:

vhost: fix vring requests validation broken if no FD in payload

On 11/13/19 12:03 PM, Zhike Wang wrote:
> When VHOST_USER_VRING_NOFD_MASK is set, the fd_num is 0,
> so validate_msg_fds() will return error. In this case,
> the negotiation of vring message between vhost user front end and
> back end would fail, and as a result, vhost user link could NOT be up.
> 
> How to reproduce:
> 1.Run dpdk testpmd insides VM, which locates at host with ovs+dpdk.
> 2.Notice that inside ovs there are endless logs regarding failure to
> handle VHOST_USER_SET_VRING_CALL, and link of vm could NOT be up.
> 
> Fixes: bf47225 ("vhost: fix possible denial of service by leaking FDs")
> Signed-off-by: Zhike Wang <wangzk320@163.com>
> ---
>  lib/librte_vhost/vhost_user.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 

otherwise:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

The title can be changed while applying if you agree with it, no need to
send v3.

I'll take care of the different backports.

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v2] vhost: fix vring message handling broken in some case

[王志克]

Thanks Maxime. 
Go ahead, your proposal sounds good.

Br,
Zhike Wang 
JDCloud, Product Development, IaaS   
------------------------------------------------------------------------------------------------
Mobile／+86 13466719566
E- mail／wangzhike@jd.com
Address／5F Building A,North-Star Century Center,8 Beichen West Street,Chaoyang District Beijing
Https://JDCloud.com
------------------------------------------------------------------------------------------------



-----Original Message-----
From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] 
Sent: Thursday, November 14, 2019 4:53 PM
To: Zhike Wang; dev@dpdk.org; stable@dpdk.org; security@dpdk.org
Cc: 王志克
Subject: Re: [dpdk-dev] [PATCH v2] vhost: fix vring message handling broken in some case

Maybe change the title to something like:

vhost: fix vring requests validation broken if no FD in payload

On 11/13/19 12:03 PM, Zhike Wang wrote:
> When VHOST_USER_VRING_NOFD_MASK is set, the fd_num is 0,
> so validate_msg_fds() will return error. In this case,
> the negotiation of vring message between vhost user front end and
> back end would fail, and as a result, vhost user link could NOT be up.
> 
> How to reproduce:
> 1.Run dpdk testpmd insides VM, which locates at host with ovs+dpdk.
> 2.Notice that inside ovs there are endless logs regarding failure to
> handle VHOST_USER_SET_VRING_CALL, and link of vm could NOT be up.
> 
> Fixes: bf47225 ("vhost: fix possible denial of service by leaking FDs")
> Signed-off-by: Zhike Wang <wangzk320@163.com>
> ---
>  lib/librte_vhost/vhost_user.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 

otherwise:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

The title can be changed while applying if you agree with it, no need to
send v3.

I'll take care of the different backports.

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v2] vhost: fix vring message handling broken in some case

[Maxime Coquelin]

On 11/14/19 11:16 AM, 王志克 wrote:
> Thanks Maxime. 
> Go ahead, your proposal sounds good.


Thanks, could you provide the qemu command line you used to reproduce
the issue, and also the kernel version?

It will help to document the commit message.

Regards,
Maxime

> Br,
> Zhike Wang 
> JDCloud, Product Development, IaaS   
> ------------------------------------------------------------------------------------------------
> Mobile／+86 13466719566
> E- mail／wangzhike@jd.com
> Address／5F Building A,North-Star Century Center,8 Beichen West Street,Chaoyang District Beijing
> Https://JDCloud.com
> ------------------------------------------------------------------------------------------------
> 
> 
> 
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] 
> Sent: Thursday, November 14, 2019 4:53 PM
> To: Zhike Wang; dev@dpdk.org; stable@dpdk.org; security@dpdk.org
> Cc: 王志克
> Subject: Re: [dpdk-dev] [PATCH v2] vhost: fix vring message handling broken in some case
> 
> Maybe change the title to something like:
> 
> vhost: fix vring requests validation broken if no FD in payload
> 
> On 11/13/19 12:03 PM, Zhike Wang wrote:
>> When VHOST_USER_VRING_NOFD_MASK is set, the fd_num is 0,
>> so validate_msg_fds() will return error. In this case,
>> the negotiation of vring message between vhost user front end and
>> back end would fail, and as a result, vhost user link could NOT be up.
>>
>> How to reproduce:
>> 1.Run dpdk testpmd insides VM, which locates at host with ovs+dpdk.
>> 2.Notice that inside ovs there are endless logs regarding failure to
>> handle VHOST_USER_SET_VRING_CALL, and link of vm could NOT be up.
>>
>> Fixes: bf47225 ("vhost: fix possible denial of service by leaking FDs")
>> Signed-off-by: Zhike Wang <wangzk320@163.com>
>> ---
>>  lib/librte_vhost/vhost_user.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
> 
> otherwise:
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> The title can be changed while applying if you agree with it, no need to
> send v3.
> 
> I'll take care of the different backports.
> 
> Thanks,
> Maxime
> 

Re: [dpdk-dev] [PATCH v2] vhost: fix vring message handling broken in some case

[王志克]

Hi Maxime,

Usually I define some xml file, and use virsh to start the VM. Below is got from ps command.

/usr/libexec/qemu-kvm -name guest=test_virtio,debug-threads=on -S -object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-4-test_virtio/master-key.aes -machine pc-i440fx-rhel7.0.0,accel=kvm,usb=off,dump-guest-core=off -cpu host -m 8192 -realtime mlock=off -smp 9,sockets=1,cores=9,threads=1 -object memory-backend-file,id=ram-node0,prealloc=yes,mem-path=/mnt/huge_1GB/libvirt/qemu/4-test_virtio,share=yes,size=8589934592,host-nodes=0,policy=preferred -numa node,nodeid=0,cpus=0,memdev=ram-node0 -uuid 311a9da4-0f02-4e9a-b558-7b2123794639 -display none -no-user-config -nodefaults -chardev socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-4-test_virtio/monitor.sock,server,nowait -mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew -global kvm-pit.lost_tick_policy=delay -no-hpet -no-reboot -global PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot strict=on -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x4.0x7 -device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4 -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x4.0x1 -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x4.0x2 -device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive file=/export/wangzhike/image/centos74.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 -device virtio-blk-pci,scsi=off,bus=pci.0,addr=0x6,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=2 -netdev tap,fd=26,id=hostnet0,vhost=on,vhostfd=28 -device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:92:db:8c,bus=pci.0,addr=0x8 -chardev socket,id=charnet1,path=/var/lib/openvswitch/port-vhost0,server -netdev vhost-user,chardev=charnet1,queues=4,id=hostnet1 -device virtio-net-pci,mrg_rxbuf=on,mq=on,vectors=10,rx_queue_size=1024,netdev=hostnet1,id=net1,mac=e4:22:22:33:44:01,bus=pci.0,multifunction=on,addr=0x9 -chardev socket,id=charnet2,path=/var/lib/openvswitch/port-vhost1,server -netdev vhost-user,chardev=charnet2,queues=4,id=hostnet2 -device virtio-net-pci,mrg_rxbuf=on,mq=on,vectors=10,rx_queue_size=1024,netdev=hostnet2,id=net2,mac=e4:22:22:33:44:02,bus=pci.0,addr=0x9.0x1 -chardev socket,id=charnet3,path=/var/lib/openvswitch/port-vhost2,server -netdev vhost-user,chardev=charnet3,queues=4,id=hostnet3 -device virtio-net-pci,mrg_rxbuf=on,mq=on,vectors=10,rx_queue_size=1024,netdev=hostnet3,id=net3,mac=e4:22:22:33:44:03,bus=pci.0,addr=0x9.0x2 -chardev socket,id=charnet4,path=/var/lib/openvswitch/port-vhost3,server -netdev vhost-user,chardev=charnet4,queues=4,id=hostnet4 -device virtio-net-pci,mrg_rxbuf=on,mq=on,vectors=10,rx_queue_size=1024,netdev=hostnet4,id=net4,mac=e4:22:22:33:44:04,bus=pci.0,addr=0x9.0x3 -chardev socket,id=charnet5,path=/var/lib/openvswitch/port-vhost4,server -netdev vhost-user,chardev=charnet5,queues=4,id=hostnet5 -device virtio-net-pci,mrg_rxbuf=on,mq=on,vectors=10,rx_queue_size=1024,netdev=hostnet5,id=net5,mac=e4:22:22:33:44:05,bus=pci.0,addr=0x9.0x4 -chardev socket,id=charnet6,path=/var/lib/openvswitch/port-vhost5,server -netdev vhost-user,chardev=charnet6,queues=4,id=hostnet6 -device virtio-net-pci,mrg_rxbuf=on,mq=on,vectors=10,rx_queue_size=1024,netdev=hostnet6,id=net6,mac=e4:22:22:33:44:06,bus=pci.0,addr=0x9.0x5 -chardev socket,id=charnet7,path=/var/lib/openvswitch/port-vhost6,server -netdev vhost-user,chardev=charnet7,queues=4,id=hostnet7 -device virtio-net-pci,mrg_rxbuf=on,mq=on,vectors=10,rx_queue_size=1024,netdev=hostnet7,id=net7,mac=e4:22:22:33:44:07,bus=pci.0,addr=0x9.0x6 -chardev pty,id=charserial0 -device isa-serial,chardev=charserial0,id=serial0 -chardev socket,id=charserial1,host=127.0.0.1,port=2445,telnet,server,nowait -device isa-serial,chardev=charserial1,id=serial1 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7 -msg timestamp=on

Both host and VM use CentOS7.4 with kernel 3.10.0-693.el7.x86_64. 
Testpmd insides VM uses DPDK18.11.

Br,
Zhike Wang 
JDCloud, Product Development, IaaS   
------------------------------------------------------------------------------------------------
Mobile／+86 13466719566
E- mail／wangzhike@jd.com
Address／5F Building A,North-Star Century Center,8 Beichen West Street,Chaoyang District Beijing
Https://JDCloud.com
------------------------------------------------------------------------------------------------


-----Original Message-----
From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] 
Sent: Thursday, November 14, 2019 6:24 PM
To: 王志克; Zhike Wang; dev@dpdk.org; stable@dpdk.org; security@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2] vhost: fix vring message handling broken in some case



On 11/14/19 11:16 AM, 王志克 wrote:
> Thanks Maxime. 
> Go ahead, your proposal sounds good.


Thanks, could you provide the qemu command line you used to reproduce
the issue, and also the kernel version?

It will help to document the commit message.

Regards,
Maxime

> Br,
> Zhike Wang 
> JDCloud, Product Development, IaaS   
> ------------------------------------------------------------------------------------------------
> Mobile／+86 13466719566
> E- mail／wangzhike@jd.com
> Address／5F Building A,North-Star Century Center,8 Beichen West Street,Chaoyang District Beijing
> Https://JDCloud.com
> ------------------------------------------------------------------------------------------------
> 
> 
> 
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] 
> Sent: Thursday, November 14, 2019 4:53 PM
> To: Zhike Wang; dev@dpdk.org; stable@dpdk.org; security@dpdk.org
> Cc: 王志克
> Subject: Re: [dpdk-dev] [PATCH v2] vhost: fix vring message handling broken in some case
> 
> Maybe change the title to something like:
> 
> vhost: fix vring requests validation broken if no FD in payload
> 
> On 11/13/19 12:03 PM, Zhike Wang wrote:
>> When VHOST_USER_VRING_NOFD_MASK is set, the fd_num is 0,
>> so validate_msg_fds() will return error. In this case,
>> the negotiation of vring message between vhost user front end and
>> back end would fail, and as a result, vhost user link could NOT be up.
>>
>> How to reproduce:
>> 1.Run dpdk testpmd insides VM, which locates at host with ovs+dpdk.
>> 2.Notice that inside ovs there are endless logs regarding failure to
>> handle VHOST_USER_SET_VRING_CALL, and link of vm could NOT be up.
>>
>> Fixes: bf47225 ("vhost: fix possible denial of service by leaking FDs")
>> Signed-off-by: Zhike Wang <wangzk320@163.com>
>> ---
>>  lib/librte_vhost/vhost_user.c | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
> 
> otherwise:
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> The title can be changed while applying if you agree with it, no need to
> send v3.
> 
> I'll take care of the different backports.
> 
> Thanks,
> Maxime
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] vhost: fix vring message handling broken in some case

[Tiwei Bie]

On Thu, Nov 14, 2019 at 09:52:30AM +0100, Maxime Coquelin wrote:
> Maybe change the title to something like:
> 
> vhost: fix vring requests validation broken if no FD in payload
> 
> On 11/13/19 12:03 PM, Zhike Wang wrote:
> > When VHOST_USER_VRING_NOFD_MASK is set, the fd_num is 0,
> > so validate_msg_fds() will return error. In this case,
> > the negotiation of vring message between vhost user front end and
> > back end would fail, and as a result, vhost user link could NOT be up.
> > 
> > How to reproduce:
> > 1.Run dpdk testpmd insides VM, which locates at host with ovs+dpdk.
> > 2.Notice that inside ovs there are endless logs regarding failure to
> > handle VHOST_USER_SET_VRING_CALL, and link of vm could NOT be up.
> > 
> > Fixes: bf47225 ("vhost: fix possible denial of service by leaking FDs")
> > Signed-off-by: Zhike Wang <wangzk320@163.com>
> > ---
> >  lib/librte_vhost/vhost_user.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> 
> otherwise:
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> The title can be changed while applying if you agree with it, no need to
> send v3.
> 
> I'll take care of the different backports.
> 

Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

Thanks!
Tiwei

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] vhost: fix vring message handling broken in some case

[David Marchand]

On Fri, Nov 15, 2019 at 5:14 AM Tiwei Bie <tiwei.bie@intel.com> wrote:
>
> On Thu, Nov 14, 2019 at 09:52:30AM +0100, Maxime Coquelin wrote:
> > Maybe change the title to something like:
> >
> > vhost: fix vring requests validation broken if no FD in payload

For the title, aligning to backport patches:
"vhost: fix vring requests validation broken if no FD"

> >
> > On 11/13/19 12:03 PM, Zhike Wang wrote:
> > > When VHOST_USER_VRING_NOFD_MASK is set, the fd_num is 0,
> > > so validate_msg_fds() will return error. In this case,
> > > the negotiation of vring message between vhost user front end and
> > > back end would fail, and as a result, vhost user link could NOT be up.
> > >
> > > How to reproduce:
> > > 1.Run dpdk testpmd insides VM, which locates at host with ovs+dpdk.
> > > 2.Notice that inside ovs there are endless logs regarding failure to
> > > handle VHOST_USER_SET_VRING_CALL, and link of vm could NOT be up.
> > >
> > > Fixes: bf47225 ("vhost: fix possible denial of service by leaking FDs")

Fixes: bf472259dde6 ("vhost: fix possible denial of service by leaking FDs")
Cc: stable@dpdk.org


> > > Signed-off-by: Zhike Wang <wangzk320@163.com>
> > > ---
> > >  lib/librte_vhost/vhost_user.c | 13 ++++++++++---
> > >  1 file changed, 10 insertions(+), 3 deletions(-)
> > >
> >
> > otherwise:
> >
> > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> > The title can be changed while applying if you agree with it, no need to
> > send v3.
> >
> > I'll take care of the different backports.
> >
>
> Reviewed-by: Tiwei Bie <tiwei.bie@intel.com>

Applied, thanks.


-- 
David Marchand