Re: [dpdk-dev] [PATCH] virtio: fix segfault when transmit pkts

["Tan, Jianfeng" <jianfeng.tan@intel.com>]

Hi Stephen,

On 4/26/2016 12:48 PM, Stephen Hemminger wrote:
> On Thu, 21 Apr 2016 12:36:10 +0000
> Jianfeng Tan <jianfeng.tan@intel.com> wrote:
>
>> Issue: when using virtio nic to transmit pkts, it causes segment fault.
>>
>> How to reproduce:
>> a. start testpmd with vhost.
>> $testpmd -c 0x3 -n 4 --socket-mem 1024,0 --no-pci \
>>    --vdev 'eth_vhost0,iface=/tmp/sock0,queues=1' -- -i --nb-cores=1
>> b. start a qemu with a virtio nic connected with the vhost-user port.
>> $qemu -smp cores=2,sockets=1 -cpu host -enable-kvm vm-0.img -vnc :5 -m 4G \
>>    -object memory-backend-file,id=mem,size=4096M,mem-path=<path>,share=on \
>>    -numa node,memdev=mem -mem-prealloc \
>>    -chardev socket,id=char1,path=$sock_vhost \
>>    -netdev type=vhost-user,id=net1,chardev=char1 \
>>    -device virtio-net-pci,netdev=net1,mac=00:01:02:03:04:05
>> c. enable testpmd on the host.
>> testpmd> set fwd io
>> testpmd> start
>> d. start testpmd in VM.
>> $testpmd -c 0x3 -n 4 -m 1024 -- -i --disable-hw-vlan-filter --txqflags=0xf01
>> testpmd> set fwd txonly
>> testpmd> start
>>
>> How to fix: this bug is because inside virtqueue_enqueue_xmit(), the flag of
>> desc has been updated inside the do {} while (); and after the loop, all descs
>> could have run out, so idx is VQ_RING_DESC_CHAIN_END (32768), use this idx to
>> reference the start_dp array will lead to segment fault.
>>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>> ---
>>   drivers/net/virtio/virtio_rxtx.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>> index ef21d8e..432aeab 100644
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -271,8 +271,6 @@ virtqueue_enqueue_xmit(struct virtqueue *txvq, struct rte_mbuf *cookie,
>>   		idx = start_dp[idx].next;
>>   	} while ((cookie = cookie->next) != NULL);
>>   
>> -	start_dp[idx].flags &= ~VRING_DESC_F_NEXT;
>> -
>>   	if (use_indirect)
>>   		idx = txvq->vq_ring.desc[head_idx].next;
>>   
> At this point in the code idx is the index past the current set of ring
> descriptors. So yes this is a real bug.
>
> I think the description meta-data needs work to explain it better.
>
>
Yes, please see v2. Yuanhan gives a hand to refine it already.

Thanks,
Jianfeng