nss-switch: add new nss-switch tool#29517
Conversation
f0c0864 to
c09c2c0
Compare
|
Ok, I think the review got cut short, but I can perceive a few things:
I look forward to your recommendations, @BKPepe |
|
Exhaustive revision. Time to perform some fixes, but still in need of human revision. |
|
All proposed fixed resolved, also added new implementations (tool is now compatible with MediaTek PPE offloading, even the main target is Qualcomm NSS), cleanups, and other. |
|
A few of proposal corrections were either inaccurate or conflicted with the main purpose of the application. Anyway, these have all been addressed, alongside other fixes or improvements. |
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
- This PR contains two commits (
2e4364aandd9ae4be), both with the identical subjectnss-switch: add new nss-switch tool, and both effectively adding/updating the same new package. Each message accurately describes its own diff, but for a new-package submission OpenWrt expects a single clean commit — please squash these into one before merge.
Inline comments cover one functional issue (incomplete prerm nftables cleanup) plus two minor nits. CI: no checks have run on the head commit yet (combined status is pending, 0 checks), so nothing to flag there.
Generated by Claude Code
openwrt-ai
left a comment
There was a problem hiding this comment.
Fresh review of this new package at 4818d612. Flagged inline: two hex-vs-decimal mark-matching bugs (ecm.sh, conntrack.sh) that silently no-op, two undefined color variables in ui.sh, a ui_hint_bar argument that's discarded, and a validation gap where rules.conf fields are interpolated into an sh-executed script on the regeneration path. Plus two nits (bundled PR changelog, leftover debug comments in the C source). The single commit's message matches its diff.
Generated by Claude Code
| local src_ip="$1" | ||
| cat /proc/net/nf_conntrack 2>/dev/null | \ | ||
| grep "src=$src_ip " | \ | ||
| grep -c "mark=$NSS_MARK" 2>/dev/null; true |
There was a problem hiding this comment.
NSS_MARK defaults to the hex string 0x00010000, but /proc/net/nf_conntrack prints the mark in decimal (mark=65536), so grep -c "mark=$NSS_MARK" never matches and ecm_is_bypassed_by_mark always reports 0. Convert both the per-line mark and NSS_MARK to decimal before comparing, the way the sibling ct_count_bypassed in conntrack.sh already does with printf '%d'.
Generated by Claude Code
| count=$(conntrack -D -m "$NSS_MARK" 2>/dev/null | wc -l) | ||
| else | ||
| # Fallback: buscar y eliminar manualmente | ||
| conntrack -L 2>/dev/null | grep "mark=$NSS_MARK" | while read -r line; do |
There was a problem hiding this comment.
conntrack -L prints the mark in decimal too, so grep "mark=$NSS_MARK" (with NSS_MARK=0x00010000) matches nothing and this manual fallback in _ct_clear_all_marks clears zero connections. Same hex-vs-decimal mismatch as the ecm.sh grep. Convert NSS_MARK with printf '%d' before grepping (and match on the decimal value), as ct_count_bypassed does a few lines above.
Generated by Claude Code
| local proto_c="$C_RESET" | ||
| case "$service_name" in | ||
| HTTP|HTTPS) proto_c="$FG_GREEN" ;; | ||
| SSH|WG) proto_c="$FG_CYAN" ;; |
There was a problem hiding this comment.
$FG_CYAN is never defined. The color block (lines 46-73) defines C_CYAN/C_MAGENTA and the FG_* set (FG_ACCENT/GREEN/RED/YELLOW/ORANGE/BRIGHT/DIM) but no FG_CYAN or FG_MAGENTA, so proto_c is left empty and SSH/WG rows render uncolored.
| SSH|WG) proto_c="$FG_CYAN" ;; | |
| SSH|WG) proto_c="$C_CYAN" ;; |
Same on line 435: $FG_MAGENTA is also undefined — use $C_MAGENTA there.
Generated by Claude Code
| # DEBUG PR-1 | ||
| # ─── Keybind hint bar ────────────────────────────── | ||
| ui_hint_bar() { | ||
| local hints="Ctrl+C / q exit • refresh every ${interval}s • Sorted by: ${sort_name} • 1-6: Sort column" |
There was a problem hiding this comment.
ui_hint_bar discards its argument and rebuilds hints from $interval/$sort_name. Those are only visible here via dynamic scoping from the watch caller (nss-switch.sh:196); the picker caller (ui.sh:607) sets neither, so the picker prints a bogus refresh every s • Sorted by: bar instead of its intended q=cancel / type number hint. Both call sites already pass the full string, so just use it:
| local hints="Ctrl+C / q exit • refresh every ${interval}s • Sorted by: ${sort_name} • 1-6: Sort column" | |
| local hints="$*" |
Generated by Claude Code
| echo 'nft_add_rules() {' >> "$FW_SCRIPT" | ||
|
|
||
| if [ -f "$RULES_FILE" ]; then | ||
| while IFS='|' read -r id proto src_ip dst_ip src_port dst_port iface persist comment; do |
There was a problem hiding this comment.
Fields read from rules.conf here (proto, src_ip, dst_ip, src_port, dst_port, iface) are interpolated unvalidated into the nft commands built by _nft_emit_rule (e.g. meta l4proto ${proto}, ${proto} dport ${dst_port}, ip saddr ${src_ip}), and the generated file is then executed with sh "$FW_SCRIPT" at line 181. rules_validate only runs on the interactive add/pick path, not on this regeneration path, so a hand-edited or malformed rules.conf field (the file is also a conffile) is a command-injection / malformed-command trust boundary — only comment is escaped. Consider validating each field in nft_generate_script/_nft_emit_rule before emitting.
Generated by Claude Code
| @@ -0,0 +1,224 @@ | |||
| # Changelog | |||
There was a problem hiding this comment.
nit: This is a per-PR review-iteration changelog ("Pull Request Review Fixes, Part 2", etc.) rather than user-facing package history, and it is not installed by the Makefile. openwrt/packages generally doesn't carry this kind of development meta-history in the tree — consider dropping it before merge.
Generated by Claude Code
|
|
||
| #define CONNTRACK_FILE "/proc/net/nf_conntrack" | ||
|
|
||
| /* DEBUG PR-1 NO hardcoded values, uci get instead */ |
There was a problem hiding this comment.
nit: For a new package, please strip the development leftovers before merge — the /* DEBUG PR-1 ... */ markers (lines 13, 18, 33, 43, 49, 157, 191, 365), the commented-out macro/prototypes (lines 14, 44-47), and the mixed Spanish debug comments throughout. They don't affect behavior but clutter the source.
Generated by Claude Code
A tool designed to manage Hardware Offload for Qualcomm NSS platforms, to permit a real non-shadowed packet inspection. While optimised for NSS-enabled hardware, it remains compatible with any other platform using Software Offloading (SFE). Signed-off-by: Alexander Gomez <alexandrglm@proton.me>
net/
nss-switchPR📦 Package Details (New package)
Maintainer: Me (Alexander Gomez alexandrglm@proton.me)
Description:
This PR is intended to introduce nss-switch availability in openWRT sources; the tool allows users to selectively bypass Hardware Acceleration (on Qualcomm
qualcommaxplatforms) or Software Offloading (SFE) on any platform, in a visual&intuitive way right from the shell.It works via packet marking, in collaboration with
conntrack, enabling full, non-shadowed, deep packet inspection for specific-desired traffic without losing global offloading benefits.Source info & README can be found here.
🧪 Run Testing Details
✅ Formalities
If your PR contains a patch:
git am(e.g., subject line, commit description, etc.)
We must try to upstream patches to reduce maintenance burden.
Amends
f0c0864: Fixed error in Makefile (
prermPATH), added SPDX header, and applied minor clean-up to thenss-ct-dumpC component (tabs, empty lines).c09c2c0: Fix Makefile (tabs) and removed core dependencies.
148a135: Initial recommendations applied, along with additional cleanups
2e4364a: Fix Makefile
d9ae4be: PR Fix suggestions from 2026-JUL, see CHANGELOG
4818d61: Commits merged/squashed into this one