[PATCH] examples/l3fwd: fix check of unchecked IP checksum

[PATCH] examples/l3fwd: fix check of unchecked IP checksum

[Morten Brørup]

If the PMD didn't check the IP header checksum, the
offload value is RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN,
not RTE_MBUF_F_RX_IP_CKSUM_NONE.

Fixes: 4b01cabfb09b ("examples/l3fwd: add option to relax Rx offload")
Cc: stable@dpdk.org

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 examples/l3fwd/l3fwd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
index 349fc37c79..331d17602d 100644
--- a/examples/l3fwd/l3fwd.h
+++ b/examples/l3fwd/l3fwd.h
@@ -171,7 +171,7 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags
 
 	/* 2. The IP checksum must be correct. */
 	/* if this is not checked in H/W, check it. */
-	if ((ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == RTE_MBUF_F_RX_IP_CKSUM_NONE) {
+	if ((ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN) {
 		uint16_t actual_cksum, expected_cksum;
 		actual_cksum = pkt->hdr_checksum;
 		pkt->hdr_checksum = 0;
-- 
2.43.0

RE: [PATCH] examples/l3fwd: fix check of unchecked IP checksum

[Morten Brørup]

+TO: Maxime, Harrison (info might be related to my question about virtio offload)

> From: Morten Brørup [mailto:mb@smartsharesystems.com]
> Sent: Tuesday, 15 September 2026 12.58
> 
> If the PMD didn't check the IP header checksum, the
> offload value is RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN,
> not RTE_MBUF_F_RX_IP_CKSUM_NONE.
> 
> Fixes: 4b01cabfb09b ("examples/l3fwd: add option to relax Rx offload")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  examples/l3fwd/l3fwd.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
> index 349fc37c79..331d17602d 100644
> --- a/examples/l3fwd/l3fwd.h
> +++ b/examples/l3fwd/l3fwd.h
> @@ -171,7 +171,7 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt,
> uint32_t link_len, uint64_t ol_flags
> 
>  	/* 2. The IP checksum must be correct. */
>  	/* if this is not checked in H/W, check it. */
> -	if ((ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) ==
> RTE_MBUF_F_RX_IP_CKSUM_NONE) {
> +	if ((ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) ==
> RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN) {
>  		uint16_t actual_cksum, expected_cksum;
>  		actual_cksum = pkt->hdr_checksum;
>  		pkt->hdr_checksum = 0;
> --
> 2.43.0

Simple AI from CI says:
> **Incorrect fix for the stated problem**
> 
> The patch description states "If the PMD didn't check the IP header
> checksum, the offload value is RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN, not
> RTE_MBUF_F_RX_IP_CKSUM_NONE."
> 
> However, according to the mbuf documentation:
> - `RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN` (value 0) means the PMD reports no
> checksum information at all
> - `RTE_MBUF_F_RX_IP_CKSUM_NONE` means checksum is not offloaded but can
> be software calculated
> - `RTE_MBUF_F_RX_IP_CKSUM_BAD` means hardware reported checksum is bad
> - `RTE_MBUF_F_RX_IP_CKSUM_GOOD` means hardware verified checksum is
> good
> 
> The original code checking for `RTE_MBUF_F_RX_IP_CKSUM_NONE` is
> semantically more correct for "software must verify the checksum" than
> `RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN`. When the value is `UNKNOWN`, the PMD
> provides no information about whether it checked or not, so software
> verification is prudent. But when it's explicitly `NONE`, software
> checksum is definitely required.

That is *not* how I interpret the enum values.
The documentation says [1]:
- RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN: no information about the RX IP checksum
- RTE_MBUF_F_RX_IP_CKSUM_BAD: the IP checksum in the packet is wrong
- RTE_MBUF_F_RX_IP_CKSUM_GOOD: the IP checksum in the packet is valid
- RTE_MBUF_F_RX_IP_CKSUM_NONE: the IP checksum is not correct in the packet data, but the integrity of the IP header is verified

I.e. UNKNOWN means that the PMD provided no information about the checksum. (Maybe it did not even look at the checksum.)
The ol_flags field contains UNKNOWN (value 0) when RTE_ETH_RX_OFFLOAD_CHECKSUM was not requested.
And this is exactly what the patch this patch fixes did:
It cleared RTE_ETH_RX_OFFLOAD_CHECKSUM when the PMD didn't offer this capability.

Whereas NONE means that the packet is good, but the checksum field in the packet has not been set by the sender (and may contain garbage).
So it is absolutely wrong to verify the checksum field's value, when the ol_flags enum says that it is unset (and may contain garbage). My patch fixes that.

[1]: https://github.com/DPDK/dpdk/blob/f43632ac7321385233ab6a11f0989aa6223dc61b/lib/mbuf/rte_mbuf_core.h#L74-L79

RE: [PATCH] examples/l3fwd: fix check of unchecked IP checksum

[Morten Brørup]

PING for review.

PS:
This example application bug has propagated into Grout, and a bug report has been registered there [2].

[2]: https://github.com/DPDK/grout/issues/756


Venlig hilsen / Kind regards,
-Morten Brørup


> From: Morten Brørup
> Sent: Tuesday, 15 September 2026 14.40
> 
> +TO: Maxime, Harrison (info might be related to my question about
> virtio offload)
> 
> > From: Morten Brørup [mailto:mb@smartsharesystems.com]
> > Sent: Tuesday, 15 September 2026 12.58
> >
> > If the PMD didn't check the IP header checksum, the
> > offload value is RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN,
> > not RTE_MBUF_F_RX_IP_CKSUM_NONE.
> >
> > Fixes: 4b01cabfb09b ("examples/l3fwd: add option to relax Rx
> offload")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> > ---
> >  examples/l3fwd/l3fwd.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
> > index 349fc37c79..331d17602d 100644
> > --- a/examples/l3fwd/l3fwd.h
> > +++ b/examples/l3fwd/l3fwd.h
> > @@ -171,7 +171,7 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt,
> > uint32_t link_len, uint64_t ol_flags
> >
> >  	/* 2. The IP checksum must be correct. */
> >  	/* if this is not checked in H/W, check it. */
> > -	if ((ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) ==
> > RTE_MBUF_F_RX_IP_CKSUM_NONE) {
> > +	if ((ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) ==
> > RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN) {
> >  		uint16_t actual_cksum, expected_cksum;
> >  		actual_cksum = pkt->hdr_checksum;
> >  		pkt->hdr_checksum = 0;
> > --
> > 2.43.0
> 
> Simple AI from CI says:
> > **Incorrect fix for the stated problem**
> >
> > The patch description states "If the PMD didn't check the IP header
> > checksum, the offload value is RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN, not
> > RTE_MBUF_F_RX_IP_CKSUM_NONE."
> >
> > However, according to the mbuf documentation:
> > - `RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN` (value 0) means the PMD reports no
> > checksum information at all
> > - `RTE_MBUF_F_RX_IP_CKSUM_NONE` means checksum is not offloaded but
> can
> > be software calculated
> > - `RTE_MBUF_F_RX_IP_CKSUM_BAD` means hardware reported checksum is
> bad
> > - `RTE_MBUF_F_RX_IP_CKSUM_GOOD` means hardware verified checksum is
> > good
> >
> > The original code checking for `RTE_MBUF_F_RX_IP_CKSUM_NONE` is
> > semantically more correct for "software must verify the checksum"
> than
> > `RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN`. When the value is `UNKNOWN`, the
> PMD
> > provides no information about whether it checked or not, so software
> > verification is prudent. But when it's explicitly `NONE`, software
> > checksum is definitely required.
> 
> That is *not* how I interpret the enum values.
> The documentation says [1]:
> - RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN: no information about the RX IP
> checksum
> - RTE_MBUF_F_RX_IP_CKSUM_BAD: the IP checksum in the packet is wrong
> - RTE_MBUF_F_RX_IP_CKSUM_GOOD: the IP checksum in the packet is valid
> - RTE_MBUF_F_RX_IP_CKSUM_NONE: the IP checksum is not correct in the
> packet data, but the integrity of the IP header is verified
> 
> I.e. UNKNOWN means that the PMD provided no information about the
> checksum. (Maybe it did not even look at the checksum.)
> The ol_flags field contains UNKNOWN (value 0) when
> RTE_ETH_RX_OFFLOAD_CHECKSUM was not requested.
> And this is exactly what the patch this patch fixes did:
> It cleared RTE_ETH_RX_OFFLOAD_CHECKSUM when the PMD didn't offer this
> capability.
> 
> Whereas NONE means that the packet is good, but the checksum field in
> the packet has not been set by the sender (and may contain garbage).
> So it is absolutely wrong to verify the checksum field's value, when
> the ol_flags enum says that it is unset (and may contain garbage). My
> patch fixes that.
> 
> [1]:
> https://github.com/DPDK/dpdk/blob/f43632ac7321385233ab6a11f0989aa6223dc
> 61b/lib/mbuf/rte_mbuf_core.h#L74-L79

[PATCH v2] examples/l3fwd: fix RFC 1812 checks

[Morten Brørup]

Four bug fixes and an enhancement.

1.
If the PMD didn't check the IP header checksum, the
offload value is RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN,
not RTE_MBUF_F_RX_IP_CKSUM_NONE.
CKSUM_NONE means that the packet integrity is good, but the checksum
field has not been set.
CKSUM_UNKNOWN means that the checksum has not been investigated.
(BTW, no PMD currently sets RTE_MBUF_F_RX_IP_CKSUM_NONE, so this part
of the check was effectively dead code.)

Fixed this by processing all four RTE_MBUF_F_RX_IP_CKSUM enum values.
All four values may not actually occur in this application,
but it serves an educational purpose.

2.
When checking the checksum, the checksum field in the packet was zeroed
for calculating the checksum.
If the checksum was correct, the zero value remained in the packet.

Fixed this by restoring the actual value in the packet after checking.

3.
When checking the Link Layer length of an IP packet, the length did
not omit the length of the preceding Layer 2 headers.

Fixed this by subtracting the Ethernet header length when calling the
IPv4 validation function.

4.
In the 5th bullet check,
"5. The IP total length field must be large enough to hold the IP
datagram header, whose length is specified in the IP header length
field."
the comparison did not use the IP header length field, but the size
of the IP header structure, which did not account for IP header
options.

Fixed this by using the IP header length field instead of the
size of the structure.

5.
Added the additional check recommended in RFC 1812 section 5.2.2,
verifying that the packet length reported by the Link Layer is at least
as large as the IP total length recorded in the packet's IP header.

Fixes: 4b01cabfb09b ("examples/l3fwd: add option to relax Rx offload")
Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v2:
* Rebased.
* Improved (1): Process all four checksum offload enum values, not just
  CKSUM_UNKNOWN instead of CKSUM_NONE.
* Added (2): Restore checksum field value in packet after checking.
* Added (3): Subtract Layer 2 header length when passing Link Layer
  length.
* Added (4): Fix IP header Total Length vs. IP Header Length check.
* Added (5): Link Layer length vs. IP header Total Length check.
* Added the Fixes tag going back to the first public release.
---
 examples/l3fwd/l3fwd.h     | 26 +++++++++++++++++++++++---
 examples/l3fwd/l3fwd_em.h  |  3 ++-
 examples/l3fwd/l3fwd_lpm.h |  3 ++-
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
index 349fc37c79..3bcc7da87a 100644
--- a/examples/l3fwd/l3fwd.h
+++ b/examples/l3fwd/l3fwd.h
@@ -170,14 +170,26 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags
 		return -1;
 
 	/* 2. The IP checksum must be correct. */
-	/* if this is not checked in H/W, check it. */
-	if ((ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == RTE_MBUF_F_RX_IP_CKSUM_NONE) {
+	switch (ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) {
+	case RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN:
+		/* Not checked by H/W; check it. */
 		uint16_t actual_cksum, expected_cksum;
 		actual_cksum = pkt->hdr_checksum;
 		pkt->hdr_checksum = 0;
 		expected_cksum = rte_ipv4_cksum(pkt);
+		pkt->hdr_checksum = actual_cksum;
 		if (actual_cksum != expected_cksum)
 			return -2;
+		break;
+	case RTE_MBUF_F_RX_IP_CKSUM_GOOD:
+		break;
+	case RTE_MBUF_F_RX_IP_CKSUM_BAD:
+		return -2;
+	case RTE_MBUF_F_RX_IP_CKSUM_NONE:
+		/* Packet integrity known good, header field unset; set it. */
+		pkt->hdr_checksum = 0;
+		pkt->hdr_checksum = rte_ipv4_cksum(pkt);
+		break;
 	}
 
 	/*
@@ -199,9 +211,17 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags
 	 * datagram header, whose length is specified in the IP header length
 	 * field.
 	 */
-	if (rte_cpu_to_be_16(pkt->total_length) < sizeof(struct rte_ipv4_hdr))
+	if (rte_cpu_to_be_16(pkt->total_length) < rte_ipv4_hdr_len(pkt))
 		return -5;
 
+	/*
+	 * Additionally, the router SHOULD verify that the packet length
+	 * reported by the Link Layer is at least as large as the IP total
+	 * length recorded in the packet's IP header.
+	 */
+	if (link_len < rte_cpu_to_be_16(pkt->total_length))
+		return -6;
+
 	return 0;
 }
 #endif /* DO_RFC_1812_CHECKS */
diff --git a/examples/l3fwd/l3fwd_em.h b/examples/l3fwd/l3fwd_em.h
index 1fee2e2e6c..34801fba2b 100644
--- a/examples/l3fwd/l3fwd_em.h
+++ b/examples/l3fwd/l3fwd_em.h
@@ -20,7 +20,8 @@ l3fwd_em_handle_ipv4(struct rte_mbuf *m, uint16_t portid,
 
 #ifdef DO_RFC_1812_CHECKS
 	/* Check to make sure the packet is valid (RFC1812) */
-	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
+	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len - sizeof(struct rte_ether_hdr),
+			m->ol_flags) < 0) {
 		rte_pktmbuf_free(m);
 		return BAD_PORT;
 	}
diff --git a/examples/l3fwd/l3fwd_lpm.h b/examples/l3fwd/l3fwd_lpm.h
index 4ee61e8d88..1731ebb2a8 100644
--- a/examples/l3fwd/l3fwd_lpm.h
+++ b/examples/l3fwd/l3fwd_lpm.h
@@ -22,7 +22,8 @@ l3fwd_lpm_simple_forward(struct rte_mbuf *m, uint16_t portid,
 
 #ifdef DO_RFC_1812_CHECKS
 		/* Check to make sure the packet is valid (RFC1812) */
-		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
+		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len - sizeof(struct rte_ether_hdr),
+				m->ol_flags) < 0) {
 			rte_pktmbuf_free(m);
 			return;
 		}
-- 
2.43.0

[PATCH v3] examples/l3fwd: fix RFC 1812 checks

[Morten Brørup]

Four bug fixes and an enhancement.

1.
If the PMD didn't check the IP header checksum, the
offload value is RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN,
not RTE_MBUF_F_RX_IP_CKSUM_NONE.
CKSUM_NONE means that the packet integrity is good, but the checksum
field has not been set.
CKSUM_UNKNOWN means that the checksum has not been investigated.
(BTW, no PMD currently sets RTE_MBUF_F_RX_IP_CKSUM_NONE, so this part
of the check was effectively dead code.)

Fixed this by processing all four RTE_MBUF_F_RX_IP_CKSUM enum values.
All four values may not actually occur in this application,
but it serves an educational purpose.

2.
When checking the checksum, the checksum field in the packet was zeroed
for calculating the checksum.
If the checksum was correct, the zero value remained in the packet.

Fixed this by simply calculating the checksum of the packet without
modifying the checksum field.
If the calculated checksum is zero, the checksum is correct.

3.
When checking the Link Layer length of an IP packet, the length did
not omit the length of the preceding Layer 2 headers.

Fixed this by subtracting the Ethernet header length when calling the
IPv4 validation function.

4.
In the 5th bullet check,
"5. The IP total length field must be large enough to hold the IP
datagram header, whose length is specified in the IP header length
field."
the comparison did not use the IP header length field, but the size
of the IP header structure, which did not account for IP header
options.

Fixed this by using the IP header length field instead of the
size of the structure.

5.
Added the additional check recommended in RFC 1812 section 5.2.2,
verifying that the packet length reported by the Link Layer is at least
as large as the IP total length recorded in the packet's IP header.

Fixes: 4b01cabfb09b ("examples/l3fwd: add option to relax Rx offload")
Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v3:
* Simplified the checksum check. No need to modify the packet for
  checking the checksum.
v2:
* Rebased.
* Improved (1): Process all four checksum offload enum values, not just
  CKSUM_UNKNOWN instead of CKSUM_NONE.
* Added (2): Restore checksum field value in packet after checking.
* Added (3): Subtract Layer 2 header length when passing Link Layer
  length.
* Added (4): Fix IP header Total Length vs. IP Header Length check.
* Added (5): Link Layer length vs. IP header Total Length check.
* Added the Fixes tag going back to the first public release.
---
 examples/l3fwd/l3fwd.h     | 31 +++++++++++++++++++++++--------
 examples/l3fwd/l3fwd_em.h  |  3 ++-
 examples/l3fwd/l3fwd_lpm.h |  3 ++-
 3 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
index 349fc37c79..f8f5cc5b34 100644
--- a/examples/l3fwd/l3fwd.h
+++ b/examples/l3fwd/l3fwd.h
@@ -170,14 +170,21 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags
 		return -1;
 
 	/* 2. The IP checksum must be correct. */
-	/* if this is not checked in H/W, check it. */
-	if ((ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == RTE_MBUF_F_RX_IP_CKSUM_NONE) {
-		uint16_t actual_cksum, expected_cksum;
-		actual_cksum = pkt->hdr_checksum;
-		pkt->hdr_checksum = 0;
-		expected_cksum = rte_ipv4_cksum(pkt);
-		if (actual_cksum != expected_cksum)
+	switch (ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) {
+	case RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN:
+		/* Not checked in H/W; check it. */
+		if (rte_ipv4_cksum(pkt) != 0)
 			return -2;
+		break;
+	case RTE_MBUF_F_RX_IP_CKSUM_GOOD:
+		break;
+	case RTE_MBUF_F_RX_IP_CKSUM_BAD:
+		return -2;
+	case RTE_MBUF_F_RX_IP_CKSUM_NONE:
+		/* Packet integrity known good, header field unset; set it. */
+		pkt->hdr_checksum = 0;
+		pkt->hdr_checksum = rte_ipv4_cksum(pkt);
+		break;
 	}
 
 	/*
@@ -199,9 +206,17 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags
 	 * datagram header, whose length is specified in the IP header length
 	 * field.
 	 */
-	if (rte_cpu_to_be_16(pkt->total_length) < sizeof(struct rte_ipv4_hdr))
+	if (rte_cpu_to_be_16(pkt->total_length) < rte_ipv4_hdr_len(pkt))
 		return -5;
 
+	/*
+	 * Additionally, the router SHOULD verify that the packet length
+	 * reported by the Link Layer is at least as large as the IP total
+	 * length recorded in the packet's IP header.
+	 */
+	if (link_len < rte_cpu_to_be_16(pkt->total_length))
+		return -6;
+
 	return 0;
 }
 #endif /* DO_RFC_1812_CHECKS */
diff --git a/examples/l3fwd/l3fwd_em.h b/examples/l3fwd/l3fwd_em.h
index 1fee2e2e6c..34801fba2b 100644
--- a/examples/l3fwd/l3fwd_em.h
+++ b/examples/l3fwd/l3fwd_em.h
@@ -20,7 +20,8 @@ l3fwd_em_handle_ipv4(struct rte_mbuf *m, uint16_t portid,
 
 #ifdef DO_RFC_1812_CHECKS
 	/* Check to make sure the packet is valid (RFC1812) */
-	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
+	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len - sizeof(struct rte_ether_hdr),
+			m->ol_flags) < 0) {
 		rte_pktmbuf_free(m);
 		return BAD_PORT;
 	}
diff --git a/examples/l3fwd/l3fwd_lpm.h b/examples/l3fwd/l3fwd_lpm.h
index 4ee61e8d88..1731ebb2a8 100644
--- a/examples/l3fwd/l3fwd_lpm.h
+++ b/examples/l3fwd/l3fwd_lpm.h
@@ -22,7 +22,8 @@ l3fwd_lpm_simple_forward(struct rte_mbuf *m, uint16_t portid,
 
 #ifdef DO_RFC_1812_CHECKS
 		/* Check to make sure the packet is valid (RFC1812) */
-		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
+		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len - sizeof(struct rte_ether_hdr),
+				m->ol_flags) < 0) {
 			rte_pktmbuf_free(m);
 			return;
 		}
-- 
2.43.0

[PATCH v4] examples/l3fwd: fix RFC 1812 checks

[Morten Brørup]

Four bug fixes and an enhancement.

1.
If the PMD didn't check the IP header checksum, the
offload value is RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN,
not RTE_MBUF_F_RX_IP_CKSUM_NONE.
CKSUM_NONE means that the packet integrity is good, but the checksum
field has not been set.
CKSUM_UNKNOWN means that the checksum has not been investigated.
(BTW, no PMD currently sets RTE_MBUF_F_RX_IP_CKSUM_NONE, so this part
of the check was effectively dead code.)

Fixed this by processing all four RTE_MBUF_F_RX_IP_CKSUM enum values.
All four values may not actually occur in this application,
but it serves an educational purpose.

2.
When checking the checksum, the checksum field in the packet was zeroed
for calculating the checksum.
If the checksum was correct, the zero value remained in the packet.

Fixed this by simply calculating the checksum of the packet without
modifying the checksum field.
If the calculated checksum is zero, the checksum is correct.

3.
When checking the Link Layer length of an IP packet, the length did
not omit the length of the preceding Layer 2 headers.

Fixed this by subtracting the Ethernet header length when calling the
IPv4 validation function.

4.
In the 5th bullet check,
"5. The IP total length field must be large enough to hold the IP
datagram header, whose length is specified in the IP header length
field."
the comparison did not use the IP header length field, but the size
of the IP header structure, which did not account for IP header
options.

Fixed this by using the IP header length field instead of the
size of the structure.

5.
Added the additional check recommended in RFC 1812 section 5.2.2,
verifying that the packet length reported by the Link Layer is at least
as large as the IP total length recorded in the packet's IP header.

Fixes: 4b01cabfb09b ("examples/l3fwd: add option to relax Rx offload")
Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v4:
* When CKSUM_NONE, don't update checksum field. (AI)
v3:
* Simplified the checksum check. No need to modify the packet for
  checking the checksum.
v2:
* Rebased.
* Improved (1): Process all four checksum offload enum values, not just
  CKSUM_UNKNOWN instead of CKSUM_NONE.
* Added (2): Restore checksum field value in packet after checking.
* Added (3): Subtract Layer 2 header length when passing Link Layer
  length.
* Added (4): Fix IP header Total Length vs. IP Header Length check.
* Added (5): Link Layer length vs. IP header Total Length check.
* Added the Fixes tag going back to the first public release.
---
 examples/l3fwd/l3fwd.h     | 29 +++++++++++++++++++++--------
 examples/l3fwd/l3fwd_em.h  |  3 ++-
 examples/l3fwd/l3fwd_lpm.h |  3 ++-
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
index 349fc37c79..45d9cc8362 100644
--- a/examples/l3fwd/l3fwd.h
+++ b/examples/l3fwd/l3fwd.h
@@ -170,14 +170,19 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags
 		return -1;
 
 	/* 2. The IP checksum must be correct. */
-	/* if this is not checked in H/W, check it. */
-	if ((ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == RTE_MBUF_F_RX_IP_CKSUM_NONE) {
-		uint16_t actual_cksum, expected_cksum;
-		actual_cksum = pkt->hdr_checksum;
-		pkt->hdr_checksum = 0;
-		expected_cksum = rte_ipv4_cksum(pkt);
-		if (actual_cksum != expected_cksum)
+	switch (ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) {
+	case RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN:
+		/* Not checked in H/W; check it. */
+		if (rte_ipv4_cksum(pkt) != 0)
 			return -2;
+		break;
+	case RTE_MBUF_F_RX_IP_CKSUM_GOOD:
+		break;
+	case RTE_MBUF_F_RX_IP_CKSUM_BAD:
+		return -2;
+	case RTE_MBUF_F_RX_IP_CKSUM_NONE:
+		/* Packet integrity known good, checksum field unset; don't check it. */
+		break;
 	}
 
 	/*
@@ -199,9 +204,17 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags
 	 * datagram header, whose length is specified in the IP header length
 	 * field.
 	 */
-	if (rte_cpu_to_be_16(pkt->total_length) < sizeof(struct rte_ipv4_hdr))
+	if (rte_cpu_to_be_16(pkt->total_length) < rte_ipv4_hdr_len(pkt))
 		return -5;
 
+	/*
+	 * Additionally, the router SHOULD verify that the packet length
+	 * reported by the Link Layer is at least as large as the IP total
+	 * length recorded in the packet's IP header.
+	 */
+	if (link_len < rte_cpu_to_be_16(pkt->total_length))
+		return -6;
+
 	return 0;
 }
 #endif /* DO_RFC_1812_CHECKS */
diff --git a/examples/l3fwd/l3fwd_em.h b/examples/l3fwd/l3fwd_em.h
index 1fee2e2e6c..34801fba2b 100644
--- a/examples/l3fwd/l3fwd_em.h
+++ b/examples/l3fwd/l3fwd_em.h
@@ -20,7 +20,8 @@ l3fwd_em_handle_ipv4(struct rte_mbuf *m, uint16_t portid,
 
 #ifdef DO_RFC_1812_CHECKS
 	/* Check to make sure the packet is valid (RFC1812) */
-	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
+	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len - sizeof(struct rte_ether_hdr),
+			m->ol_flags) < 0) {
 		rte_pktmbuf_free(m);
 		return BAD_PORT;
 	}
diff --git a/examples/l3fwd/l3fwd_lpm.h b/examples/l3fwd/l3fwd_lpm.h
index 4ee61e8d88..1731ebb2a8 100644
--- a/examples/l3fwd/l3fwd_lpm.h
+++ b/examples/l3fwd/l3fwd_lpm.h
@@ -22,7 +22,8 @@ l3fwd_lpm_simple_forward(struct rte_mbuf *m, uint16_t portid,
 
 #ifdef DO_RFC_1812_CHECKS
 		/* Check to make sure the packet is valid (RFC1812) */
-		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
+		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len - sizeof(struct rte_ether_hdr),
+				m->ol_flags) < 0) {
 			rte_pktmbuf_free(m);
 			return;
 		}
-- 
2.43.0

[PATCH v5] examples/l3fwd: fix RFC 1812 checks

[Morten Brørup]

Four bug fixes and an enhancement.

1.
If the PMD didn't check the IP header checksum, the
offload value is RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN,
not RTE_MBUF_F_RX_IP_CKSUM_NONE.
CKSUM_NONE means that the packet integrity is good, but the checksum
field has not been set.
CKSUM_UNKNOWN means that the checksum has not been investigated.
(BTW, no PMD currently sets RTE_MBUF_F_RX_IP_CKSUM_NONE, so this part
of the check was effectively dead code.)

Fixed this by processing all four RTE_MBUF_F_RX_IP_CKSUM enum values.
All four values may not actually occur in this application,
but it serves an educational purpose.

2.
When checking the checksum, the checksum field in the packet was zeroed
for calculating the checksum.
If the checksum was correct, the zero value remained in the packet.

Fixed this by simply calculating the checksum of the packet without
modifying the checksum field.
If the calculated checksum is zero, the checksum is correct.

3.
When checking the Link Layer length of an IP packet, the length did
not omit the length of the preceding Layer 2 headers.

Fixed this by subtracting the Ethernet header length when calling the
IPv4 validation function.

4.
In the 5th bullet check,
"5. The IP total length field must be large enough to hold the IP
datagram header, whose length is specified in the IP header length
field."
the comparison did not use the IP header length field, but the size
of the IP header structure, which did not account for IP header
options.

Fixed this by using the IP header length field instead of the
size of the structure.

5.
Added the additional check recommended in RFC 1812 section 5.2.2,
verifying that the packet length reported by the Link Layer is at least
as large as the IP total length recorded in the packet's IP header.

Fixes: 4b01cabfb09b ("examples/l3fwd: add option to relax Rx offload")
Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
v5:
* Use rte_be_to_cpu_16(), not rte_cpu_to_be_16(), for reading
  IP header total_length field. (AI)
v4:
* When CKSUM_NONE, don't update checksum field. (AI)
v3:
* Simplified the checksum check. No need to modify the packet for
  checking the checksum.
v2:
* Rebased.
* Improved (1): Process all four checksum offload enum values, not just
  CKSUM_UNKNOWN instead of CKSUM_NONE.
* Added (2): Restore checksum field value in packet after checking.
* Added (3): Subtract Layer 2 header length when passing Link Layer
  length.
* Added (4): Fix IP header Total Length vs. IP Header Length check.
* Added (5): Link Layer length vs. IP header Total Length check.
* Added the Fixes tag going back to the first public release.
---
 examples/l3fwd/l3fwd.h     | 29 +++++++++++++++++++++--------
 examples/l3fwd/l3fwd_em.h  |  3 ++-
 examples/l3fwd/l3fwd_lpm.h |  3 ++-
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
index 349fc37c79..db59611464 100644
--- a/examples/l3fwd/l3fwd.h
+++ b/examples/l3fwd/l3fwd.h
@@ -170,14 +170,19 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags
 		return -1;
 
 	/* 2. The IP checksum must be correct. */
-	/* if this is not checked in H/W, check it. */
-	if ((ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == RTE_MBUF_F_RX_IP_CKSUM_NONE) {
-		uint16_t actual_cksum, expected_cksum;
-		actual_cksum = pkt->hdr_checksum;
-		pkt->hdr_checksum = 0;
-		expected_cksum = rte_ipv4_cksum(pkt);
-		if (actual_cksum != expected_cksum)
+	switch (ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) {
+	case RTE_MBUF_F_RX_IP_CKSUM_UNKNOWN:
+		/* Not checked in H/W; check it. */
+		if (rte_ipv4_cksum(pkt) != 0)
 			return -2;
+		break;
+	case RTE_MBUF_F_RX_IP_CKSUM_GOOD:
+		break;
+	case RTE_MBUF_F_RX_IP_CKSUM_BAD:
+		return -2;
+	case RTE_MBUF_F_RX_IP_CKSUM_NONE:
+		/* Packet integrity known good, checksum field unset; don't check it. */
+		break;
 	}
 
 	/*
@@ -199,9 +204,17 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags
 	 * datagram header, whose length is specified in the IP header length
 	 * field.
 	 */
-	if (rte_cpu_to_be_16(pkt->total_length) < sizeof(struct rte_ipv4_hdr))
+	if (rte_be_to_cpu_16(pkt->total_length) < rte_ipv4_hdr_len(pkt))
 		return -5;
 
+	/*
+	 * Additionally, the router SHOULD verify that the packet length
+	 * reported by the Link Layer is at least as large as the IP total
+	 * length recorded in the packet's IP header.
+	 */
+	if (link_len < rte_be_to_cpu_16(pkt->total_length))
+		return -6;
+
 	return 0;
 }
 #endif /* DO_RFC_1812_CHECKS */
diff --git a/examples/l3fwd/l3fwd_em.h b/examples/l3fwd/l3fwd_em.h
index 1fee2e2e6c..34801fba2b 100644
--- a/examples/l3fwd/l3fwd_em.h
+++ b/examples/l3fwd/l3fwd_em.h
@@ -20,7 +20,8 @@ l3fwd_em_handle_ipv4(struct rte_mbuf *m, uint16_t portid,
 
 #ifdef DO_RFC_1812_CHECKS
 	/* Check to make sure the packet is valid (RFC1812) */
-	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
+	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len - sizeof(struct rte_ether_hdr),
+			m->ol_flags) < 0) {
 		rte_pktmbuf_free(m);
 		return BAD_PORT;
 	}
diff --git a/examples/l3fwd/l3fwd_lpm.h b/examples/l3fwd/l3fwd_lpm.h
index 4ee61e8d88..1731ebb2a8 100644
--- a/examples/l3fwd/l3fwd_lpm.h
+++ b/examples/l3fwd/l3fwd_lpm.h
@@ -22,7 +22,8 @@ l3fwd_lpm_simple_forward(struct rte_mbuf *m, uint16_t portid,
 
 #ifdef DO_RFC_1812_CHECKS
 		/* Check to make sure the packet is valid (RFC1812) */
-		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
+		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len - sizeof(struct rte_ether_hdr),
+				m->ol_flags) < 0) {
 			rte_pktmbuf_free(m);
 			return;
 		}
-- 
2.43.0