|SUCCESS| examples/ipsec-secgw: drop packets in poll mode

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-06-01

Below is the review of the patch:

Commit: (git sha not available in patch file)
Author: Rakesh Kudurumalla <rkudurumalla at marvell.com>
Subject: examples/ipsec-secgw: drop packets in poll mode

This patch addresses packet dropping behavior during antireplay tests in
poll mode, where packets were incorrectly forwarded despite errors.

> diff --git a/examples/ipsec-secgw/ipsec_worker.c b/examples/ipsec-secgw/ipsec_worker.c
> index 04609964cd..61b4126759 100644
> --- a/examples/ipsec-secgw/ipsec_worker.c
> +++ b/examples/ipsec-secgw/ipsec_worker.c
> @@ -428,7 +428,7 @@ process_ipsec_ev_inbound(struct ipsec_ctx *ctx, struct route_table *rt,
>  		if (pkt->ol_flags & RTE_MBUF_F_RX_SEC_OFFLOAD) {
>  			if (unlikely(pkt->ol_flags &
>  				     RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED)) {
> -				RTE_LOG(ERR, IPSEC,
> +				RTE_LOG(DEBUG, IPSEC,
>  					"Inbound security offload failed\n");
>  				goto drop_pkt_and_exit;
>  			}
> @@ -446,7 +446,7 @@ process_ipsec_ev_inbound(struct ipsec_ctx *ctx, struct route_table *rt,
>  		if (pkt->ol_flags & RTE_MBUF_F_RX_SEC_OFFLOAD) {
>  			if (unlikely(pkt->ol_flags &
>  				     RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED)) {
> -				RTE_LOG(ERR, IPSEC,
> +				RTE_LOG(DEBUG, IPSEC,
>  					"Inbound security offload failed\n");
>  				goto drop_pkt_and_exit;
>  			}

These changes downgrade error logging from ERR to DEBUG level. Why is a
security offload failure not considered an error-worthy event? This seems
to reduce visibility into genuine security processing failures.

> diff --git a/examples/ipsec-secgw/ipsec_worker.h b/examples/ipsec-secgw/ipsec_worker.h
> index 8f96161293..d5b8618fc8 100644
> --- a/examples/ipsec-secgw/ipsec_worker.h
> +++ b/examples/ipsec-secgw/ipsec_worker.h
> @@ -421,7 +421,9 @@ inbound_sp_sa(struct sp_ctx *sp, struct sa_ctx *sa, struct traffic_type *ip,
>  		}
>  
>  		/* Only check SPI match for processed IPSec packets */
> -		if (i < lim && ((m->ol_flags & RTE_MBUF_F_RX_SEC_OFFLOAD) == 0)) {
> +		if (i < lim && ((m->ol_flags & (RTE_MBUF_F_RX_SEC_OFFLOAD |
> +				 RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED))) !=
> +				 RTE_MBUF_F_RX_SEC_OFFLOAD) {
>  			stats->discard++;
>  			free_pkts(&m, 1);
>  			continue;

Does this condition correctly identify packets that should be dropped?
The original logic dropped packets without RTE_MBUF_F_RX_SEC_OFFLOAD. The
modified logic appears to drop packets when:
  - Neither flag is set (same as before)
  - RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED is set alone
  - Both flags are set

Is the third case (both flags set simultaneously) a valid hardware state?
Can RTE_MBUF_F_RX_SEC_OFFLOAD and RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED both
be set on the same packet?

The logic would be clearer if written explicitly as:
  if (i < lim && (m->ol_flags & RTE_MBUF_F_RX_SEC_OFFLOAD) == 0 ||
      (m->ol_flags & RTE_MBUF_F_RX_SEC_OFFLOAD_FAILED))

This makes the intent more obvious: drop if offload was not performed OR
if offload failed.

The multi-line formatting violates DPDK coding style. Continuation lines
should align with the opening parenthesis or be indented with tabs plus
spaces to align. The current indentation uses spaces inconsistently.

The commit message states packets are forwarded despite errors in poll
mode. However, the changes affect both process_ipsec_ev_inbound()
(event mode) and inbound_sp_sa() (poll mode). Does the bug exist in event
mode as well, or are these changes unintended?