lib, zebra: proper PtP peer prefixes across ZAPI#22313
Conversation
We were (1) losing the peer prefix length, (2) couldn't support the same address with more than one peer prefix (which to be fair will probably break protocols), and (3) were possibly deleting the wrong thing, since that lookup was without the peer prefix. Straighten all of this out, properly carry peer prefix across ZAPI. Reported-by: Olivier Cochard-Labbé <olivier@cochard.me> Signed-off-by: David 'equinox' Lamparter <equinox@opensourcerouting.org>
The comment was incorrectly stating that there's a byte length field after the address, with an example "4" for IPv4. It's a prefix length field. Also add it for the destination prefix. Signed-off-by: David 'equinox' Lamparter <equinox@opensourcerouting.org>
cb2548f to
1cc6663
Compare
Greptile SummaryThis PR fixes PtP (point-to-point) peer prefix handling across the ZAPI wire protocol: the destination/peer prefix length was previously dropped, multiple entries with the same address but different peers could collide, and deletes could match the wrong entry. The fix threads a
Confidence Score: 4/5The core logic is sound and the version bump prevents protocol mismatches with old clients; safe to merge with the noted edge case acknowledged. The wire-format change is well-scoped: sender and receiver are symmetric, ZSERV_VERSION is correctly bumped, and the only active callers of the changed lib/if.c functions are updated. The 0/0 NULL-sentinel ambiguity is a pre-existing edge case acknowledged in the existing carp/OpenBSD comment. A minor comment typo and the unresolved /0 sentinel question keep this just below a clean score. lib/zclient.c — NULL-sentinel check and the comment typo; worth a second look before merging. Important Files Changed
Sequence DiagramsequenceDiagram
participant Z as zebra (zapi_msg.c)
participant W as wire (ZAPI v7)
participant C as client (zclient.c)
participant I as if.c
Note over Z: ifc->destination present?
Z->>W: stream_put(dest.u.prefix, blen)
Z->>W: stream_putc(dest.prefixlen)
Note over Z: ifc->destination == NULL
Z->>W: stream_put(NULL, blen+1) [all-zero sentinel]
W->>C: "zclient_stream_get_prefix(&p) [address + plen]"
W->>C: "zclient_stream_get_prefix(&d) [dest bytes + dplen]"
C->>C: "dp = (d.prefixlen==0 && all-zeros) ? NULL : &d"
alt ZEBRA_INTERFACE_ADDRESS_ADD
C->>I: "connected_lookup_prefix_exact(ifp, &p, dp)"
I-->>C: ifc or NULL
C->>I: "connected_add_by_prefix(ifp, &p, dp) [if not found]"
else ZEBRA_INTERFACE_ADDRESS_DELETE
C->>I: "connected_delete_by_prefix(ifp, &p, dp)"
I->>I: connected_lookup_prefix_exact(ifp, p, dest)
I->>I: if_connected_del(...)
end
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
lib/zclient.c:3194
**Typo in comment: "prefifxlen"**
The label reads `prefifxlen` — the `f` and `x` are transposed. Should be `prefixlen` to match the terminology used throughout the rest of the message format diagram.
### Issue 2 of 2
lib/zclient.c:3249
**NULL destination indistinguishable from a real 0.0.0.0/0 or ::/0 peer**
When a PtP interface legitimately carries a destination of `0.0.0.0/0` (all-zero address with `prefixlen == 0`), the sender writes four zero bytes followed by a zero prefixlen byte — identical to the NULL encoding produced by `stream_put(s, NULL, blen + 1)`. The receiver will therefore set `dp = NULL` and treat the valid peer prefix as absent. This ambiguity was partly present before this PR (the old receiver decoded only the raw bytes with the same `memconstant` check), but adding the `d.prefixlen == 0` guard does not fully resolve it: the condition is true for both the null sentinel *and* a real `/0` destination, so the two cases remain conflated. The existing "carp interfaces on OpenBSD with 0.0.0.0/0 as peer" error path confirms this edge case is known; consider reserving a distinct sentinel (e.g. a special flag bit, or always encoding the destination family separately) to make NULL unambiguous.
Reviews (1): Last reviewed commit: "lib: fix incorrect comment re. ZAPI enco..." | Re-trigger Greptile |
| continue; | ||
| if (!ifc->destination && !dest) | ||
| return ifc; | ||
| if (ifc->destination && dest && prefix_same(ifc->destination, dest)) |
There was a problem hiding this comment.
we don't need to test for ifc->destination && dest now right? 1008 tests for that
| #define ZAPI_MESSAGE_OPAQUE 0x0400 | ||
|
|
||
| #define ZSERV_VERSION 6 | ||
| #define ZSERV_VERSION 7 |
There was a problem hiding this comment.
I wonder how long before we cycle through the version space :)
We were (1) losing the peer prefix length, (2) couldn't support the same
address with more than one peer prefix (which to be fair will probably
break protocols), and (3) were possibly deleting the wrong thing, since
that lookup was without the peer prefix.
Straighten all of this out, properly carry peer prefix across ZAPI.
more comprehensive fix for the first commit in #22129. The 2nd and 3rd commit from that PR are still needed.
Haven't really tested this, let's see if it blows up in CI.