Re: [PATCH v3] bpf: bpf_dbg: split pcap_next_pkt() validation/advance, fix off-by-one in cmd_select

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 3728 bytes --]

> diff --git a/tools/bpf/bpf_dbg.c b/tools/bpf/bpf_dbg.c
> --- a/tools/bpf/bpf_dbg.c
> +++ b/tools/bpf/bpf_dbg.c
> @@ -918,21 +918,30 @@ static struct pcap_pkthdr *pcap_curr_pkt(void)
>  	return (void *) pcap_ptr_va_curr;
>  }
>
> -static bool pcap_next_pkt(void)
> +static bool pcap_curr_pkt_valid(void)
>  {
>  	struct pcap_pkthdr *hdr = pcap_curr_pkt();
>
>  	if (pcap_ptr_va_curr + sizeof(*hdr) -
> -	    pcap_ptr_va_start >= pcap_map_size)
> +	    pcap_ptr_va_start > pcap_map_size)
>  		return false;
>  	if (hdr->caplen == 0 || hdr->len == 0 || hdr->caplen > hdr->len)
>  		return false;
>  	if (pcap_ptr_va_curr + sizeof(*hdr) + hdr->caplen -
> -	    pcap_ptr_va_start >= pcap_map_size)
> +	    pcap_ptr_va_start > pcap_map_size)
>  		return false;
> +	return true;
> +}
>
> +static bool pcap_next_pkt(void)
> +{
> +	struct pcap_pkthdr *hdr;
> +
> +	if (!pcap_curr_pkt_valid())
> +		return false;
> +	hdr = pcap_curr_pkt();
>  	pcap_ptr_va_curr += (sizeof(*hdr) + hdr->caplen);
> -	return true;
> +	return pcap_curr_pkt_valid();
>  }
>
>  static void pcap_reset_pkt(void)
> @@ -1143,7 +1152,7 @@ static int cmd_select(char *num)
>
>  	for (i = 0; i < which && (have_next = pcap_next_pkt()); i++)
>  		/* noop */;
> -	if (!have_next || pcap_curr_pkt() == NULL) {
> +	if (!have_next || !pcap_curr_pkt_valid()) {

The commit message asserts that after this fix, 'select K shows packet K
for all K in 1..N' and 'select N+1 correctly errors with no packet #N+1
available!'. Does the actual code achieve this behavior given that the
loop still initializes i to 0?

With the new pcap_next_pkt() semantics (validate-current, advance,
validate-new), trace through 'select 1' on a file with >=2 packets:

  Iteration 1 (i=0):
    pcap_next_pkt() validates pkt1 (ok)
    advances cursor to pkt2
    validates pkt2 (ok)
    returns true
    i becomes 1

  Loop exit (i=1, i<1 is false)

  Cursor is now on pkt2
  pcap_curr_pkt_valid() returns true
  Function returns CMD_OK

The user requested packet 1, but subsequent 'step' or 'run' operates on
pkt2. This reproduces the exact defect the commit claims to fix: 'Before
this fix, select 1 shows packet 2's payload.'

Similarly, trace 'select 3' on a file with exactly 3 packets:

  Iteration 1 (i=0): advances pkt1 -> pkt2, validates pkt2 (ok), i=1
  Iteration 2 (i=1): advances pkt2 -> pkt3, validates pkt3 (ok), i=2
  Iteration 3 (i=2): pkt3 valid, advances past pkt3 (cursor at EOF),
                     post-advance pcap_curr_pkt_valid() is false,
                     pcap_next_pkt() returns false, have_next=false

  Loop exits with have_next=false
  Code hits 'no packet #3 available!' even though packet 3 exists

The commit message itself identifies the correct fix: "Simply changing the
loop init to 'i = 1' (so it advances N-1 times) fixes the user-visible
symptom". That change was not applied in the diff. The loop still uses
'i = 0', which causes it to advance N times instead of N-1 times, landing
on packet N+1 when the user requested packet N.

The v3 patch discussion on lore.kernel.org/bpf addressed this by changing
the loop from 'for (i = 0; i < which ...)' to 'for (i = 1; i < which ...)'
to properly implement 1-based indexing. The current code has the
pcap_curr_pkt_valid() refactoring but is missing the loop initialization
fix.

Reference: https://lore.kernel.org/bpf/20260429123543.61559-1-basbunarhasan@gmail.com/

>  		rl_printf("no packet #%u available!\n", which);
>  		pcap_reset_pkt();
>  		return CMD_ERR;


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25109638113