Re: [dpdk-dev] [PATCH 1/2] ethdev: fix data room size verification in Rx queue setup

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 6/19/20 6:42 AM, Wei Hu (Xavier) wrote:
> From: Chengchang Tang <tangchengchang@huawei.com>
> 
> In the rte_eth_rx_queue_setup API function, the local variable named
> mbp_buf_size, which is the data room size of the input parameter mp,
> is checked to guarantee that each memory chunck used for net device
> in the mbuf is bigger than the min_rx_bufsize. But if mbp_buf_size is
> less than RTE_PKTMBUF_HEADROOM, the value of the following  statement
> will be a large number since the mbp_buf_size is a unsigned value.
>   mbp_buf_size - RTE_PKTMBUF_HEADROOM
> As a result, it will cause a segment fault in this situation.
> 
> This patch fixes it by adding a check condition to guarantee that the
> local varibale named mbp_buf_size is bigger than RTE_PKTMBUF_HEADROOM.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 8e10a6f..122fd2a 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1822,7 +1822,8 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>  	}
>  	mbp_buf_size = rte_pktmbuf_data_room_size(mp);
>  
> -	if ((mbp_buf_size - RTE_PKTMBUF_HEADROOM) < dev_info.min_rx_bufsize) {
> +	if (mbp_buf_size < RTE_PKTMBUF_HEADROOM ||
> +	    (mbp_buf_size - RTE_PKTMBUF_HEADROOM) < dev_info.min_rx_bufsize) {

May be it could be shorten to
mbp_buf_size < dev_info.min_rx_bufsize + RTE_PKTMBUF_HEADROOM

However, way suggested in the patch is 100% safe against
integer overflow.

>  		RTE_ETHDEV_LOG(ERR,
>  			"%s mbuf_data_room_size %d < %d (RTE_PKTMBUF_HEADROOM=%d + min_rx_bufsize(dev)=%d)\n",
>  			mp->name, (int)mbp_buf_size,
> 

Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>