Re: [dpdk-dev] [PATCH v3] kni: fix possible kernel crash with va2pa

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

On 7/10/2019 9:09 PM, Ferruh Yigit wrote:
> On 6/25/2019 4:04 PM, Yangchao Zhou wrote:
>> va2pa depends on the physical address and virtual address offset of
>> current mbuf. It may get the wrong physical address of next mbuf which
>> allocated in another hugepage segment.
>>
>> In rte_mempool_populate_default(), trying to allocate whole block of
>> contiguous memory could be failed. Then, it would reserve memory in
>> several memzones that have different physical address and virtual address
>> offsets. The rte_mempool_populate_default() is used by
>> rte_pktmbuf_pool_create().
>>
>> Fixes: 8451269e6d7b ("kni: remove continuous memory restriction")
>>
>> Signed-off-by: Yangchao Zhou <zhouyates@gmail.com>
> 
> Overall looks good to me, not from this patch but can you please check below
> comment too.
> Also there is a comment from Junxiao, lets clear it before the ack.
> 

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

> <...>
> 
>> @@ -396,7 +401,7 @@ kni_net_rx_lo_fifo(struct kni_dev *kni)
>>  	uint32_t ret;
>>  	uint32_t len;
>>  	uint32_t i, num, num_rq, num_tq, num_aq, num_fq;
>> -	struct rte_kni_mbuf *kva;
>> +	struct rte_kni_mbuf *kva, *next_kva;
>>  	void *data_kva;
>>  	struct rte_kni_mbuf *alloc_kva;
>>  	void *alloc_data_kva;
>> @@ -439,6 +444,13 @@ kni_net_rx_lo_fifo(struct kni_dev *kni)
>>  			data_kva = kva2data_kva(kva);
>>  			kni->va[i] = pa2va(kni->pa[i], kva);
>>  
>> +			while (kva->next) {
>> +				next_kva = pa2kva(kva->next);
>> +				/* Convert physical address to virtual address */
>> +				kva->next = pa2va(kva->next, next_kva);
>> +				kva = next_kva;
>> +			}
> 
> Not done in this patch, but in 'kni_net_rx_lo_fifo()' the len calculated as
> 'len = kva->pkt_len;'
> 
> But while copying 'data' to 'alloc_data' the segmentation is not taken into
> account and 'len' is used:
> memcpy(alloc_data_kva, data_kva, len);
> 
> This may lead overflow 'alloc_data_kva' for some 'pkt_len' values.
> 

I will send separate patch for this.