Re: [PATCH v2] net/memif: fix buffer overflow in zero copy Rx

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 7/8/2024 12:45 PM, Ferruh Yigit wrote:
> On 7/8/2024 4:39 AM, Mihai Brodschi wrote:
>>
>>
>> On 07/07/2024 21:46, Mihai Brodschi wrote:
>>>
>>>
>>> On 07/07/2024 18:18, Mihai Brodschi wrote:
>>>>
>>>>
>>>> On 07/07/2024 17:05, Ferruh Yigit wrote:
>>>>>
>>>>> My expectation is numbers should be like following:
>>>>>
>>>>> Initially:
>>>>>  size = 256
>>>>>  head = 0
>>>>>  tail = 0
>>>>>
>>>>> In first refill:
>>>>>  n_slots = 256
>>>>>  head = 256
>>>>>  tail = 0
>>>>>
>>>>> Subsequent run that 32 slots used:
>>>>>  head = 256
>>>>>  tail = 32
>>>>>  n_slots = 32
>>>>>  rte_pktmbuf_alloc_bulk(mq, buf[head & mask], n_slots);
>>>>>   head & mask = 0
>>>>>   // So it fills first 32 elements of buffer, which is inbound
>>>>>
>>>>> This will continue as above, combination of only gap filled and head
>>>>> masked with 'mask' provides the wrapping required.
>>>>
>>>> If I understand correctly, this works only if eth_memif_rx_zc always processes
>>>> a number of packets which is a power of 2, so that the ring's head always wraps
>>>> around at the end of a refill loop, never in the middle of it.
>>>> Is there any reason this should be the case?
>>>> Maybe the tests don't trigger the crash because this condition holds true for them?
>>>
>>> Here's how to reproduce the crash on DPDK stable 23.11.1, using testpmd:
>>>
>>> Server:
>>> # ./dpdk-testpmd --vdev=net_memif0,id=1,role=server,bsize=1024,rsize=8 --single-file-segments -l2,3 --file-prefix test1 -- -i
>>>
>>> Client:
>>> # ./dpdk-testpmd --vdev=net_memif0,id=1,role=client,bsize=1024,rsize=8,zero-copy=yes --single-file-segments -l4,5 --file-prefix test2 -- -i
>>> testpmd> start
>>>
>>> Server:
>>> testpmd> start tx_first
>>> testpmt> set burst 15
>>>
>>> At this point, the client crashes with a segmentation fault.
>>> Before the burst is set to 15, its default value is 32.
>>> If the receiver processes packets in bursts of size 2^N, the crash does not occur.
>>> Setting the burst size to any power of 2 works, anything else crashes.
>>> After applying this patch, the crashes are completely gone.
>>
>> Sorry, this might not crash with a segmentation fault. To confirm the mempool is
>> corrupted, please compile DPDK with debug=true and the c_args -DRTE_LIBRTE_MEMPOOL_DEBUG.
>> You should see the client panic when changing the burst size to not be a power of 2.
>> This also works on the latest main branch.
>>
> 
> Hi Mihai,
> 
> Right, if the buffer size is not multiple of burst size, issue is valid.
> And as there is a requirement to have buffer size power of two, burst
> should have the same.
> I assume this issue is not caught before because default burst size is 32.
> 
> Can you please share some performance impact of the change, with two
> possible solutions we discussed above?
> 
> Other option is to add this as a limitation to the memif zero copy, but
> this won't be good for usability.
> 
> We can decide based on performance numbers.
> 
> 

Hi Jakup,

Do you have any comment on this?

I think we should either document this as limitation of the driver, or
fix it, and if so need to decide the fix.