Skip to content

miniupnpd-nftables: reduce hook weight - #27623

Open
brada4 wants to merge 1 commit into
openwrt:masterfrom
brada4:ctpnp
Open

miniupnpd-nftables: reduce hook weight#27623
brada4 wants to merge 1 commit into
openwrt:masterfrom
brada4:ctpnp

Conversation

@brada4

@brada4 brada4 commented Oct 8, 2025

Copy link
Copy Markdown
Contributor

Reduce usage of resources of miniupnp ruleset

  • do payload matching only once per connection state
  • trust dnat mark to forward packet
  • leave dangling unreachable resources, their only function is to sink miniupnpd logging their absence on every client request.

Fixes: #29712

📦 Package Details

Maintainer: @Self-Hosting-Group

Description: Forwards ports, post PR using less resources to do so

🧪 Run Testing Details

  • 24 and SNAPSHOT and 25
  • three
  • four of them

✅ Formalities

  • [ x] I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

If your PR contains a patch:

  • [ x] It can be applied using git am
  • [ x] It has been refreshed to avoid offsets, fuzzes, etc., using
    make package/<your-package>/refresh V=s
  • [ x] It is structured in a way that it is potentially upstreamable
    The idea is upstreamable, patched ruleset hooks bypass misteaks

@brada4

brada4 commented Oct 8, 2025

Copy link
Copy Markdown
Contributor Author

semicolon at eof is anachronism from when nftable include was concatenation.

@yangfl

yangfl commented Oct 8, 2025

Copy link
Copy Markdown
Contributor

I'm not the maintainer. Also, you might be interested in other undergoing miniupnpd PRs.

@brada4

brada4 commented Oct 8, 2025

Copy link
Copy Markdown
Contributor Author

Independent from those. But thanks for advice.

@Neustradamus

Copy link
Copy Markdown

@brada4: @Self-Hosting-Group works on miniupnpd too.

@Self-Hosting-Group

Copy link
Copy Markdown
Contributor

@brada4 Although I am not officially a maintainer, I would rewrite a large part of the code with the package revisions PR. Yes, the upnp_forward and upnp_prerouting chains seem to be used, but, as you correctly describe, it looks like the upnp_postrouting chain is not. So why are we configuring it in the daemon config at all? I would remove it if it is not needed, but it would be a good idea to check with the daemon maintainer first. Perhaps its documentation should also be updated.
https://github.com/miniupnp/miniupnp/blob/master/miniupnpd/miniupnpd.conf#L95-L104

@brada4

brada4 commented Nov 12, 2025

Copy link
Copy Markdown
Contributor Author

A If you remove parameter it takes default value, never accessed, but you will never know. Patching it out and maintaining would be excess work.

B Rationale about forward - if you add a single dnat rule then status dnat is added in all traversed chains, I do same. Can be chain-pre even (subject to re-arrangement in my own making ) at present you can only prepend before ct state dispatch which would add 1mm damage to general traffic.

C It does a weird matching all attributes in payload, so I guard it to push only new connections through essentially conntrack reimplementation. You could take them all as immediate ct meta https://wiki.nftables.org/wiki-nftables/index.php/Matching_connection_tracking_stateful_metainformation#Conntrack-assigned_metadata
Again "downstream" patch would be a burden.

(you do not carry the badge of maintainer, but as a matter of fact you are the only one doing the practical plumbing part)

@brada4

brada4 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Formality Check: Failed

We completed the verification flow. Please review the formatting overview logs below.

🛑 CRITICAL ERRORS

Commit 96ef0d4 - miniupnpd-nftables: reduce hook weight:

  • Makefile introduces PKG_VERSION '2.3.9', but this version string is missing in the commit subject line

Something broken? Consider reporting an issue.Running version 1f61a85 deployed on 2026-06-30 23:38:02 CEST

No shit sherlock, rebase from web ui cripples commit author.

@brada4
brada4 force-pushed the ctpnp branch 2 times, most recently from 7477863 to 7c7f8da Compare July 1, 2026 18:04
@brada4

brada4 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Dont you dare to complain about removed version bump.

@brada4

brada4 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Dear OpenWRT devlopers,
Please unsubscribe me from YOUR ci timeout notifications.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 commit; inline comments below. Not a backport; the commit message accurately describes the ruleset changes, so no commit-message discrepancies. CI is green.


Generated by Claude Code

Comment thread net/miniupnpd/files/nftables.d/chain-post/dstnat/20-miniupnpd.nft
Comment thread net/miniupnpd/files/nftables.d/chain-post/forward/20-miniupnpd.nft
@openwrt

openwrt Bot commented Jul 10, 2026

Copy link
Copy Markdown

Formality Check: Suggestions Available

We checked this pull request against the contribution guidelines. Here is what needs your attention:

⚠️ STYLISTIC WARNINGS & SUGGESTIONS

Package Release Audit:

  • ⚠️ Content changed in these packages, but without a PKG_RELEASE or version bump:

    • net/miniupnpd

    Increment PKG_RELEASE by 1 (or bump PKG_VERSION/PKG_SOURCE_DATE and reset PKG_RELEASE to 1) so users receive the update.

Do not increment release for minor changes. Cosmetic edits (e.g., typos in comments, copyright updates, formatting/whitespace), changing the package maintainer (PKG_MAINTAINER), or updating source download info (PKG_SOURCE_URL / PKG_HASH) do not require incrementing PKG_RELEASE.


Something broken? Consider reporting an issue.
Running version 21db518 deployed on 2026-08-16 10:59:44 CEST

@brada4

brada4 commented Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

without a PKG_RELEASE

Thats not what other clanker said.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the updated revision. The tree is byte-identical to the previously reviewed commit (7c7f8da); only the commit message changed, so the earlier inline comments still apply and no new code issues were found.

Commit checks

  • The reworded message drops the bullet that documented the prerouting hook change, but the diff still adds ct state new to chain-post/dstnat/20-miniupnpd.nft (restricting the jump upnp_prerouting to new connections). Consider restoring a line describing that change so the message fully reflects the diff.

Generated by Claude Code

@brada4

brada4 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

This just addresses heaviest integration point, instead of addressing upstream pessimalities just waling around them

The rules from miniupnpd extracts meta nfproto . meta l4proto . ct original-proto-dst by reading each bit from the packet for each forwarded port , since it is programmed directly via kernel netlink the CLI heuristics and mini optimisations stand no chance. Besides miniupnp-generated rules fail validation as parsed back. Ignoring that they still do what they are supposed to albeit at 10x cost.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the updated revision. The tree is byte-identical to the previously reviewed commit (4be3d3d3); the only change is the commit message, which now re-adds the bullet "skip heavy payload expressions in miniupnpd rules by emulating nat type hook by selecting only new flows as candidates". That documents the ct state new addition in chain-post/dstnat/20-miniupnpd.nft and resolves the point raised in my previous review, so the message now fully reflects the diff. No new code changes and no new issues found.


Generated by Claude Code

@Self-Hosting-Group

Self-Hosting-Group commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

In my daemon PR, I removed the upnp_postrouting registration snippet that is not necessary for nftables, including the corresponding flush command and daemon configuration in init. The registration deletion could be removed here, if desired.

FYI: Your suggested changes to reduce hook weight added to my PR worked in a test. Found some nits in the commit message: miniupnp -> miniupnpd, and some redundancy.

@brada4

brada4 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

I will adj&st and compare.
In principle pass dnat could be integrated in fw4 - currently it is conditionally set when any dnat rule is active

Reduce usage of resources of miniupnp ruleset
- eliminate postrouting hook as miniupnpd never creates rules there
- but leave unreachable chain referenced in config file
- skip forward chain using ct-based heuristics just like default
  firewall
- but leave unreachable chain referenced in config file
- avoid non-transactional prerouting+forward updates creating out of
  sync ruleset
- skip heavy payload expressions in miniupnpd rules by emulating nat
  type hook by selecting only new flows as candidates
- remove chain unused by miniupnpd

Signed-off-by: Andris PE <neandris@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Miniupnpd: firewall rule for upnp jump forward wrong

5 participants