RE: [PATCH v2] mbuf: fix read packet data

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> The sum of offset + length, both 32 bit unsigned integers, could wrap
> around, causing comparisons to give the wrong result.
> This was fixed by using 64 bit instead of 32 bit for calculating the sum.
> 
> Note:
> When the branch is not taken for the initial "if ((uint64_t)off + len >
> rte_pktmbuf_pkt_len(m))" comparison, the sum is known to not exceed the
> maximum possible value of rte_pktmbuf_pkt_len(m), UINT32_MAX, and
> following sum calculations can proceed using 32 bit.
> 
> Also, fixed a related bug in an mbuf test case:
> It expected reading a length of UINT_MAX from a non-zero offset to not
> fail.
> And due to the offset+length wraparound bug, the read operation did not
> fail.
> This test case was updated to expect the read operation to fail.
> 
> Fixes: b84110e7baa2 ("mbuf: add function to read packet data")
> Fixes: 7b295dceea07 ("test/mbuf: add unit test cases")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
> v2:
> * Fixed mbuf test case. (Marat Khalili)
> ---
>  app/test/test_mbuf.c | 18 +++++++++---------
>  lib/mbuf/rte_mbuf.c  |  2 +-
>  lib/mbuf/rte_mbuf.h  |  2 +-
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/app/test/test_mbuf.c b/app/test/test_mbuf.c
> index a41d2d0f97..db23259745 100644
> --- a/app/test/test_mbuf.c
> +++ b/app/test/test_mbuf.c
> @@ -2037,13 +2037,13 @@ test_pktmbuf_read_from_offset(struct
> rte_mempool *pktmbuf_pool)
>  	/* read length greater than mbuf data_len */
>  	if (rte_pktmbuf_read(m, hdr_len, rte_pktmbuf_data_len(m) + 1,
>  				NULL) != NULL)
> -		GOTO_FAIL("%s: Requested len is larger than mbuf data len!\n",
> +		GOTO_FAIL("%s: Requested offset + len is larger than mbuf data
> len!\n",
>  				__func__);
> 
>  	/* read length greater than mbuf pkt_len */
>  	if (rte_pktmbuf_read(m, hdr_len, rte_pktmbuf_pkt_len(m) + 1,
>  				NULL) != NULL)
> -		GOTO_FAIL("%s: Requested len is larger than mbuf pkt len!\n",
> +		GOTO_FAIL("%s: Requested offset + len is larger than mbuf pkt
> len!\n",
>  				__func__);
> 
>  	/* read data of zero len from valid offset */
> @@ -2065,21 +2065,21 @@ test_pktmbuf_read_from_offset(struct
> rte_mempool *pktmbuf_pool)
> 
>  	/* read data of max length from valid offset */
>  	data_copy = rte_pktmbuf_read(m, hdr_len, UINT_MAX, NULL);
> -	if (data_copy == NULL)
> -		GOTO_FAIL("%s: Error in reading packet data!\n", __func__);
> -	/* check if the received address is the beginning of data segment */
> -	if (data_copy != data)
> -		GOTO_FAIL("%s: Corrupted data address!\n", __func__);
> +	if (data_copy != NULL)
> +		GOTO_FAIL("%s: Requested offset + max len is larger than mbuf
> pkt len!\n",
> +				__func__);
> 
>  	/* try to read from mbuf with max size offset */
>  	data_copy = rte_pktmbuf_read(m, UINT_MAX, 0, NULL);
>  	if (data_copy != NULL)
> -		GOTO_FAIL("%s: Error in reading packet data!\n", __func__);
> +		GOTO_FAIL("%s: Requested max offset is larger than mbuf pkt
> len!\n",
> +				__func__);
> 
>  	/* try to read from mbuf with max size offset and len */
>  	data_copy = rte_pktmbuf_read(m, UINT_MAX, UINT_MAX, NULL);
>  	if (data_copy != NULL)
> -		GOTO_FAIL("%s: Error in reading packet data!\n", __func__);
> +		GOTO_FAIL("%s: Requested max offset + max len is larger than
> mbuf pkt len!\n",
> +				__func__);
> 
>  	rte_pktmbuf_dump(stdout, m, rte_pktmbuf_pkt_len(m));
> 
> diff --git a/lib/mbuf/rte_mbuf.c b/lib/mbuf/rte_mbuf.c
> index a5d16e4c97..c2476e7704 100644
> --- a/lib/mbuf/rte_mbuf.c
> +++ b/lib/mbuf/rte_mbuf.c
> @@ -795,7 +795,7 @@ const void *__rte_pktmbuf_read(const struct rte_mbuf
> *m, uint32_t off,
>  	const struct rte_mbuf *seg = m;
>  	uint32_t buf_off = 0, copy_len;
> 
> -	if (off + len > rte_pktmbuf_pkt_len(m))
> +	if ((uint64_t)off + len > rte_pktmbuf_pkt_len(m))

Thanks Morten for addressing it.
Just as a nit, for 32-bit systems, wouldn't it be more friendly:
If (off + len < off || off + len > rte_pktmbuf_pkt_len(m))
?
Same for comparison in rte_pktmbuf_read(). 
With or without suggested change:
Acked-by: Konstantin Ananyev <konstantin.ananyev@huawei.com>

>  		return NULL;
> 
>  	while (off >= rte_pktmbuf_data_len(seg)) {
> diff --git a/lib/mbuf/rte_mbuf.h b/lib/mbuf/rte_mbuf.h
> index 592af2388c..d6602f74bc 100644
> --- a/lib/mbuf/rte_mbuf.h
> +++ b/lib/mbuf/rte_mbuf.h
> @@ -1843,7 +1843,7 @@ const void *__rte_pktmbuf_read(const struct rte_mbuf
> *m, uint32_t off,
>  static inline const void *rte_pktmbuf_read(const struct rte_mbuf *m,
>  	uint32_t off, uint32_t len, void *buf)
>  {
> -	if (likely(off + len <= rte_pktmbuf_data_len(m)))
> +	if (likely((uint64_t)off + len <= rte_pktmbuf_data_len(m)))
>  		return rte_pktmbuf_mtod_offset(m, char *, off);
>  	else
>  		return __rte_pktmbuf_read(m, off, len, buf);
> --
> 2.43.0