DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net: check that seg is valid before dereference
@ 2020-09-28 15:32 Chas Williams
  2020-09-29  3:01 ` wangyunjian
  2020-10-01 10:22 ` [dpdk-dev] [PATCH v2 1/2] " Chas Williams
  0 siblings, 2 replies; 8+ messages in thread
From: Chas Williams @ 2020-09-28 15:32 UTC (permalink / raw)
  To: dev; +Cc: olivier.matz, Chas Williams

If the overall pkt_len and segment lengths are out of agreement,
it is possible for the seg to be NULL after the loop. Add assert
to check this condition in debug builds.

Fixes: c442fed81bb9 ("net: add function to calculate checksum in mbuf")

Signed-off-by: Chas Williams <3chas3@gmail.com>
---
 lib/librte_net/rte_ip.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index fcd1eb342..6b3e4cdda 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -225,6 +225,7 @@ rte_raw_cksum_mbuf(const struct rte_mbuf *m, uint32_t off, uint32_t len,
 			break;
 		off -= seglen;
 	}
+	RTE_ASSERT(seg != NULL);
 	seglen -= off;
 	buf = rte_pktmbuf_mtod_offset(seg, const char *, off);
 	if (seglen >= len) {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net: check that seg is valid before dereference
  2020-09-28 15:32 [dpdk-dev] [PATCH] net: check that seg is valid before dereference Chas Williams
@ 2020-09-29  3:01 ` wangyunjian
  2020-09-29 20:19   ` Chas Williams
  2020-10-01 10:22 ` [dpdk-dev] [PATCH v2 1/2] " Chas Williams
  1 sibling, 1 reply; 8+ messages in thread
From: wangyunjian @ 2020-09-29  3:01 UTC (permalink / raw)
  To: Chas Williams, dev; +Cc: olivier.matz, xudingke

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
> Sent: Monday, September 28, 2020 11:32 PM
> To: dev@dpdk.org
> Cc: olivier.matz@6wind.com; Chas Williams <3chas3@gmail.com>
> Subject: [dpdk-dev] [PATCH] net: check that seg is valid before dereference
> 
> If the overall pkt_len and segment lengths are out of agreement, it is possible
> for the seg to be NULL after the loop. Add assert to check this condition in
> debug builds.
> 
> Fixes: c442fed81bb9 ("net: add function to calculate checksum in mbuf")
> 
> Signed-off-by: Chas Williams <3chas3@gmail.com>
> ---
>  lib/librte_net/rte_ip.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h index
> fcd1eb342..6b3e4cdda 100644
> --- a/lib/librte_net/rte_ip.h
> +++ b/lib/librte_net/rte_ip.h
> @@ -225,6 +225,7 @@ rte_raw_cksum_mbuf(const struct rte_mbuf *m,
> uint32_t off, uint32_t len,
>  			break;
>  		off -= seglen;
>  	}
> +	RTE_ASSERT(seg != NULL);

Is it better to return an error code?

>  	seglen -= off;
>  	buf = rte_pktmbuf_mtod_offset(seg, const char *, off);
>  	if (seglen >= len) {
> --
> 2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] net: check that seg is valid before dereference
  2020-09-29  3:01 ` wangyunjian
@ 2020-09-29 20:19   ` Chas Williams
  0 siblings, 0 replies; 8+ messages in thread
From: Chas Williams @ 2020-09-29 20:19 UTC (permalink / raw)
  To: wangyunjian, dev; +Cc: olivier.matz, xudingke

On 9/28/20 11:01 PM, wangyunjian wrote:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Chas Williams
>> Sent: Monday, September 28, 2020 11:32 PM
>> To: dev@dpdk.org
>> Cc: olivier.matz@6wind.com; Chas Williams <3chas3@gmail.com>
>> Subject: [dpdk-dev] [PATCH] net: check that seg is valid before dereference
>>
>> If the overall pkt_len and segment lengths are out of agreement, it is possible
>> for the seg to be NULL after the loop. Add assert to check this condition in
>> debug builds.
>>
>> Fixes: c442fed81bb9 ("net: add function to calculate checksum in mbuf")
>>
>> Signed-off-by: Chas Williams <3chas3@gmail.com>
>> ---
>>   lib/librte_net/rte_ip.h | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h index
>> fcd1eb342..6b3e4cdda 100644
>> --- a/lib/librte_net/rte_ip.h
>> +++ b/lib/librte_net/rte_ip.h
>> @@ -225,6 +225,7 @@ rte_raw_cksum_mbuf(const struct rte_mbuf *m,
>> uint32_t off, uint32_t len,
>>                      break;
>>              off -= seglen;
>>      }
>> +    RTE_ASSERT(seg != NULL);
>
> Is it better to return an error code?

Maybe. However, to get into this state your mbuf chain is already badly broken.
Personally, I would prefer an immediate failure in the application so
I can track
down the source of this error.

No one is checking the return code now, so returning one wouldn't
really help unless I also fix the callers. Lastly, would everyone
be happy with an extra branch in the virtio RX path?


>
>>      seglen -= off;
>>      buf = rte_pktmbuf_mtod_offset(seg, const char *, off);
>>      if (seglen >= len) {
>> --
>> 2.26.2
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v2 1/2] net: check that seg is valid before dereference
  2020-09-28 15:32 [dpdk-dev] [PATCH] net: check that seg is valid before dereference Chas Williams
  2020-09-29  3:01 ` wangyunjian
@ 2020-10-01 10:22 ` Chas Williams
  2020-10-01 10:22   ` [dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf Chas Williams
  1 sibling, 1 reply; 8+ messages in thread
From: Chas Williams @ 2020-10-01 10:22 UTC (permalink / raw)
  To: dev; +Cc: olivier.matz, Chas Williams

If the overall pkt_len and segment lengths are out of agreement,
it is possible for the seg to be NULL after the loop. Add assert
to check this condition in debug builds. Otherwise, return failure.

Fixes: c442fed81bb9 ("net: add function to calculate checksum in mbuf")

Signed-off-by: Chas Williams <3chas3@gmail.com>
---
 lib/librte_net/rte_ip.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_net/rte_ip.h b/lib/librte_net/rte_ip.h
index fcd1eb342..b2bb5d7b9 100644
--- a/lib/librte_net/rte_ip.h
+++ b/lib/librte_net/rte_ip.h
@@ -225,6 +225,9 @@ rte_raw_cksum_mbuf(const struct rte_mbuf *m, uint32_t off, uint32_t len,
 			break;
 		off -= seglen;
 	}
+	RTE_ASSERT(seg != NULL);
+	if (seg == NULL)
+		return -1;
 	seglen -= off;
 	buf = rte_pktmbuf_mtod_offset(seg, const char *, off);
 	if (seglen >= len) {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf
  2020-10-01 10:22 ` [dpdk-dev] [PATCH v2 1/2] " Chas Williams
@ 2020-10-01 10:22   ` Chas Williams
  2020-10-06 21:15     ` Thomas Monjalon
  2020-10-09  7:14     ` Maxime Coquelin
  0 siblings, 2 replies; 8+ messages in thread
From: Chas Williams @ 2020-10-01 10:22 UTC (permalink / raw)
  To: dev; +Cc: olivier.matz, Chas Williams

rte_raw_cksum_mbuf can fail, so we should check to see if it
has. If so, return with an error.

Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")

Signed-off-by: Chas Williams <3chas3@gmail.com>
---
 drivers/net/virtio/virtio_rxtx.c            | 5 +++--
 drivers/net/virtio/virtio_rxtx_packed_avx.c | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 0ade35215..019187bf9 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -910,9 +910,10 @@ virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
 			 */
 			uint16_t csum = 0, off;
 
-			rte_raw_cksum_mbuf(m, hdr->csum_start,
+			if (rte_raw_cksum_mbuf(m, hdr->csum_start,
 				rte_pktmbuf_pkt_len(m) - hdr->csum_start,
-				&csum);
+				&csum) < 0)
+				return -EINVAL;
 			if (likely(csum != 0xffff))
 				csum = ~csum;
 			off = hdr->csum_offset + hdr->csum_start;
diff --git a/drivers/net/virtio/virtio_rxtx_packed_avx.c b/drivers/net/virtio/virtio_rxtx_packed_avx.c
index 6a8214725..67a55abce 100644
--- a/drivers/net/virtio/virtio_rxtx_packed_avx.c
+++ b/drivers/net/virtio/virtio_rxtx_packed_avx.c
@@ -328,9 +328,10 @@ virtio_vec_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
 			 */
 			uint16_t csum = 0, off;
 
-			rte_raw_cksum_mbuf(m, hdr->csum_start,
+			if (rte_raw_cksum_mbuf(m, hdr->csum_start,
 				rte_pktmbuf_pkt_len(m) - hdr->csum_start,
-				&csum);
+				&csum) < 0)
+				return -1;
 			if (likely(csum != 0xffff))
 				csum = ~csum;
 			off = hdr->csum_offset + hdr->csum_start;
-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf
  2020-10-01 10:22   ` [dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf Chas Williams
@ 2020-10-06 21:15     ` Thomas Monjalon
  2020-10-09  7:14     ` Maxime Coquelin
  1 sibling, 0 replies; 8+ messages in thread
From: Thomas Monjalon @ 2020-10-06 21:15 UTC (permalink / raw)
  To: Chas Williams
  Cc: dev, olivier.matz, Maxime Coquelin, Chenbo Xia, Zhihong Wang

> rte_raw_cksum_mbuf can fail, so we should check to see if it
> has. If so, return with an error.
> 
> Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")

It is an old bug, so it should be backported, right?
Cc: stable@dpdk.org

> Signed-off-by: Chas Williams <3chas3@gmail.com>
> ---
> 
>  drivers/net/virtio/virtio_rxtx.c            | 5 +++--
>  drivers/net/virtio/virtio_rxtx_packed_avx.c | 5 +++--

Please use --cc-cmd devtools/get-maintainer.sh
so the maintainers are aware.



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf
  2020-10-01 10:22   ` [dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf Chas Williams
  2020-10-06 21:15     ` Thomas Monjalon
@ 2020-10-09  7:14     ` Maxime Coquelin
  2020-10-12 21:15       ` Thomas Monjalon
  1 sibling, 1 reply; 8+ messages in thread
From: Maxime Coquelin @ 2020-10-09  7:14 UTC (permalink / raw)
  To: Chas Williams, dev; +Cc: olivier.matz



On 10/1/20 12:22 PM, Chas Williams wrote:
> rte_raw_cksum_mbuf can fail, so we should check to see if it
> has. If so, return with an error.
> 
> Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
> 
> Signed-off-by: Chas Williams <3chas3@gmail.com>
> ---
>  drivers/net/virtio/virtio_rxtx.c            | 5 +++--
>  drivers/net/virtio/virtio_rxtx_packed_avx.c | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf
  2020-10-09  7:14     ` Maxime Coquelin
@ 2020-10-12 21:15       ` Thomas Monjalon
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Monjalon @ 2020-10-12 21:15 UTC (permalink / raw)
  To: Chas Williams; +Cc: dev, olivier.matz, Maxime Coquelin

09/10/2020 09:14, Maxime Coquelin:
> 
> On 10/1/20 12:22 PM, Chas Williams wrote:
> > rte_raw_cksum_mbuf can fail, so we should check to see if it
> > has. If so, return with an error.
> > 
> > Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
> > 
> > Signed-off-by: Chas Williams <3chas3@gmail.com>
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Series applied, thanks



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-10-12 21:15 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-28 15:32 [dpdk-dev] [PATCH] net: check that seg is valid before dereference Chas Williams
2020-09-29  3:01 ` wangyunjian
2020-09-29 20:19   ` Chas Williams
2020-10-01 10:22 ` [dpdk-dev] [PATCH v2 1/2] " Chas Williams
2020-10-01 10:22   ` [dpdk-dev] [PATCH v2 2/2] net/virtio: check return from rte_raw_cksum_mbuf Chas Williams
2020-10-06 21:15     ` Thomas Monjalon
2020-10-09  7:14     ` Maxime Coquelin
2020-10-12 21:15       ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).