Re: [PATCH v4 01/24] net/_common_intel: add pkt reassembly fn for intel drivers

[Stephen Hemminger]

On Mon, 6 Jan 2025 14:25:05 +
Bruce Richardson  wrote:

> On Fri, Dec 20, 2024 at 08:15:40AM -0800, Stephen Hemminger wrote:
> > On Fri, 20 Dec 2024 14:38:58 +
> > Bruce Richardson  wrote:
> >   
> > > +
> > > + if (!split_flags[buf_idx]) {
> > > + /* it's the last packet of the set */
> > > + start->hash = end->hash;
> > > + start->vlan_tci = end->vlan_tci;
> > > + start->ol_flags = end->ol_flags;
> > > + /* we need to strip crc for the whole packet */
> > > + start->pkt_len -= crc_len;
> > > + if (end->data_len > crc_len) {
> > > + end->data_len -= crc_len;
> > > + } else {
> > > + /* free up last mbuf */
> > > + struct rte_mbuf *secondlast = start;
> > > +
> > > + start->nb_segs--;
> > > + while (secondlast->next != end)
> > > + secondlast = secondlast->next;
> > > + secondlast->data_len -= (crc_len - 
> > > end->data_len);
> > > + secondlast->next = NULL;
> > > + rte_pktmbuf_free_seg(end);
> > > + }  
> > 
> > The problem with freeing the last buffer is that the CRC will be garbage.
> > What if the CRC is sitting past the last mbuf?
> > 
> > +---++-+
> > | Data  +--->+ CRC |
> > +---++-+
> > 
> > This part (from original code) will free the second mbuf which contains
> > the CRC. The whole "don't strip CRC and leave it past the mbuf data" model
> > of mbuf's is a danger trap.  
> 
> Can you explain more clearly what you see as the issue with the current
> code above?
> 
> The "crc_len" variable contains the length of the CRC included in the
> packet, which should be removed from that before returning the mbuf from RX.
> It contains "0" if the CRC is HW stripped, and "4" if the CRC needs to be
> removed by software - something that is done just by subtracting the CRC
> length from the packet and buffer lengths. The freeing of the last segment
> occurs only in the case that the last segment contains the CRC only - or
> part of the CRC only, as otherwise we would have an extra empty buffer at
> the end of the chain.

But if the application is using the driver in "keep CRC" mode,
then it probably intends to look at the CRC. In that case crc_len is 4,
and that trailing buffer may need to be retained.

It all goes back to the design mistake of the semantics of "keep CRC"
mode. In that mode, the mbuf chain has hidden data (past pkt_len and data_len).

[PATCH] examples/flow_filtering: fix destination IP mask

[Shani Peretz]

This patch corrects the destination IP address mask to
restore the previous implementation's behavior.

Also it fixes a misuse of rte_flow_item_tcp struct.
Replace it with the appropriate rte_flow_item_ipv4 struct,
as the code in this context filters ipv4 traffic.

Fixes: 16158f349000 ("examples/flow_filtering: introduce use cases snippets")

Signed-off-by: Shani Peretz 
---
 examples/flow_filtering/snippets/snippet_match_ipv4.c | 10 +-
 examples/flow_filtering/snippets/snippet_match_ipv4.h |  1 -
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/examples/flow_filtering/snippets/snippet_match_ipv4.c 
b/examples/flow_filtering/snippets/snippet_match_ipv4.c
index 808208e7b0..d32f4ebfdc 100644
--- a/examples/flow_filtering/snippets/snippet_match_ipv4.c
+++ b/examples/flow_filtering/snippets/snippet_match_ipv4.c
@@ -55,7 +55,7 @@ snippet_ipv4_flow_create_patterns(struct rte_flow_item 
*patterns)
fprintf(stderr, "Failed to allocate memory for ip_mask\n");
 
ip_spec->hdr.dst_addr = htonl(DEST_IP); /* The dest ip value to match 
the input packet. */
-   ip_mask->hdr.dst_addr = DEST_MASK; /* The mask to apply to the dest ip. 
*/
+   ip_mask->hdr.dst_addr = FULL_MASK; /* The mask to apply to the dest ip. 
*/
ip_spec->hdr.src_addr = htonl(SRC_IP); /* The src ip value to match the 
input packet. */
ip_mask->hdr.src_addr = EMPTY_MASK; /* The mask to apply to the src ip. 
*/
patterns[1].spec = ip_spec;
@@ -77,7 +77,7 @@ snippet_ipv4_flow_create_actions_template(uint16_t port_id, 
struct rte_flow_erro
};
 
tactions[0].type = RTE_FLOW_ACTION_TYPE_QUEUE;
-   tactions[0].type = RTE_FLOW_ACTION_TYPE_END;
+   tactions[1].type = RTE_FLOW_ACTION_TYPE_END;
 
/* This sets the masks to match the actions, indicating that all fields 
of the actions
 * should be considered as part of the template.
@@ -93,7 +93,7 @@ static struct rte_flow_pattern_template *
 snippet_ipv4_flow_create_pattern_template(uint16_t port_id, struct 
rte_flow_error *error)
 {
struct rte_flow_item titems[MAX_PATTERN_NUM] = {0};
-   struct rte_flow_item_tcp ip_mask = {0};
+   struct rte_flow_item_ipv4 ip_mask = {0};
 
struct rte_flow_pattern_template_attr attr = {
.relaxed_matching = 1,
@@ -102,8 +102,8 @@ snippet_ipv4_flow_create_pattern_template(uint16_t port_id, 
struct rte_flow_erro
 
titems[0].type = RTE_FLOW_ITEM_TYPE_ETH;
titems[1].type = RTE_FLOW_ITEM_TYPE_IPV4;
-   ip_mask.hdr.src_port = EMPTY_MASK;
-   ip_mask.hdr.dst_port = DEST_MASK;
+   ip_mask.hdr.src_addr = EMPTY_MASK;
+   ip_mask.hdr.dst_addr = FULL_MASK;
titems[1].mask = &ip_mask;
titems[2].type = RTE_FLOW_ITEM_TYPE_END;
 
diff --git a/examples/flow_filtering/snippets/snippet_match_ipv4.h 
b/examples/flow_filtering/snippets/snippet_match_ipv4.h
index 847784beef..597a1c954e 100644
--- a/examples/flow_filtering/snippets/snippet_match_ipv4.h
+++ b/examples/flow_filtering/snippets/snippet_match_ipv4.h
@@ -13,7 +13,6 @@
 #define DEST_IP ((192<<24) + (168<<16) + (1<<8) + 1) /* dest ip = 192.168.1.1 
*/
 #define FULL_MASK 0x /* full mask */
 #define EMPTY_MASK 0x0 /* empty mask */
-#define DEST_MASK 0x /* full mask */
 
 #define MAX_PATTERN_NUM3 /* Maximal number of patterns for 
this example. */
 #define MAX_ACTION_NUM 2 /* Maximal number of actions for this 
example. */
-- 
2.34.1