[PATCH] AGENTS: reduce false positives

[Stephen Hemminger <stephen@networkplumber.org>]

Revise the AGENTS.md file to reduce the number of false positives
from in CI reviews. The existing file will make up non-existent
functions and get confused about exposed API's.

Also, reflow the text on sentence boundaries instead of hard
limits at 70 cols.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 AGENTS.md | 199 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 160 insertions(+), 39 deletions(-)

diff --git a/AGENTS.md b/AGENTS.md
index 813584352b..104d59ef91 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -5,13 +5,16 @@
 
 ## CRITICAL INSTRUCTION - READ FIRST
 
-This document has two categories of review rules with different confidence thresholds:
+This document has two categories of review rules with different confidence thresholds,
+plus rules on evidence and scope that apply to both.
 
 ### 1. Correctness Bugs -- HIGHEST PRIORITY (report at >=50% confidence)
 
 **Always report potential correctness bugs.** These are the most valuable findings.
-When in doubt, report them with a note about your confidence level.
-A possible use-after-free or resource leak is worth mentioning even if you are not certain.
+When in doubt, report them -- subject to the evidence rule below.
+Uncertainty about code you have read is fine:
+a possible use-after-free or resource leak is worth mentioning even if you are not certain.
+Uncertainty about code you have not read is not a finding at all.
 
 Correctness bugs include:
 - Use-after-free (accessing memory after `free`/`rte_free`)
@@ -44,9 +47,65 @@ Correctness bugs include:
 - Rx queue setup silently drops oversized packets instead of enabling scatter or returning an error
 - Rx function selection ignores `scattered_rx` flag or MTU-vs-mbuf-size check
 
-**Do NOT self-censor correctness bugs.** If you identify a code path
-where a resource could leak or memory could be used after free, report it.
-Do not talk yourself out of it.
+**Do NOT self-censor correctness bugs.**
+If you identify a code path where a resource could leak or memory could be used after free,
+report it -- subject to the evidence rule below.
+Do not talk yourself out of a bug you can see.
+Do not talk yourself into one you cannot.
+
+### Evidence rule (applies to every finding)
+
+Every finding must rest on code you have actually read.
+"Read" means a line in the patch, or a file you opened with a tool.
+
+This document is used in two ways and the rule is the same in both.
+
+**Diff only** -- CI, `review-patch.py`, and any run without tool access.
+All you have read is the patch: added and removed lines, plus a few lines of context.
+You cannot see the rest of the file, other callers, or any other file in the tree,
+so any claim that depends on them cannot be made.
+
+**With tool access** -- CLI or agentic use, where you can clone the tree, apply the series,
+grep, and read whole files.
+Read them.
+Check the surrounding function, the callers, and the headers before you report.
+Tool access does not lower the bar; it widens what you are able to read,
+and therefore what you are expected to have read before filing a finding.
+
+In either mode:
+
+- Report a finding only if you can name the added or changed line that causes it,
+  and quote that line verbatim from what you read.
+- Never quote, paraphrase, or reconstruct code you have not read.
+  If you are writing a code block with line numbers you did not read, you are inventing it.
+  Delete the finding.
+- If a claim depends on code you have not read -- other callers, other files,
+  the rest of the function -- either go read it or omit the claim.
+  Do not file it with a caveat, do not call it a risk,
+  and do not ask the author to verify it on your behalf.
+- "presumably", "appears to be", "if this is", "potentially", "could be" inside a finding
+  mean you lack the evidence.
+  Get the evidence or omit the finding.
+
+The >=50% bar for correctness bugs applies to your judgement about code you have read.
+It is not permission to speculate about code you have not.
+
+### Pre-existing code (applies to every finding)
+
+The unit of review is the patch.
+Code that appears only as diff context, or that you read in the tree but the patch does not touch,
+is not what this author is being asked to fix.
+A finding about such code must never be an Error or a Warning
+and must never withhold a Reviewed-by.
+
+Report it as Info, and say plainly that it is pre-existing and not introduced by this patch.
+This is worth doing: it puts the observation in front of the people working in that code
+so it can be picked up in a separate patch later.
+
+The evidence rule still applies in full.
+An Info note about surrounding code needs the same proof as any other finding:
+you must have read the code and be able to quote the line.
+"Pre-existing" is a lower severity, not a lower standard of evidence.
 
 ### 2. Style, Process, and Formatting -- suppress false positives
 
@@ -95,8 +154,11 @@ than one that catches every style issue but misses the bug.
 - Check every `goto error` and early `return`: does it clean up everything allocated so far?
 - Look for use-after-free: after `free(p)`, is `p` accessed again?
 - Check that error codes are propagated, not silently dropped
-- Report at >=50% confidence; note uncertainty if appropriate
-- It is better to report a potential bug that turns out to be safe than to miss a real bug
+- Report at >=50% confidence about code you have read
+- Never substitute a confidence caveat for evidence:
+  if the reasoning depends on code you have not read, go read it or drop the finding
+- It is better to report a potential bug that turns out to be safe than to miss a real bug,
+  provided you can point to the line that causes it
 
 ### Style and process review guidelines
 - Only comment on style/process issues when you have HIGH CONFIDENCE (>80%) that an issue exists
@@ -129,6 +191,8 @@ than one that catches every style issue but misses the bug.
   Do not include a patch in your output just to say "no issues found"
   or to summarize what the patch does.
   Only include patches where you have actual findings to report.
+- Do NOT end the review with a summary section listing every patch in the series.
+  A clean patch must not appear anywhere in the output, including in a summary or status table.
 
 ## Priority Areas (Review These)
 
@@ -515,39 +579,52 @@ than one that catches every style issue but misses the bug.
 
 ### Cryptographic and Security Code
 
-Applies only when the patch touches crypto PMDs (drivers/crypto/),
-the security or ipsec libraries, or code handling key material, IVs,
-or authentication digests. Stays silent on all other patches.
+Applies only when the patch touches crypto PMDs (`drivers/crypto/`),
+the security or ipsec libraries,
+or code handling key material, IVs, or authentication digests.
+Stays silent on all other patches.
 
 - **Non-constant-time digest/tag comparison** (Error):
   When verifying an authentication tag, digest, MAC, or ICV --
   comparing a computed value against an attacker-supplied one to decide accept/reject --
-  plain memcmp leaks timing information.
-  Use rte_memeq_timingsafe() instead.
-  Return semantics differ from memcmp: memcmp returns 0 on equal,
-  rte_memeq_timingsafe() returns true on equal.
-  So `if (memcmp(tag, digest, len))` becomes
-  `if (!rte_memeq_timingsafe(tag, digest, len))`.
-  Do NOT flag memcmp on non-secret data: algorithm IDs, key lengths,
-  capability/feature structs, lookup keys.
+  plain `memcmp()` leaks timing information.
+  Use `rte_memeq_timingsafe()` instead.
+  Return semantics differ: `memcmp()` returns 0 on equal,
+  `rte_memeq_timingsafe()` returns true on equal.
+  So `if (memcmp(tag, digest, len))` becomes `if (!rte_memeq_timingsafe(tag, digest, len))`.
+  Do NOT flag `memcmp()` on non-secret data:
+  algorithm IDs, key lengths, capability/feature structs, lookup keys.
   Only flag comparisons that gate acceptance of attacker-influenced data.
 
 - **Sensitive material not zeroed before free** (Error):
-  Keys, expanded key schedules, HMAC ipad/opad, and session secrets
-  must be wiped, not merely freed -- a plain free leaves secrets in heap.
-  - rte_malloc/rte_zmalloc'd secret: use rte_free_sensitive() instead of rte_free()
-  - local/stack secret going out of scope: rte_memzero_explicit()
-    before return; plain memset() may be optimized away.
-  Do NOT flag buffers that never held secrets (descriptors, dev_info),
-  or memset the compiler cannot elide because the pointer escapes to free.
-
-- **Insecure RNG for keys/IVs** (Error):
-  rte_rand()/rand()/random() are not cryptographically secure;
-  do not use them to generate keys, IVs, or nonces in crypto/security code.
+  Keys, expanded key schedules, HMAC ipad/opad, and session secrets must be wiped,
+  not merely freed -- a plain free leaves secrets in the heap.
+  - `rte_malloc`/`rte_zmalloc`'d secret: use `rte_free_sensitive()` instead of `rte_free()`
+  - local or stack secret going out of scope: `rte_memzero_explicit()` before return;
+    plain `memset()` may be optimized away
+  Flag only where the patch itself allocates, copies, or frees the secret.
+  Do NOT flag buffers that never held secrets (descriptors, `dev_info`),
+  pre-existing free paths the patch does not touch,
+  or `memset()` the compiler cannot elide because the pointer escapes to `free()`.
+  `rte_memzero_explicit()` followed by a plain `rte_free()` is correct
+  and is not a missing `rte_free_sensitive()`.
+
+- **Non-cryptographic RNG for keys/IVs** (Error):
+  `rte_rand()` is seeded from system entropy (`getentropy()`, with RDSEED and TSC fallbacks),
+  but the generator itself is an LFSR and is linear:
+  an observer who sees enough output can recover the state and predict every later value.
+  Good seeding is not sufficient for key, IV, or nonce generation.
+  Use a cryptographic source instead:
+  the device's own entropy source or DRBG,
+  or `getrandom(2)` / `/dev/urandom` in the control path.
+  `rand()` and `random()` are weaker still and must not appear in crypto or security code.
+  Do NOT flag `rte_rand()` used for non-secret purposes:
+  hash seeds (`lib/ipsec/ipsec_sad.c` uses it for `hash_func_init_val`),
+  jitter, backoff, load balancing, or test data.
 
 Do NOT flag:
 - IV/nonce reuse -- a runtime property, not determinable from a patch.
-- memcmp on lengths, algorithm selectors, or non-secret config.
+- `memcmp()` on lengths, algorithm selectors, or non-secret config.
 
 ### Architecture & Patterns
 - Code that violates existing patterns in the code base
@@ -1585,6 +1662,25 @@ rte_foo_internal_helper(void)
 - Export macros go in `.c` files, not headers
 - The build system generates linker version maps automatically
 
+#### What Counts as ABI
+
+ABI concerns apply to symbols and types reachable from an installed header.
+A header is installed only if a meson.build lists it in `headers` or `indirect_headers`.
+
+Most drivers install nothing, but some do.
+`drivers/net/bonding/meson.build` installs `rte_eth_bond.h`,
+`drivers/net/intel/i40e/meson.build` installs `rte_pmd_i40e.h`,
+and bus/pci, bus/vdev and several baseband drivers install public headers as well.
+Check the driver's meson.build rather than assuming either way.
+
+A function declared only in a driver-private header,
+with no `RTE_EXPORT_*` annotation and no presence in an installed header, has no ABI.
+Removing it or changing its signature is not an ABI break,
+and speculation about unseen external callers is not a finding.
+
+This does not weaken the separate requirement that a *new* public function
+carry an `RTE_EXPORT_*` macro; a new public API without one is still a Warning.
+
 #### What NOT to Review
 
 - Do **NOT** flag missing `version.map` updates - maps are auto-generated
@@ -1678,9 +1774,9 @@ Checked by `devtools/checkpatches.sh` -- not duplicated here.
 - [ ] `bool` used for pure true/false variables, parameters, and predicate return types
 - [ ] Shared variables use `rte_atomic_*_explicit()`, not `volatile` or bare access
 - [ ] Memory ordering is the weakest correct choice (`relaxed` for counters, `acquire`/`release` for publish/consume)
-- [ ] Auth tag/digest comparisons use rte_memeq_timingsafe(), not memcmp
-- [ ] Key material zeroed before free (rte_free_sensitive / rte_memzero_explicit)
-- [ ] Keys/IVs/nonces not generated with rte_rand()/rand()/random()
+- [ ] Auth tag/digest comparisons use `rte_memeq_timingsafe()`, not `memcmp()`
+- [ ] Key material zeroed before free (`rte_free_sensitive()` / `rte_memzero_explicit()`)
+- [ ] Keys/IVs/nonces not generated with `rte_rand()`, `rand()`, or `random()`
 
 ### API Tags
 
@@ -1793,8 +1889,9 @@ devtools/get-maintainer.sh <patch-file>
 - `mtu_set` rejects valid MTU when scatter Rx is already enabled
 - Rx function selection ignores `scattered_rx` flag or MTU-vs-mbuf-size comparison
 - Non-constant-time comparison of auth tag/digest/MAC/ICV (timing side channel)
-- Key material or session secrets freed without zeroing (rte_free_sensitive/rte_memzero_explicit)
-- Non-cryptographic RNG (rte_rand/rand/random) used to generate keys, IVs, or nonces
+- Key material or session secrets freed without zeroing
+  (`rte_free_sensitive()` / `rte_memzero_explicit()`)
+- Non-cryptographic RNG (`rte_rand()`, `rand()`, `random()`) used to generate keys, IVs, or nonces
 
 *Process and format errors:*
 - Forbidden tokens in code
@@ -1857,6 +1954,13 @@ devtools/get-maintainer.sh <patch-file>
 - Cross-patch compilation dependencies - you cannot determine patch ordering correctness from review
 - Claims that a symbol "was removed in patch N" causing issues in patch M - assume author ordered correctly
 - Any speculation about whether patches will compile when applied in sequence
+- Any finding about code you have not read - reconstructed from diff context,
+  inferred from other patches, or assumed from a function name
+- Pre-existing problems in code the patch does not touch, as an Error or Warning
+  (report these as Info, labelled pre-existing)
+- ABI or "external callers" concerns for a function in a driver-private header
+  with no `RTE_EXPORT_*` annotation
+- `rte_rand()` used for hash seeds, jitter, backoff, load balancing, or test data
 - Mutexes/locks in process-private memory (standard `malloc`, stack, static non-shared) - these don't need `PTHREAD_PROCESS_SHARED`
 - Use of `rte_spinlock_t` or `rte_rwlock_t` in shared memory (these work correctly without special init)
 - `volatile` used for MMIO/hardware register access in drivers (this is correct usage)
@@ -1875,6 +1979,8 @@ devtools/get-maintainer.sh <patch-file>
 - Minor style preferences
 - Optimization suggestions
 - Alternative approaches
+- Pre-existing bugs in surrounding code that the patch does not introduce,
+  labelled as pre-existing so whoever works on that code next can pick them up
 
 ---
 
@@ -1891,7 +1997,7 @@ Example: This could panic if the string is NULL.
 
 ## FINAL CHECK BEFORE SUBMITTING REVIEW
 
-Before outputting your review, do two separate passes:
+Before outputting your review, do three separate passes:
 
 ### Pass 1: Verify correctness bugs are included
 
@@ -1899,7 +2005,8 @@ Ask: "Did I trace every error path for resource leaks?
 Did I check for use-after-free?
 Did I verify error codes are propagated?"
 
-If you identified a potential correctness bug but talked yourself out of it, **add it back**.
+If you identified a potential correctness bug but talked yourself out of it, **add it back**,
+provided you can point to the line that causes it.
 It is better to report a possible bug than to miss a real one.
 
 ### Pass 2: Remove style/process false positives
@@ -1912,4 +2019,18 @@ It should not be in your output.
 An item that says "X is wrong... actually this is correct" is a FALSE POSITIVE and must be removed.
 This applies to style, format, and process items only.
 
-**If your Errors section would be empty after this check, that's fine -- it means the patches are good.**
+### Pass 3: Verify every finding has evidence
+
+For EACH remaining item, of any severity, ask:
+"Did I read the code this claims is broken, or did I reconstruct it?"
+
+If you cannot quote the added or changed line that causes the finding, DELETE IT.
+If the finding is hedged with "presumably", "appears to be", "potentially", or "could be",
+you did not have the evidence: DELETE IT.
+If the finding is about code the patch does not touch, demote it to Info
+and label it as pre-existing -- never an Error or a Warning.
+Demoting the severity does not excuse it from the checks above:
+an Info note you cannot quote a line for is still deleted.
+
+**If your Errors section would be empty after these checks, that's fine --
+it means the patches are good.**
-- 
2.53.0