|SUCCESS| [2/2] ip_frag: randomize hash seed

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162949

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-09

Commit: 95908f52393d ("ip_frag: free mbufs on reassembly table destroy")
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH 1/2] ip_frag: fix unsafe TAILQ usage

This patch fixes unsafe TAILQ iterator usage in
rte_ip_frag_table_del_expired_entries() where the next pointer was
accessed after TAILQ_REMOVE().

> diff --git a/lib/ip_frag/ip_frag_common.h b/lib/ip_frag/ip_frag_common.h
> index 51fc9d47fb..34be5bb6ab 100644
> --- a/lib/ip_frag/ip_frag_common.h
> +++ b/lib/ip_frag/ip_frag_common.h
> @@ -8,6 +8,7 @@
>  #include <sys/queue.h>
>  
>  #include <rte_common.h>
> +#include <rte_tailq.h>

The include order violates the coding style guide. System headers should
precede DPDK headers, but rte_common.h should be included before
rte_tailq.h. The guide shows DPDK headers should be grouped together,
but the ordering here appears inconsistent. Can this header be placed
after rte_common.h following the established pattern?

> diff --git a/lib/ip_frag/rte_ip_frag_common.c b/lib/ip_frag/rte_ip_frag_common.c
> index ee9aa93027..79ac45289b 100644
> --- a/lib/ip_frag/rte_ip_frag_common.c
> +++ b/lib/ip_frag/rte_ip_frag_common.c
> @@ -135,18 +135,18 @@ rte_ip_frag_table_del_expired_entries(struct rte_ip_frag_tbl *tbl,
>  	struct rte_ip_frag_death_row *dr, uint64_t tms)
>  {
>  	uint64_t max_cycles;
> -	struct ip_frag_pkt *fp;
> +	struct ip_frag_pkt *fp, *tmp;
>  
>  	max_cycles = tbl->max_cycles;
>  
> -	TAILQ_FOREACH(fp, &tbl->lru, lru)
> -		if (max_cycles + fp->start < tms) {
> -			/* check that death row has enough space */
> -			if (RTE_IP_FRAG_DEATH_ROW_MBUF_LEN - dr->cnt >=
> -					fp->last_idx)
> -				ip_frag_tbl_del(tbl, dr, fp);
> -			else
> -				return;
> -		} else
> +	RTE_TAILQ_FOREACH_SAFE(fp, &tbl->lru, lru, tmp) {
> +		if (max_cycles + fp->start >= tms)
> +			return;
> +
> +		/* check that death row has enough space */
> +		if (RTE_IP_FRAG_DEATH_ROW_MBUF_LEN - dr->cnt < fp->last_idx)
>  			return;
> +
> +		ip_frag_tbl_del(tbl, dr, fp);
> +	}
>  }

The logic transformation looks correct. However, does the inverted
condition "max_cycles + fp->start >= tms" produce identical behavior
to the original code? In the original, entries with timestamps exactly
equal to "tms - max_cycles" would be deleted, but now they would not be.
Is this boundary condition change intentional?

---

Commit: 416707812c03 ("ip_frag: refactor reassembly code into a proper library")
Author: Stephen Hemminger <stephen at networkplumber.org>
Subject: [PATCH 2/2] ip_frag: randomize hash seed

This patch addresses a hash collision attack vector by replacing the
fixed prime seed with a random value generated at table creation time.

> diff --git a/lib/ip_frag/ip_frag_internal.c b/lib/ip_frag/ip_frag_internal.c
> index 7cbef647df..382f42d0e1 100644
> --- a/lib/ip_frag/ip_frag_internal.c
> +++ b/lib/ip_frag/ip_frag_internal.c
> @@ -9,8 +9,6 @@
>  
>  #include "ip_frag_common.h"
>  
> -#define	PRIME_VALUE	0xeaad8405
> -

[ ... ]

> @@ -38,7 +36,7 @@ ip_frag_tbl_reuse(struct rte_ip_frag_tbl *tbl, struct rte_ip_frag_death_row *dr,
>  
>  
>  static inline void
> -ipv4_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
> +ipv4_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2, uint32_t seed)
>  {
>  	uint32_t v;
>  	const uint32_t *p;
> @@ -46,12 +44,12 @@ ipv4_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
>  	p = (const uint32_t *)&key->src_dst;
>  
>  #if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> -	v = rte_hash_crc_4byte(p[0], PRIME_VALUE);
> +	v = rte_hash_crc_4byte(p[0], seed);
>  	v = rte_hash_crc_4byte(p[1], v);
>  	v = rte_hash_crc_4byte(key->id, v);
>  #else
>  
> -	v = rte_jhash_3words(p[0], p[1], key->id, PRIME_VALUE);
> +	v = rte_jhash_3words(p[0], p[1], key->id, seed);
>  #endif /* RTE_ARCH_X86 */

[ ... ]

> @@ -59,7 +57,7 @@ ipv4_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
>  }
>  
>  static inline void
> -ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
> +ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2, uint32_t seed)
>  {
>  	uint32_t v;
>  	const uint32_t *p;
> @@ -67,7 +65,7 @@ ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
>  	p = (const uint32_t *) &key->src_dst;
>  
>  #if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> -	v = rte_hash_crc_4byte(p[0], PRIME_VALUE);
> +	v = rte_hash_crc_4byte(p[0], seed);
>  	v = rte_hash_crc_4byte(p[1], v);
>  	v = rte_hash_crc_4byte(p[2], v);
>  	v = rte_hash_crc_4byte(p[3], v);
> @@ -78,7 +76,7 @@ ipv6_frag_hash(const struct ip_frag_key *key, uint32_t *v1, uint32_t *v2)
>  	v = rte_hash_crc_4byte(key->id, v);
>  #else
>  
> -	v = rte_jhash_3words(p[0], p[1], p[2], PRIME_VALUE);
> +	v = rte_jhash_3words(p[0], p[1], p[2], seed);
>  	v = rte_jhash_3words(p[3], p[4], p[5], v);
>  	v = rte_jhash_3words(p[6], p[7], key->id, v);
>  #endif /* RTE_ARCH_X86 */

[ ... ]

> @@ -301,9 +299,9 @@ ip_frag_lookup(struct rte_ip_frag_tbl *tbl,
>  
>  	/* different hashing methods for IPv4 and IPv6 */
>  	if (key->key_len == IPV4_KEYLEN)
> -		ipv4_frag_hash(key, &sig1, &sig2);
> +		ipv4_frag_hash(key, &sig1, &sig2, tbl->seed);
>  	else
> -		ipv6_frag_hash(key, &sig1, &sig2);
> +		ipv6_frag_hash(key, &sig1, &sig2, tbl->seed);

[ ... ]

> diff --git a/lib/ip_frag/ip_reassembly.h b/lib/ip_frag/ip_reassembly.h
> index 54afed5417..9e1666ef67 100644
> --- a/lib/ip_frag/ip_reassembly.h
> +++ b/lib/ip_frag/ip_reassembly.h
> @@ -79,6 +79,7 @@ struct rte_ip_frag_tbl {
>  	uint32_t bucket_entries; /* hash associativity. */
>  	uint32_t nb_entries;     /* total size of the table. */
>  	uint32_t nb_buckets;     /* num of associativity lines. */
> +	uint32_t seed;		 /* hash function init value */

The comment style is inconsistent with existing fields. The other fields
use "/* comment */" with period endings, while this uses "/* comment */"
without a period. Can this follow the established pattern with "/* Hash
function init value. */"?

Also, does adding this field break ABI compatibility? The struct
rte_ip_frag_tbl size changes, which could affect applications compiled
against older library versions. Should this be marked with __rte_cache_aligned
or is ABI stability not a concern for this internal structure?

> diff --git a/lib/ip_frag/rte_ip_frag_common.c b/lib/ip_frag/rte_ip_frag_common.c
> index 79ac45289b..00bf9476de 100644
> --- a/lib/ip_frag/rte_ip_frag_common.c
> +++ b/lib/ip_frag/rte_ip_frag_common.c
> @@ -79,6 +79,7 @@ rte_ip_frag_table_create(uint32_t bucket_num, uint32_t bucket_entries,
>  	tbl->nb_buckets = bucket_num;
>  	tbl->bucket_entries = bucket_entries;
>  	tbl->entry_mask = (tbl->nb_entries - 1) & ~(tbl->bucket_entries  - 1);
> +	tbl->seed = rte_rand();

Does rte_rand() provide cryptographically secure random numbers suitable
for defense against deliberate collision attacks? The commit message
describes a security concern about precomputed collisions. If rte_rand()
uses a predictable PRNG, could an attacker still mount a timing attack to
discover the seed value? Should this use a more secure random source?