[PWCI] [ovs-dev,v10] lldp: Add ovs-appctl lldp/neighbor command#4
[PWCI] [ovs-dev,v10] lldp: Add ovs-appctl lldp/neighbor command#4
Conversation
Signed-off-by: Changliang Wu <changliang.wu@smartx.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
New appctl 'lldp/neighbor' displays lldp neighbor information. Support json output with --format json --pretty Signed-off-by: Changliang Wu <changliang.wu@smartx.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
The 802.1 and 802.3 standards list ways for communicating important PoE, Maximum Frame Size, and other important link-level features that peers may want to look at. This commit provides a port of the LLDPD project's protocol decode for 802.1 and 802.3 TLVs of interest, preserving the implementation as closely as possible to the source materials. The supported decodes will allow decoding and displaying: * IEEE 802.1-2005 (Port VLAN and Protocol Configuration) * IEEE 802.3at (Power over Ethernet plus) * IEEE 802.3bt (4-pair PoE) The commit supports displaying this data in human readable and json formats. Signed-off-by: Changliang Wu <changliang.wu@smartx.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Signed-off-by: Changliang Wu <changliang.wu@smartx.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
WalkthroughAdds LLDP feature support and visibility: extends LLDP TLV decoding (Dot1 VLAN/PPVID/PI and Dot3 MAC/PHY/Power incl. 802.3bt), augments port data structures and cleanup, introduces an ovs-appctl unixctl command lldp/neighbor with text/JSON output, adds a nullable string helper and minor colors parsing tweak, updates docs and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant A as ovs-appctl
participant V as ovs-vswitchd (unixctl)
participant L as LLDP subsystem
participant DB as LLDP State (ports/neighbors)
U->>A: run "ovs-appctl [--format json] lldp/neighbor [iface]"
A->>V: unixctl request (lldp/neighbor, args, format)
V->>L: lldp_unixctl_show_neighbor(iface?, json?)
L->>DB: enumerate interfaces with LLDP (filter if iface)
DB-->>L: neighbors per interface
alt JSON requested
L->>V: JSON payload (interfaces[], neighbors[], Dot1/Dot3 fields)
else Text
L->>V: Human-readable text (per-interface neighbors)
end
V-->>A: response
A-->>U: display output
note over L,DB: Neighbor data populated earlier by LLDP TLV parsing<br/>(Dot1 VLAN/PPVID/PI, Dot3 MAC/PHY/Power incl. 802.3bt)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Reviewer's GuideThis PR introduces a new "ovs-appctl lldp/neighbor" command providing detailed LLDP neighbor information in human-readable and JSON formats. It extends the LLDP implementation to decode additional Dot1 and Dot3 TLVs (VLAN, PPVID, PI, MAC/PHY autonegotiation, power), adds corresponding data structures and cleanup routines, and refactors string checks for robustness. Finally, it updates documentation, tests, and build scripts to integrate the new functionality. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `lib/lldp/lldpd-structs.c:116` </location>
<code_context>
}
}
+static void
+lldpd_dot13_lists_cleanup(struct lldpd_port *port)
+{
+ struct lldpd_ppvid *ppvid;
+ struct lldpd_vlan *vlan;
+ struct lldpd_pi *pid;
+ if (port->p_vlans.next && !ovs_list_is_empty(&port->p_vlans)) {
+ LIST_FOR_EACH_SAFE (vlan, v_entries, &port->p_vlans) {
+ ovs_list_remove(&vlan->v_entries);
+ free(vlan->v_name);
+ free(vlan);
+ }
+ ovs_list_init(&port->p_vlans);
+ }
</code_context>
<issue_to_address>
Consider renaming lldpd_dot13_lists_cleanup to lldpd_dot1_lists_cleanup for clarity.
The current name suggests the function handles dot13 lists, but it actually operates on dot1 lists. Renaming will align the function name with its purpose and the naming conventions used elsewhere.
Suggested implementation:
```c
static void
lldpd_dot1_lists_cleanup(struct lldpd_port *port)
```
If this function is called elsewhere in the codebase (other files or other parts of this file), you will need to update those call sites to use the new name `lldpd_dot1_lists_cleanup` instead of `lldpd_dot13_lists_cleanup`.
</issue_to_address>
### Comment 2
<location> `lib/lldp/lldp.c:601` </location>
<code_context>
+ port->p_pvid = PEEK_UINT16;
+ break;
+
+ case LLDP_TLV_DOT1_PPVID:
+ CHECK_TLV_SIZE(7, "PPVID");
+ /* validation needed */
+ /* PPVID has to be unique if more than
+ one PPVID TLVs are received -
+ discard if duplicate */
+ /* if support bit is not set and
+ enabled bit is set - PPVID TLV is
+ considered error and discarded */
+ /* if PPVID > 4096 - bad and discard */
+ ppvid = xzalloc(sizeof *ppvid);
+ ppvid->p_cap_status = PEEK_UINT8;
+ ppvid->p_ppvid = PEEK_UINT16;
+ ovs_list_push_back(&port->p_ppvids, &ppvid->p_entries);
+ break;
+
+ case LLDP_TLV_DOT1_PI:
</code_context>
<issue_to_address>
Add validation for duplicate PPVID TLVs and invalid values.
Please add logic to discard duplicate PPVID TLVs and values exceeding 4096, as well as handle cases where the support bit is not set but the enabled bit is set, to ensure proper validation per the specification.
</issue_to_address>
### Comment 3
<location> `lib/lldp/lldp.c:617` </location>
<code_context>
+ ovs_list_push_back(&port->p_ppvids, &ppvid->p_entries);
+ break;
+
+ case LLDP_TLV_DOT1_PI:
+ /* validation needed */
+ /* PI has to be unique if more than
+ one PI TLVs are received - discard
+ if duplicate ?? */
+ pi = xzalloc(sizeof *pi);
+ pi->p_pi_len = PEEK_UINT8;
+ CHECK_TLV_SIZE(5 + pi->p_pi_len, "PI");
+ pi->p_pi = xzalloc(pi->p_pi_len);
+ PEEK_BYTES(pi->p_pi, pi->p_pi_len);
+ ovs_list_push_back(&port->p_pids, &pi->p_entries);
+ pi = NULL;
+ break;
+
+ default:
</code_context>
<issue_to_address>
Implement uniqueness check for PI TLVs as per comment.
Please add logic to check for and discard duplicate PI TLVs before adding them to port->p_pids to ensure data consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -113,6 +113,37 @@ lldpd_aa_maps_cleanup(struct lldpd_port *port) | |||
| } | |||
There was a problem hiding this comment.
suggestion: Consider renaming lldpd_dot13_lists_cleanup to lldpd_dot1_lists_cleanup for clarity.
The current name suggests the function handles dot13 lists, but it actually operates on dot1 lists. Renaming will align the function name with its purpose and the naming conventions used elsewhere.
Suggested implementation:
static void
lldpd_dot1_lists_cleanup(struct lldpd_port *port)If this function is called elsewhere in the codebase (other files or other parts of this file), you will need to update those call sites to use the new name lldpd_dot1_lists_cleanup instead of lldpd_dot13_lists_cleanup.
| PEEK_BYTES(vlan->v_name, vlan_len); | ||
| ovs_list_push_back(&port->p_vlans, &vlan->v_entries); | ||
| vlan = NULL; | ||
| break; |
There was a problem hiding this comment.
issue (bug_risk): Add validation for duplicate PPVID TLVs and invalid values.
Please add logic to discard duplicate PPVID TLVs and values exceeding 4096, as well as handle cases where the support bit is not set but the enabled bit is set, to ensure proper validation per the specification.
| PEEK_BYTES(vlan->v_name, vlan_len); | ||
| ovs_list_push_back(&port->p_vlans, &vlan->v_entries); | ||
| vlan = NULL; | ||
| break; |
There was a problem hiding this comment.
issue (bug_risk): Implement uniqueness check for PI TLVs as per comment.
Please add logic to check for and discard duplicate PI TLVs before adding them to port->p_pids to ensure data consistency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
lib/lldp/lldpd-structs.h (1)
66-103: Dot3 MAC/PHY and Power structs: confirm units/encoding.Clarify in comments whether 16-bit power values are in watts, 0.1W, or mW and whether they are host-order. Prevents misinterpretation in printers/JSON.
lib/lldp/lldpd-structs.c (2)
122-145: Drop redundant next-pointer checks; rely on ovs_list_is_empty for consistency and safetyUsing
port->p_{vlans,ppvids,pids}.next && !ovs_list_is_empty(...)is redundant and inconsistent withlldpd_aa_maps_cleanup()above, which only usesovs_list_is_empty(). If list heads are always initialized (expected invariant), the extra.nextcheck is unnecessary and obscures intent.Recommend simplifying the three guards to match house style:
- if (port->p_vlans.next && !ovs_list_is_empty(&port->p_vlans)) { + if (!ovs_list_is_empty(&port->p_vlans)) { ... - if (port->p_ppvids.next && !ovs_list_is_empty(&port->p_ppvids)) { + if (!ovs_list_is_empty(&port->p_ppvids)) { ... - if (port->p_pids.next && !ovs_list_is_empty(&port->p_pids)) { + if (!ovs_list_is_empty(&port->p_pids)) {This reduces surface area and aligns with
include/openvswitch/list.hutilities. If there are code paths that may leave these lists uninitialized, we should fix initialization rather than paper over it here.
124-129: Minor: mirror the LIST_FOR_EACH pattern used elsewhereElsewhere in this file we use LIST_FOR_EACH_* macros without pre-checking emptiness; iterating an empty, initialized list is safe. As an optional cleanup, you can drop the
if (!ovs_list_is_empty(...))guards entirely and just reinitialize the list after the loop (keeping current behavior identical).Also applies to: 131-136, 138-144
tests/ovs-lldp.at (3)
15-37: Tighten user-facing text expectations (PPVID line formatting)The expected line reads “PPVID: supported: yes,enabled no” (missing a space and possibly a colon). Consider standardizing to “supported: yes, enabled: no” to improve readability, then update the tests accordingly.
Also applies to: 88-131
39-83: Stabilize JSON schema shape for “interface”For a single neighbor, “interface” is an object; for two neighbors, it becomes an array of objects each repeating “p1”. This type flip complicates consumers.
Consider emitting a consistent shape, e.g.:
- Always object: { "interface": { "p1": [ {neighbor...}, {neighbor...} ] } }
or- Always array with explicit fields: { "interfaces": [ { "name": "p1", "neighbors": [ ... ] } ] }
If you change the producer, please adjust these expectations here.
Also applies to: 133-218
12-14: Frame fixtures: consider brief inline commentsOptional: add a short comment indicating what each crafted frame represents (e.g., “neighbor on Gi1/0/15”, “second neighbor on Gi1/0/5”) to aid future maintainers.
Also applies to: 85-87
lib/ovs-lldp.c (2)
199-199: Minor: Consider consistent null check style.The condition was changed from
c_id_len > 0to truthiness checkc_id_len. While functionally equivalent, maintaining consistency throughout the codebase improves readability.Consider using the explicit comparison for clarity:
- if (port->p_chassis->c_id_len) { + if (port->p_chassis->c_id_len > 0) {
435-475: Repetitive DOT3 autoneg advertisement functions.The
lldp_dot3_autoneg_advertisedandlldp_dot3_autoneg_advertised_jsonfunctions contain repetitive calls with similar parameters. This violates the DRY principle.Consider using a data-driven approach with a lookup table:
+static const struct { + uint16_t hd_bit; + uint16_t fd_bit; + const char *type; +} autoneg_types[] = { + { LLDP_DOT3_LINK_AUTONEG_10BASE_T, LLDP_DOT3_LINK_AUTONEG_10BASET_FD, "10Base-T" }, + { LLDP_DOT3_LINK_AUTONEG_100BASE_T4, LLDP_DOT3_LINK_AUTONEG_100BASE_T4, "100Base-T4" }, + // ... more entries +}; + +static void +lldp_print_all_autoneg_advertised(struct ds *ds, uint16_t pmd_auto_nego) +{ + for (size_t i = 0; i < ARRAY_SIZE(autoneg_types); i++) { + lldp_dot3_autoneg_advertised(ds, pmd_auto_nego, + autoneg_types[i].hd_bit, + autoneg_types[i].fd_bit, + autoneg_types[i].type); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
NEWS(1 hunks)lib/colors.c(1 hunks)lib/lldp/lldp-const.h(1 hunks)lib/lldp/lldp.c(7 hunks)lib/lldp/lldpd-structs.c(2 hunks)lib/lldp/lldpd-structs.h(2 hunks)lib/ovs-lldp.c(5 hunks)lib/util.h(1 hunks)tests/automake.mk(1 hunks)tests/ovs-lldp.at(1 hunks)tests/testsuite.at(1 hunks)vswitchd/ovs-vswitchd.8.in(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
lib/lldp/lldpd-structs.c (1)
include/openvswitch/list.h (3)
ovs_list_is_empty(318-322)ovs_list_remove(249-255)ovs_list_init(147-151)
lib/ovs-lldp.c (7)
lib/dynamic-string.c (6)
ds_put_format(137-145)ds_put_buffer(117-121)ds_cstr_ro(351-355)ds_destroy(368-372)ds_put_hex_with_delimiter(406-419)ds_cstr(341-349)lib/json.c (7)
json_object_create(391-398)json_object_put(416-420)json_integer_create(400-406)json_boolean_create(172-176)json_string_create(187-202)json_array_create_empty(225-234)json_array_add(236-278)include/openvswitch/list.h (1)
ovs_list_size(305-315)lib/util.h (1)
nullable_string_not_empty(200-204)lib/packets.h (1)
in6_addr_set_mapped_ipv4(1218-1222)lib/packets.c (2)
ipv6_format_mapped(926-935)ipv6_string_mapped(956-966)lib/unixctl.c (3)
unixctl_command_get_output_format(221-225)unixctl_command_reply_json(291-296)unixctl_command_reply(265-282)
lib/lldp/lldp.c (4)
lib/util.h (1)
nullable_string_not_empty(200-204)lib/dp-packet.c (1)
dp_packet_put(367-373)include/openvswitch/list.h (2)
ovs_list_init(147-151)ovs_list_push_back(202-206)lib/util.c (1)
xzalloc(165-170)
🔇 Additional comments (21)
vswitchd/ovs-vswitchd.8.in (1)
152-156: LGTM: new lldp/neighbor command doc is clear and consistent.Matches style of existing interface-scoped commands.
lib/util.h (1)
200-205: LGTM: small, reusable helper.Inline, null-safe, and improves readability at call sites.
lib/colors.c (1)
120-120: LGTM: clearer loop guard via helper.Semantics unchanged; inlined helper keeps it zero-cost.
lib/lldp/lldpd-structs.h (2)
48-65: No missing Dot1 list handling: p_vlans, p_ppvids and p_pids are initialized in port creation, deep-freed in lldpd_dot13_lists_cleanup (invoked by lldpd_port_cleanup on normal and early-exit paths), and ports are not cloned elsewhere, so no additional deep-copy is needed.
159-169: Remove stale p_id_subtype boundary comment Port‐change logic only compares port ID and chassis fields via explicit checks (memcmp on p_id and chassis ID), so the Dot1/Dot3 fields placement has no effect on change detection; update or remove the misleading comment in lib/lldp/lldpd-structs.h.Likely an incorrect or invalid review comment.
lib/lldp/lldpd-structs.c (1)
161-161: Good integration into port cleanup pathHooking
lldpd_dot13_lists_cleanup(port)right afterlldpd_aa_maps_cleanup(port)keeps all nested Dot1/Dot3 per-port lists cleaned in one place. LGTM.tests/testsuite.at (1)
69-69: Wire-up looks correctIncluding
tests/ovs-lldp.athere integrates the new LLDP tests into the main suite in the expected spot. LGTM.tests/automake.mk (1)
71-71: Added test is properly listed in TESTSUITE_ATThis ensures the LLDP tests are generated and distributed. LGTM.
lib/lldp/lldp-const.h (1)
101-105: Verify 802.3bt type values match spec and internal usageNew bt-type constants reuse values 0..2 (OFF/TYPE3/TYPE4). In PoE nomenclature, “Type 3/4” are often literal 3/4. If these are internal enums scoped to a separate field from 802.3at types, reusing 0..2 is fine; otherwise it may be ambiguous.
Please confirm they:
- Match 802.3bt TLV semantics, and
- Don’t collide with any consumer that expects “3” or “4”.
If needed, add a brief comment clarifying that these are internal enum values (not literal PoE type numbers).
tests/ovs-lldp.at (1)
5-11: Good focused scenario setupUsing netdev-dummy and enabling LLDP per-interface keeps the test hermetic. LGTM.
lib/ovs-lldp.c (5)
39-39: LGTM! JSON header inclusion for new neighbor command support.The addition of
openvswitch/json.his appropriate for implementing the JSON output functionality in the newlldp/neighborcommand.
41-42: LGTM! Necessary headers for LLDP DOT1/DOT3 TLV support.The inclusion of
lldp-const.handlldp-tlv.hprovides the required constants and definitions for DOT1/DOT3 TLV parsing and reporting.
626-748: Well-structured text output for LLDP neighbors.The function properly handles multiple interfaces and provides comprehensive neighbor information including DOT1 and DOT3 details.
1012-1051: LGTM! Clean implementation of the new unixctl command.The
lldp_unixctl_show_neighborfunction properly handles both JSON and text output formats, with appropriate filtering by interface name when provided.
1320-1321: LGTM! Proper registration of the new unixctl command.The new
lldp/neighborcommand is correctly registered with optional interface filtering support.lib/lldp/lldp.c (6)
21-23: LGTM! Required headers for enhanced LLDP TLV support.The addition of headers for constants, TLV handling, and utility functions is appropriate for the DOT1/DOT3 parsing implementation.
197-197: Good improvement: Using nullable string helper.The change to use
nullable_string_not_empty()is more readable and safer than the previous empty-or-NULL check.
204-204: Consistent use of nullable string checks.Good consistency in applying
nullable_string_not_empty()across all relevant TLV emissions.Also applies to: 237-237
396-398: LGTM! Proper initialization of DOT1/DOT3 lists.The initialization of the new per-port lists for VLANs, PPVIDs, and PIDs is correctly placed and follows the existing pattern.
639-734: Comprehensive DOT3 TLV parsing implementation.The DOT3 parsing correctly handles MAC/PHY, link aggregation, MFS, and power TLVs including 802.3at and 802.3bt extensions. The bit-field extraction for power fields is properly implemented.
631-637: Improved error handling with specific unrecognized count tracking.Good addition of incrementing
h_rx_unrecognized_cntfor unknown DOT1 subtypes, providing better diagnostics.
| struct lldpd_vlan *vlan = NULL; | ||
| struct lldpd_pi *pi = NULL; | ||
| struct lldpd_ppvid *ppvid; | ||
| int vlan_len; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify memory cleanup for DOT1 structures.
The addition of DOT1 structure variables is necessary for parsing. However, ensure these are properly freed in all error paths.
🏁 Script executed:
#!/bin/bash
# Check for proper cleanup of DOT1 structures in error paths
ast-grep --pattern 'free($VAR)' lib/lldp/lldp.c | grep -E "vlan|pi|ppvid"
# Also check the cleanup function
rg -A 10 "lldpd_port_cleanup" lib/lldp/Length of output: 3623
Free DOT1 structures in all cleanup paths.
- In the error path around lines 379–382 of
lib/lldp/lldp.c, callfree(vlan),free(pi)andfree(ppvid)(and set them to NULL) before returning. - In
lldpd_port_cleanup(lib/lldp/lldpd-structs.c), addfree(port->p_vlan),free(port->p_pi)andfree(port->p_ppvid)(and NULL them) to avoid leaks.
🤖 Prompt for AI Agents
In lib/lldp/lldp.c around lines 379-382, the DOT1-related heap objects vlan, pi
and ppvid are not freed on the error path; before returning on error call
free(vlan), free(pi) and free(ppvid) and set each pointer to NULL to avoid
leaks. In lib/lldp/lldpd-structs.c inside lldpd_port_cleanup, add
free(port->p_vlan), free(port->p_pi) and free(port->p_ppvid) and then set
port->p_vlan = port->p_pi = port->p_ppvid = NULL so allocated DOT1 structures
are always released during port cleanup.
| switch (tlv_subtype) { | ||
| case LLDP_TLV_DOT1_VLANNAME: | ||
| CHECK_TLV_SIZE(7, "VLAN"); | ||
| vlan = xzalloc(sizeof *vlan); | ||
| vlan->v_vid = PEEK_UINT16; | ||
| vlan_len = PEEK_UINT8; | ||
| CHECK_TLV_SIZE(7 + vlan_len, "VLAN"); | ||
| vlan->v_name = xzalloc(vlan_len + 1); | ||
| PEEK_BYTES(vlan->v_name, vlan_len); | ||
| ovs_list_push_back(&port->p_vlans, &vlan->v_entries); | ||
| vlan = NULL; | ||
| break; | ||
|
|
||
| case LLDP_TLV_DOT1_PVID: | ||
| CHECK_TLV_SIZE(6, "PVID"); | ||
| port->p_pvid = PEEK_UINT16; | ||
| break; | ||
|
|
||
| case LLDP_TLV_DOT1_PPVID: | ||
| CHECK_TLV_SIZE(7, "PPVID"); | ||
| /* validation needed */ | ||
| /* PPVID has to be unique if more than | ||
| one PPVID TLVs are received - | ||
| discard if duplicate */ | ||
| /* if support bit is not set and | ||
| enabled bit is set - PPVID TLV is | ||
| considered error and discarded */ | ||
| /* if PPVID > 4096 - bad and discard */ | ||
| ppvid = xzalloc(sizeof *ppvid); | ||
| ppvid->p_cap_status = PEEK_UINT8; | ||
| ppvid->p_ppvid = PEEK_UINT16; | ||
| ovs_list_push_back(&port->p_ppvids, &ppvid->p_entries); | ||
| break; | ||
|
|
||
| case LLDP_TLV_DOT1_PI: | ||
| /* validation needed */ | ||
| /* PI has to be unique if more than | ||
| one PI TLVs are received - discard | ||
| if duplicate ?? */ | ||
| pi = xzalloc(sizeof *pi); | ||
| pi->p_pi_len = PEEK_UINT8; | ||
| CHECK_TLV_SIZE(5 + pi->p_pi_len, "PI"); | ||
| pi->p_pi = xzalloc(pi->p_pi_len); | ||
| PEEK_BYTES(pi->p_pi, pi->p_pi_len); | ||
| ovs_list_push_back(&port->p_pids, &pi->p_entries); | ||
| pi = NULL; | ||
| break; | ||
|
|
||
| default: | ||
| VLOG_INFO("unknown org tlv [%02x:%02x:%02x] received " | ||
| "on %s", | ||
| orgid[0], orgid[1], orgid[2], | ||
| hardware->h_ifname); | ||
| hardware->h_rx_unrecognized_cnt++; | ||
| } |
There was a problem hiding this comment.
Complex DOT1 TLV parsing with potential buffer overflow risks.
The DOT1 parsing code handles variable-length fields but needs careful validation:
- Line 588: VLAN name length is read but the subsequent check might overflow if
vlan_lenis very large - Line 625: PI length validation looks correct but should ensure the allocation doesn't overflow
Apply defensive bounds checking:
case LLDP_TLV_DOT1_VLANNAME:
CHECK_TLV_SIZE(7, "VLAN");
vlan = xzalloc(sizeof *vlan);
vlan->v_vid = PEEK_UINT16;
vlan_len = PEEK_UINT8;
+ if (vlan_len > 32) { // Reasonable max for VLAN name
+ VLOG_WARN("VLAN name too long (%d) on %s",
+ vlan_len, hardware->h_ifname);
+ free(vlan);
+ goto malformed;
+ }
CHECK_TLV_SIZE(7 + vlan_len, "VLAN");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (tlv_subtype) { | |
| case LLDP_TLV_DOT1_VLANNAME: | |
| CHECK_TLV_SIZE(7, "VLAN"); | |
| vlan = xzalloc(sizeof *vlan); | |
| vlan->v_vid = PEEK_UINT16; | |
| vlan_len = PEEK_UINT8; | |
| CHECK_TLV_SIZE(7 + vlan_len, "VLAN"); | |
| vlan->v_name = xzalloc(vlan_len + 1); | |
| PEEK_BYTES(vlan->v_name, vlan_len); | |
| ovs_list_push_back(&port->p_vlans, &vlan->v_entries); | |
| vlan = NULL; | |
| break; | |
| case LLDP_TLV_DOT1_PVID: | |
| CHECK_TLV_SIZE(6, "PVID"); | |
| port->p_pvid = PEEK_UINT16; | |
| break; | |
| case LLDP_TLV_DOT1_PPVID: | |
| CHECK_TLV_SIZE(7, "PPVID"); | |
| /* validation needed */ | |
| /* PPVID has to be unique if more than | |
| one PPVID TLVs are received - | |
| discard if duplicate */ | |
| /* if support bit is not set and | |
| enabled bit is set - PPVID TLV is | |
| considered error and discarded */ | |
| /* if PPVID > 4096 - bad and discard */ | |
| ppvid = xzalloc(sizeof *ppvid); | |
| ppvid->p_cap_status = PEEK_UINT8; | |
| ppvid->p_ppvid = PEEK_UINT16; | |
| ovs_list_push_back(&port->p_ppvids, &ppvid->p_entries); | |
| break; | |
| case LLDP_TLV_DOT1_PI: | |
| /* validation needed */ | |
| /* PI has to be unique if more than | |
| one PI TLVs are received - discard | |
| if duplicate ?? */ | |
| pi = xzalloc(sizeof *pi); | |
| pi->p_pi_len = PEEK_UINT8; | |
| CHECK_TLV_SIZE(5 + pi->p_pi_len, "PI"); | |
| pi->p_pi = xzalloc(pi->p_pi_len); | |
| PEEK_BYTES(pi->p_pi, pi->p_pi_len); | |
| ovs_list_push_back(&port->p_pids, &pi->p_entries); | |
| pi = NULL; | |
| break; | |
| default: | |
| VLOG_INFO("unknown org tlv [%02x:%02x:%02x] received " | |
| "on %s", | |
| orgid[0], orgid[1], orgid[2], | |
| hardware->h_ifname); | |
| hardware->h_rx_unrecognized_cnt++; | |
| } | |
| case LLDP_TLV_DOT1_VLANNAME: | |
| CHECK_TLV_SIZE(7, "VLAN"); | |
| vlan = xzalloc(sizeof *vlan); | |
| vlan->v_vid = PEEK_UINT16; | |
| vlan_len = PEEK_UINT8; | |
| /* Reject excessively long VLAN names to prevent over-allocation | |
| * and enforce the 802.1 standard maximum of 32 bytes. */ | |
| if (vlan_len > 32) { | |
| VLOG_WARN("VLAN name too long (%d) on %s", | |
| vlan_len, hardware->h_ifname); | |
| free(vlan); | |
| goto malformed; | |
| } | |
| CHECK_TLV_SIZE(7 + vlan_len, "VLAN"); | |
| vlan->v_name = xzalloc(vlan_len + 1); | |
| PEEK_BYTES(vlan->v_name, vlan_len); | |
| ovs_list_push_back(&port->p_vlans, &vlan->v_entries); | |
| vlan = NULL; | |
| break; |
| static void | ||
| lldp_print_neighbor_port_dot1(struct ds *ds, struct lldpd_port *port) | ||
| { | ||
| struct lldpd_ppvid *ppvid; | ||
| struct lldpd_vlan *vlan; | ||
| struct lldpd_pi *pid; | ||
|
|
||
| LIST_FOR_EACH (vlan, v_entries, &port->p_vlans) { | ||
| ds_put_format(ds, " %-20s%d, %s", "VLAN:", vlan->v_vid, | ||
| port->p_pvid == vlan->v_vid ? "pvid: yes" : "pvid: no"); | ||
| if (vlan->v_name) { | ||
| ds_put_format(ds, ", %s", vlan->v_name); | ||
| } | ||
| ds_put_format(ds, "\n"); | ||
| } | ||
|
|
||
| LIST_FOR_EACH (ppvid, p_entries, &port->p_ppvids) { | ||
| ds_put_format(ds, " %-20s", "PPVID:"); | ||
| if (ppvid->p_ppvid) { | ||
| ds_put_format(ds, ", %d ", ppvid->p_ppvid); | ||
| } | ||
| ds_put_format( | ||
| ds, "supported: %s,enabled %s\n", | ||
| (ppvid->p_cap_status & LLDP_PPVID_CAP_SUPPORTED) ? "yes" : "no", | ||
| (ppvid->p_cap_status & LLDP_PPVID_CAP_ENABLED) ? "yes" : "no"); | ||
| } | ||
|
|
||
| LIST_FOR_EACH (pid, p_entries, &port->p_pids) { | ||
| if (pid->p_pi && pid->p_pi_len > 0) { | ||
| ds_put_format(ds, " %-20s", "PI:"); | ||
| ds_put_buffer(ds, pid->p_pi, pid->p_pi_len); | ||
| ds_put_format(ds, "\n"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| static void | ||
| lldp_print_neighbor_port_dot1_json(struct json *interface_item_json, | ||
| struct lldpd_port *port) | ||
| { | ||
| struct json *vlan_json_array = NULL; | ||
| struct json *vlan_json = NULL; | ||
| struct json *ppvid_json_array = NULL; | ||
| struct json *ppvid_json = NULL; | ||
| struct json *pi_json_array = NULL; | ||
| struct json *pi_json = NULL; | ||
|
|
||
| struct lldpd_ppvid *ppvid; | ||
| struct lldpd_vlan *vlan; | ||
| struct lldpd_pi *pid; | ||
|
|
||
| LIST_FOR_EACH (vlan, v_entries, &port->p_vlans) { | ||
| vlan_json = json_object_create(); | ||
| json_object_put(vlan_json, "vlan-id", | ||
| json_integer_create(vlan->v_vid)); | ||
| json_object_put(vlan_json, "pvid", | ||
| json_boolean_create(port->p_pvid == vlan->v_vid)); | ||
| if (vlan->v_name) { | ||
| json_object_put(vlan_json, "value", | ||
| json_string_create(vlan->v_name)); | ||
| } | ||
| if (ovs_list_size(&port->p_vlans) > 1) { | ||
| if (vlan_json_array == NULL) { | ||
| vlan_json_array = json_array_create_empty(); | ||
| } | ||
| json_array_add(vlan_json_array, vlan_json); | ||
| } | ||
| } | ||
| if (vlan_json_array || vlan_json) { | ||
| json_object_put(interface_item_json, "vlan", | ||
| vlan_json_array ? vlan_json_array : vlan_json); | ||
| } | ||
|
|
||
| LIST_FOR_EACH (ppvid, p_entries, &port->p_ppvids) { | ||
| ppvid_json = json_object_create(); | ||
| if (ppvid->p_ppvid) { | ||
| json_object_put(ppvid_json, "ppvid", | ||
| json_integer_create(ppvid->p_ppvid)); | ||
| } | ||
| json_object_put(ppvid_json, "supported", | ||
| json_boolean_create(ppvid->p_cap_status & | ||
| LLDP_PPVID_CAP_SUPPORTED)); | ||
| json_object_put(ppvid_json, "enabled", | ||
| json_boolean_create(ppvid->p_cap_status & | ||
| LLDP_PPVID_CAP_ENABLED)); | ||
| if (ovs_list_size(&port->p_ppvids) > 1) { | ||
| if (ppvid_json_array == NULL) { | ||
| ppvid_json_array = json_array_create_empty(); | ||
| } | ||
| json_array_add(ppvid_json_array, ppvid_json); | ||
| } | ||
| } | ||
| if (ppvid_json_array || ppvid_json) { | ||
| json_object_put(interface_item_json, "ppvid", | ||
| ppvid_json_array ? ppvid_json_array : ppvid_json); | ||
| } | ||
|
|
||
| LIST_FOR_EACH (pid, p_entries, &port->p_pids) { | ||
| if (pid->p_pi && pid->p_pi_len > 0) { | ||
| pi_json = json_object_create(); | ||
| struct ds pi_ds = DS_EMPTY_INITIALIZER; | ||
| ds_put_buffer(&pi_ds, pid->p_pi, pid->p_pi_len); | ||
| json_object_put(pi_json, "pi", | ||
| json_string_create(ds_cstr_ro(&pi_ds))); | ||
| ds_destroy(&pi_ds); | ||
| if (ovs_list_size(&port->p_pids) > 1) { | ||
| if (pi_json_array == NULL) { | ||
| pi_json_array = json_array_create_empty(); | ||
| } | ||
| json_array_add(pi_json_array, pi_json); | ||
| } | ||
| } | ||
| } | ||
| if (pi_json_array || pi_json) { | ||
| json_object_put(interface_item_json, "pi", | ||
| pi_json_array ? pi_json_array : pi_json); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complex nested JSON construction with potential memory leaks.
The function creates multiple JSON objects but doesn't handle cleanup consistently if an error occurs during construction. Additionally, the complex nesting logic with conditional arrays makes the code hard to follow and maintain.
Consider:
- Adding error handling to ensure JSON objects are properly freed on failure
- Breaking down the function into smaller helper functions for each TLV type
- Using a consistent pattern for handling single vs. multiple items
Example refactor for better structure:
+static struct json *
+lldp_create_vlan_json(struct lldpd_vlan *vlan, uint16_t pvid)
+{
+ struct json *vlan_json = json_object_create();
+ json_object_put(vlan_json, "vlan-id",
+ json_integer_create(vlan->v_vid));
+ json_object_put(vlan_json, "pvid",
+ json_boolean_create(pvid == vlan->v_vid));
+ if (vlan->v_name) {
+ json_object_put(vlan_json, "value",
+ json_string_create(vlan->v_name));
+ }
+ return vlan_json;
+}Committable suggestion skipped: line range outside the PR's diff.
| - ovs-vsctl: | ||
| * Added JSON output support to the 'ovs/route/show' command. | ||
| * Added 'lldp/neighbor' command that displays lldp neighbor information. | ||
| - lldp: | ||
| * Added support for decoding dot1 and dot3 TLVs. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix component and duplication; capitalize LLDP.
- The new lldp/neighbor is an ovs-appctl command, not ovs-vsctl.
- "ovs/route/show" JSON support already appears under v3.6.0’s ovs-appctl section; don’t repeat it post-v3.6.0 unless there’s a new change.
- Use “LLDP” capitalization.
Apply:
- - ovs-vsctl:
- * Added JSON output support to the 'ovs/route/show' command.
- * Added 'lldp/neighbor' command that displays lldp neighbor information.
+ - ovs-appctl:
+ * Added 'lldp/neighbor' command that displays LLDP neighbor information.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - ovs-vsctl: | |
| * Added JSON output support to the 'ovs/route/show' command. | |
| * Added 'lldp/neighbor' command that displays lldp neighbor information. | |
| - lldp: | |
| * Added support for decoding dot1 and dot3 TLVs. | |
| - ovs-appctl: | |
| * Added 'lldp/neighbor' command that displays LLDP neighbor information. | |
| - lldp: | |
| * Added support for decoding dot1 and dot3 TLVs. |
🤖 Prompt for AI Agents
In NEWS around lines 6 to 10, the entries misattribute and duplicate items and
use wrong capitalization: move the "lldp/neighbor" bullet into the ovs-appctl
section (not ovs-vsctl), remove or omit the "ovs/route/show" JSON-support line
here if that change is already listed under v3.6.0's ovs-appctl (do not
duplicate), and change "lldp" and any TLV mentions to uppercase "LLDP" (e.g.,
"LLDP/neighbor" and "dot1 and dot3 TLVs"); update the section header and bullets
so the LLDP command appears under ovs-appctl with correct capitalization and no
duplicated ovs/route/show entry.
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
When vport_add_channel() is called duplicate, the resources for previously
specified sock was not freed. This patch fixes this issue.
Reported by Address Sanitizer.
Direct leak of 60 byte(s) in 3 object(s) allocated from:
#0 0xffffb3658080 in malloc (/usr/lib64/libasan.so.6+0xa9080)
#1 0x922630 in xmalloc__ lib/util.c:141
#2 0x922718 in xmalloc lib/util.c:176
#3 0x9c67e4 in nl_sock_create lib/netlink-socket.c:147
#4 0x94cb6c in create_nl_sock lib/dpif-netlink.c:283
#5 0x950bec in dpif_netlink_port_add__ lib/dpif-netlink.c:978
#6 0x951a20 in dpif_netlink_port_add_compat lib/dpif-netlink.c:1101
#7 0x951cd0 in dpif_netlink_port_add lib/dpif-netlink.c:1147
#8 0x616354 in dpif_port_add lib/dpif.c:602
#9 0x49f424 in port_add ofproto/ofproto-dpif.c:4144
#10 0x44d51c in ofproto_port_add ofproto/ofproto.c:2204
#11 0x416914 in iface_do_create vswitchd/bridge.c:2203
#12 0x416dbc in iface_create vswitchd/bridge.c:2246
#13 0x40e1d0 in bridge_add_ports__ vswitchd/bridge.c:1225
#14 0x40e290 in bridge_add_ports vswitchd/bridge.c:1241
#15 0x40cc6c in bridge_reconfigure vswitchd/bridge.c:952
#16 0x420884 in bridge_run vswitchd/bridge.c:3440
#17 0x42f3d0 in main vswitchd/ovs-vswitchd.c:137
Reproduce steps:
ovs-vsctl add-br br-ovs
ovs-vsctl add-port br-ovs test -- set interface test type=internal
ip netns add ns_test
ip link set test netns ns_test
ip netns del ns_test
ifconfig br-ovs up
sleep 1
ifconfig br-ovs down
sleep 1
ovs-vsctl del-br br-ovs
Fixes: 69c5158 ("dpif-netlink: don't allocate per thread netlink sockets")
Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: 0-day Robot <robot@bytheb.org>
2 participants
NOTE: This is an auto submission for
Summary by Sourcery
Add
ovs-appctl lldp/neighborcommand to display LLDP neighbor details per interface in both text and JSON formats, and extend the LLDP agent to parse, store, and present additional 802.1 and 802.3 TLVs.New Features:
lldp/neighborunixctl command with text and JSON outputEnhancements:
Tests:
tests/ovs-lldp.atto exercise the new LLDP neighbor commandSummary by CodeRabbit
New Features
Documentation
Tests