* [PATCH proxmox-firewall] nftables: move protocol match rendering into the lib
@ 2026-08-27 13:15 Hannes Laimer
2026-09-23 10:00 ` Stefan Hanreich
2026-09-23 19:15 ` applied: " Thomas Lamprecht
0 siblings, 2 replies; 4+ messages in thread
From: Hannes Laimer @ 2026-08-27 13:15 UTC (permalink / raw)
To: pve-devel
The statements selecting a transport protocol, its ports or an ICMP type
and code were generated in proxmox-firewall, although which statements
select a protocol is nftables knowledge and does not depend on the
firewall config. Move the rendering into proxmox-nftables, working on
plain expressions, and add an entry point taking the firewall's protocol
type to the config extension. The firewall keeps its IP family pinning
on top.
No functional change intended.
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
spiritually depends/based on [1], but apply order doesnt really matter,
and they are both good on their own
[1] https://lore.proxmox.com/pve-devel/20260827115803.1331245-1-h.laimer@proxmox.com/
proxmox-firewall/src/rule.rs | 166 +++----------------------------
proxmox-nftables/src/lib.rs | 1 +
proxmox-nftables/src/protocol.rs | 79 +++++++++++++++
3 files changed, 93 insertions(+), 153 deletions(-)
create mode 100644 proxmox-nftables/src/protocol.rs
diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs
index 048c00e..ea63a08 100644
--- a/proxmox-firewall/src/rule.rs
+++ b/proxmox-firewall/src/rule.rs
@@ -6,6 +6,7 @@ use proxmox_log as log;
use proxmox_nftables::{
Expression, Statement,
expression::{Ct, IpFamily, Meta, Payload, Prefix},
+ protocol,
statement::{Log, LogLevel, Match, Operator},
types::{AddRule, ChainPart, SetName, TableFamily, TablePart},
};
@@ -19,9 +20,7 @@ use proxmox_ve_config::{
ipset::{Ipfilter, IpsetName, RuleIpsetName},
log::LogRateLimit,
rule::{Direction, Kind, RuleGroup, Verdict as ConfigVerdict},
- rule_match::{
- Icmp, Icmpv6, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Sctp, Tcp, Udp,
- },
+ rule_match::{IpAddrMatch, IpMatch, Protocol, RuleMatch},
},
},
guest::types::Vmid,
@@ -596,162 +595,23 @@ impl ToNftRules for IpMatch {
}
}
-fn handle_protocol(rules: &mut [NftRule], _env: &NftRuleEnv, name: &str) -> Result<(), Error> {
- for rule in rules.iter_mut() {
- rule.push(Match::new_eq(Meta::new("l4proto"), Expression::from(name)).into());
- }
-
- Ok(())
-}
-
impl ToNftRules for Protocol {
- fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
- log::trace!("adding protocol: {self:?}");
-
- match self {
- Protocol::Tcp(tcp) => tcp.to_nft_rules(rules, env),
- Protocol::Udp(udp) => udp.to_nft_rules(rules, env),
- Protocol::Dccp(ports) => {
- handle_protocol(rules, env, "dccp")?;
- ports.to_nft_rules(rules, env)
- }
- Protocol::UdpLite(ports) => {
- handle_protocol(rules, env, "udplite")?;
- ports.to_nft_rules(rules, env)
- }
- Protocol::Sctp(sctp) => sctp.to_nft_rules(rules, env),
- Protocol::Icmp(icmp) => icmp.to_nft_rules(rules, env),
- Protocol::Icmpv6(icmpv6) => icmpv6.to_nft_rules(rules, env),
- Protocol::Named(name) => handle_protocol(rules, env, name),
- Protocol::Numeric(id) => {
- for rule in rules.iter_mut() {
- rule.push(Match::new_eq(Meta::new("l4proto"), Expression::from(*id)).into());
- }
-
- Ok(())
- }
- }
- }
-}
-
-impl ToNftRules for Tcp {
- fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
- handle_protocol(rules, env, "tcp")?;
- self.ports().to_nft_rules(rules, env)
- }
-}
-
-impl ToNftRules for Udp {
- fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
- handle_protocol(rules, env, "udp")?;
- self.ports().to_nft_rules(rules, env)
- }
-}
-
-impl ToNftRules for Sctp {
- fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
- handle_protocol(rules, env, "sctp")?;
- self.ports().to_nft_rules(rules, env)
- }
-}
-
-impl ToNftRules for Icmp {
fn to_nft_rules(&self, rules: &mut Vec<NftRule>, _env: &NftRuleEnv) -> Result<(), Error> {
- for rule in rules.iter_mut() {
- if matches!(rule.family(), Some(Family::V4) | None) {
- if let Some(icmp_type) = self.ty() {
- rule.push(
- Match::new_eq(Payload::field("icmp", "type"), Expression::from(icmp_type))
- .into(),
- );
- }
-
- if let Some(icmp_code) = self.code() {
- rule.push(
- Match::new_eq(Payload::field("icmp", "code"), Expression::from(icmp_code))
- .into(),
- );
- }
-
- if self.code().is_none() && self.ty().is_none() {
- rule.push(Match::new_eq(Meta::new("l4proto"), Expression::from("icmp")).into());
- }
-
- rule.set_family(Family::V4);
- }
- }
-
- Ok(())
- }
-}
-
-impl ToNftRules for Icmpv6 {
- fn to_nft_rules(&self, rules: &mut Vec<NftRule>, _env: &NftRuleEnv) -> Result<(), Error> {
- log::trace!("applying icmpv6: {self:?}");
+ log::trace!("adding protocol: {self:?}");
+ let family = self.family();
for rule in rules.iter_mut() {
- if matches!(rule.family(), Some(Family::V6) | None) {
- if let Some(icmp_type) = self.ty() {
- rule.push(
- Match::new_eq(
- Payload::field("icmpv6", "type"),
- Expression::from(icmp_type),
- )
- .into(),
- );
- }
-
- if let Some(icmp_code) = self.code() {
- rule.push(
- Match::new_eq(
- Payload::field("icmpv6", "code"),
- Expression::from(icmp_code),
- )
- .into(),
- );
- }
-
- if self.code().is_none() && self.ty().is_none() {
- rule.push(
- Match::new_eq(Meta::new("l4proto"), Expression::from("icmpv6")).into(),
- );
- }
-
- rule.set_family(Family::V6);
+ if family
+ .zip(rule.family())
+ .is_some_and(|(wanted, pinned)| wanted != pinned)
+ {
+ continue;
}
- }
-
- Ok(())
- }
-}
-
-impl ToNftRules for Ports {
- fn to_nft_rules(&self, rules: &mut Vec<NftRule>, _env: &NftRuleEnv) -> Result<(), Error> {
- log::trace!("applying ports: {self:?}");
-
- for rule in rules {
- if let Some(sport) = self.sport() {
- log::trace!("applying sport: {sport:?}");
-
- rule.push(
- Match::new_eq(
- Expression::from(Payload::field("th", "sport")),
- Expression::from(sport),
- )
- .into(),
- )
+ for statement in protocol::matches(self) {
+ rule.push(statement);
}
-
- if let Some(dport) = self.dport() {
- log::trace!("applying dport: {dport:?}");
-
- rule.push(
- Match::new_eq(
- Expression::from(Payload::field("th", "dport")),
- Expression::from(dport),
- )
- .into(),
- )
+ if let Some(family) = family {
+ rule.set_family(family);
}
}
diff --git a/proxmox-nftables/src/lib.rs b/proxmox-nftables/src/lib.rs
index 2003e1b..c54905b 100644
--- a/proxmox-nftables/src/lib.rs
+++ b/proxmox-nftables/src/lib.rs
@@ -2,6 +2,7 @@ pub mod client;
pub mod command;
pub mod expression;
pub mod helper;
+pub mod protocol;
pub mod statement;
pub mod types;
diff --git a/proxmox-nftables/src/protocol.rs b/proxmox-nftables/src/protocol.rs
new file mode 100644
index 0000000..fcd657f
--- /dev/null
+++ b/proxmox-nftables/src/protocol.rs
@@ -0,0 +1,79 @@
+//! Rendering of a transport protocol match into the statements that select it, which are
+//! `meta l4proto` for the protocol, the transport-header ports, and the ICMP type and code.
+//! The statements carry no IP family test of their own, so they work unchanged in the inet
+//! and bridge families. An ICMP match still only applies to its own IP family, which is left
+//! to the caller.
+
+#[cfg(feature = "config-ext")]
+use proxmox_ve_config::firewall::types::rule_match::{Ports, Protocol};
+
+use crate::expression::{Meta, Payload};
+use crate::statement::Match;
+use crate::{Expression, Statement};
+
+/// Selects a protocol by name or number.
+pub fn l4proto(protocol: impl Into<Expression>) -> Statement {
+ Match::new_eq(Meta::new("l4proto"), protocol.into()).into()
+}
+
+/// Selects the transport-header source and destination ports that are given.
+pub fn ports(sport: Option<Expression>, dport: Option<Expression>) -> Vec<Statement> {
+ let mut statements = Vec::new();
+ if let Some(sport) = sport {
+ statements.push(Match::new_eq(Payload::field("th", "sport"), sport).into());
+ }
+ if let Some(dport) = dport {
+ statements.push(Match::new_eq(Payload::field("th", "dport"), dport).into());
+ }
+ statements
+}
+
+/// Selects an ICMP flavour by type and code. Matching either already implies the protocol, so
+/// only a bare match needs the explicit `l4proto` test.
+pub fn icmp(protocol: &str, ty: Option<Expression>, code: Option<Expression>) -> Vec<Statement> {
+ if ty.is_none() && code.is_none() {
+ return vec![l4proto(protocol)];
+ }
+ let mut statements = Vec::new();
+ if let Some(ty) = ty {
+ statements.push(Match::new_eq(Payload::field(protocol, "type"), ty).into());
+ }
+ if let Some(code) = code {
+ statements.push(Match::new_eq(Payload::field(protocol, "code"), code).into());
+ }
+ statements
+}
+
+/// The statements selecting `protocol`, in evaluation order.
+#[cfg(feature = "config-ext")]
+pub fn matches(protocol: &Protocol) -> Vec<Statement> {
+ match protocol {
+ Protocol::Tcp(tcp) => with_ports("tcp", tcp.ports()),
+ Protocol::Udp(udp) => with_ports("udp", udp.ports()),
+ Protocol::Sctp(sctp) => with_ports("sctp", sctp.ports()),
+ Protocol::Dccp(config) => with_ports("dccp", config),
+ Protocol::UdpLite(config) => with_ports("udplite", config),
+ Protocol::Icmp(config) => icmp(
+ "icmp",
+ config.ty().map(Expression::from),
+ config.code().map(Expression::from),
+ ),
+ Protocol::Icmpv6(config) => icmp(
+ "icmpv6",
+ config.ty().map(Expression::from),
+ config.code().map(Expression::from),
+ ),
+ Protocol::Named(name) => vec![l4proto(name.as_str())],
+ Protocol::Numeric(id) => vec![l4proto(*id)],
+ }
+}
+
+#[cfg(feature = "config-ext")]
+fn with_ports(protocol: &str, config: &Ports) -> Vec<Statement> {
+ let mut statements = vec![l4proto(protocol)];
+ statements.extend(ports(
+ config.sport().map(Expression::from),
+ config.dport().map(Expression::from),
+ ));
+ statements
+}
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH proxmox-firewall] nftables: move protocol match rendering into the lib
2026-08-27 13:15 [PATCH proxmox-firewall] nftables: move protocol match rendering into the lib Hannes Laimer
@ 2026-09-23 10:00 ` Stefan Hanreich
2026-09-23 11:18 ` Hannes Laimer
2026-09-23 19:15 ` applied: " Thomas Lamprecht
1 sibling, 1 reply; 4+ messages in thread
From: Stefan Hanreich @ 2026-09-23 10:00 UTC (permalink / raw)
To: pve-devel
On 8/27/26 3:15 PM, Hannes Laimer wrote:
> The statements selecting a transport protocol, its ports or an ICMP type
> and code were generated in proxmox-firewall, although which statements
> select a protocol is nftables knowledge and does not depend on the
> firewall config. Move the rendering into proxmox-nftables, working on
> plain expressions, and add an entry point taking the firewall's protocol
> type to the config extension. The firewall keeps its IP family pinning
> on top.
>
> No functional change intended.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> spiritually depends/based on [1], but apply order doesnt really matter,
> and they are both good on their own
>
> [1] https://lore.proxmox.com/pve-devel/20260827115803.1331245-1-h.laimer@proxmox.com/
>
> proxmox-firewall/src/rule.rs | 166 +++----------------------------
> proxmox-nftables/src/lib.rs | 1 +
> proxmox-nftables/src/protocol.rs | 79 +++++++++++++++
> 3 files changed, 93 insertions(+), 153 deletions(-)
> create mode 100644 proxmox-nftables/src/protocol.rs
>
> diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs
> index 048c00e..ea63a08 100644
> --- a/proxmox-firewall/src/rule.rs
> +++ b/proxmox-firewall/src/rule.rs
> @@ -6,6 +6,7 @@ use proxmox_log as log;
> use proxmox_nftables::{
> Expression, Statement,
> expression::{Ct, IpFamily, Meta, Payload, Prefix},
> + protocol,
> statement::{Log, LogLevel, Match, Operator},
> types::{AddRule, ChainPart, SetName, TableFamily, TablePart},
> };
> @@ -19,9 +20,7 @@ use proxmox_ve_config::{
> ipset::{Ipfilter, IpsetName, RuleIpsetName},
> log::LogRateLimit,
> rule::{Direction, Kind, RuleGroup, Verdict as ConfigVerdict},
> - rule_match::{
> - Icmp, Icmpv6, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Sctp, Tcp, Udp,
> - },
> + rule_match::{IpAddrMatch, IpMatch, Protocol, RuleMatch},
> },
> },
> guest::types::Vmid,
> @@ -596,162 +595,23 @@ impl ToNftRules for IpMatch {
> }
> }
>
> -fn handle_protocol(rules: &mut [NftRule], _env: &NftRuleEnv, name: &str) -> Result<(), Error> {
> - for rule in rules.iter_mut() {
> - rule.push(Match::new_eq(Meta::new("l4proto"), Expression::from(name)).into());
> - }
> -
> - Ok(())
> -}
> -
> impl ToNftRules for Protocol {
> - fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
> - log::trace!("adding protocol: {self:?}");
> -
> - match self {
> - Protocol::Tcp(tcp) => tcp.to_nft_rules(rules, env),
> - Protocol::Udp(udp) => udp.to_nft_rules(rules, env),
> - Protocol::Dccp(ports) => {
> - handle_protocol(rules, env, "dccp")?;
> - ports.to_nft_rules(rules, env)
> - }
> - Protocol::UdpLite(ports) => {
> - handle_protocol(rules, env, "udplite")?;
> - ports.to_nft_rules(rules, env)
> - }
> - Protocol::Sctp(sctp) => sctp.to_nft_rules(rules, env),
> - Protocol::Icmp(icmp) => icmp.to_nft_rules(rules, env),
> - Protocol::Icmpv6(icmpv6) => icmpv6.to_nft_rules(rules, env),
> - Protocol::Named(name) => handle_protocol(rules, env, name),
> - Protocol::Numeric(id) => {
> - for rule in rules.iter_mut() {
> - rule.push(Match::new_eq(Meta::new("l4proto"), Expression::from(*id)).into());
> - }
> -
> - Ok(())
> - }
> - }
> - }
> -}
> -
> -impl ToNftRules for Tcp {
> - fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
> - handle_protocol(rules, env, "tcp")?;
> - self.ports().to_nft_rules(rules, env)
> - }
> -}
> -
> -impl ToNftRules for Udp {
> - fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
> - handle_protocol(rules, env, "udp")?;
> - self.ports().to_nft_rules(rules, env)
> - }
> -}
> -
> -impl ToNftRules for Sctp {
> - fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
> - handle_protocol(rules, env, "sctp")?;
> - self.ports().to_nft_rules(rules, env)
> - }
> -}
> -
> -impl ToNftRules for Icmp {
> fn to_nft_rules(&self, rules: &mut Vec<NftRule>, _env: &NftRuleEnv) -> Result<(), Error> {
> - for rule in rules.iter_mut() {
> - if matches!(rule.family(), Some(Family::V4) | None) {
> - if let Some(icmp_type) = self.ty() {
> - rule.push(
> - Match::new_eq(Payload::field("icmp", "type"), Expression::from(icmp_type))
> - .into(),
> - );
> - }
> -
> - if let Some(icmp_code) = self.code() {
> - rule.push(
> - Match::new_eq(Payload::field("icmp", "code"), Expression::from(icmp_code))
> - .into(),
> - );
> - }
> -
> - if self.code().is_none() && self.ty().is_none() {
> - rule.push(Match::new_eq(Meta::new("l4proto"), Expression::from("icmp")).into());
> - }
> -
> - rule.set_family(Family::V4);
> - }
> - }
> -
> - Ok(())
> - }
> -}
> -
> -impl ToNftRules for Icmpv6 {
> - fn to_nft_rules(&self, rules: &mut Vec<NftRule>, _env: &NftRuleEnv) -> Result<(), Error> {
> - log::trace!("applying icmpv6: {self:?}");
> + log::trace!("adding protocol: {self:?}");
>
> + let family = self.family();
> for rule in rules.iter_mut() {
> - if matches!(rule.family(), Some(Family::V6) | None) {
> - if let Some(icmp_type) = self.ty() {
> - rule.push(
> - Match::new_eq(
> - Payload::field("icmpv6", "type"),
> - Expression::from(icmp_type),
> - )
> - .into(),
> - );
> - }
> -
> - if let Some(icmp_code) = self.code() {
> - rule.push(
> - Match::new_eq(
> - Payload::field("icmpv6", "code"),
> - Expression::from(icmp_code),
> - )
> - .into(),
> - );
> - }
> -
> - if self.code().is_none() && self.ty().is_none() {
> - rule.push(
> - Match::new_eq(Meta::new("l4proto"), Expression::from("icmpv6")).into(),
> - );
> - }
> -
> - rule.set_family(Family::V6);
> + if family
> + .zip(rule.family())
> + .is_some_and(|(wanted, pinned)| wanted != pinned)
> + {
> + continue;
> }
> - }
> -
> - Ok(())
> - }
> -}
> -
> -impl ToNftRules for Ports {
> - fn to_nft_rules(&self, rules: &mut Vec<NftRule>, _env: &NftRuleEnv) -> Result<(), Error> {
> - log::trace!("applying ports: {self:?}");
> -
> - for rule in rules {
> - if let Some(sport) = self.sport() {
> - log::trace!("applying sport: {sport:?}");
> -
> - rule.push(
> - Match::new_eq(
> - Expression::from(Payload::field("th", "sport")),
> - Expression::from(sport),
> - )
> - .into(),
> - )
> + for statement in protocol::matches(self) {
> + rule.push(statement);
> }
nit: could be extend?
generally the whole function reads a bit awkward to me, but I cant think of anything
better on the spot.
> -
> - if let Some(dport) = self.dport() {
> - log::trace!("applying dport: {dport:?}");
> -
> - rule.push(
> - Match::new_eq(
> - Expression::from(Payload::field("th", "dport")),
> - Expression::from(dport),
> - )
> - .into(),
> - )
> + if let Some(family) = family {
> + rule.set_family(family);
> }
> }
>
> diff --git a/proxmox-nftables/src/lib.rs b/proxmox-nftables/src/lib.rs
> index 2003e1b..c54905b 100644
> --- a/proxmox-nftables/src/lib.rs
> +++ b/proxmox-nftables/src/lib.rs
> @@ -2,6 +2,7 @@ pub mod client;
> pub mod command;
> pub mod expression;
> pub mod helper;
> +pub mod protocol;
> pub mod statement;
> pub mod types;
>
> diff --git a/proxmox-nftables/src/protocol.rs b/proxmox-nftables/src/protocol.rs
> new file mode 100644
> index 0000000..fcd657f
> --- /dev/null
> +++ b/proxmox-nftables/src/protocol.rs
> @@ -0,0 +1,79 @@
> +//! Rendering of a transport protocol match into the statements that select it, which are
> +//! `meta l4proto` for the protocol, the transport-header ports, and the ICMP type and code.
> +//! The statements carry no IP family test of their own, so they work unchanged in the inet
> +//! and bridge families. An ICMP match still only applies to its own IP family, which is left
> +//! to the caller.
> +
> +#[cfg(feature = "config-ext")]
> +use proxmox_ve_config::firewall::types::rule_match::{Ports, Protocol};
> +
> +use crate::expression::{Meta, Payload};
> +use crate::statement::Match;
> +use crate::{Expression, Statement};
> +
> +/// Selects a protocol by name or number.
> +pub fn l4proto(protocol: impl Into<Expression>) -> Statement {
> + Match::new_eq(Meta::new("l4proto"), protocol.into()).into()
> +}
> +
> +/// Selects the transport-header source and destination ports that are given.
> +pub fn ports(sport: Option<Expression>, dport: Option<Expression>) -> Vec<Statement> {
> + let mut statements = Vec::new();
> + if let Some(sport) = sport {
> + statements.push(Match::new_eq(Payload::field("th", "sport"), sport).into());
> + }
> + if let Some(dport) = dport {
> + statements.push(Match::new_eq(Payload::field("th", "dport"), dport).into());
> + }
> + statements
> +}
> +
> +/// Selects an ICMP flavour by type and code. Matching either already implies the protocol, so
> +/// only a bare match needs the explicit `l4proto` test.
> +pub fn icmp(protocol: &str, ty: Option<Expression>, code: Option<Expression>) -> Vec<Statement> {
> + if ty.is_none() && code.is_none() {
> + return vec![l4proto(protocol)];
> + }
nit: is it necessary to short circuit rather than just do this and have the two if statements below?
let mut statements = vec![l4proto(protocol)];
iirc, nftables simplifies this when creating the rule anyway, so it shouldn't be less efficient in the
ruleset:
$ nft 'add rule inet test test-chain meta l4proto icmpv6 icmpv6 code 4'
$ nft 'list chain inet test test-chain'
chain test-chain {
icmpv6 code 4
}
> + let mut statements = Vec::new();
> + if let Some(ty) = ty {
> + statements.push(Match::new_eq(Payload::field(protocol, "type"), ty).into());
> + }
> + if let Some(code) = code {
> + statements.push(Match::new_eq(Payload::field(protocol, "code"), code).into());
> + }
> + statements
> +}
> +
> +/// The statements selecting `protocol`, in evaluation order.
> +#[cfg(feature = "config-ext")]
> +pub fn matches(protocol: &Protocol) -> Vec<Statement> {
> + match protocol {
> + Protocol::Tcp(tcp) => with_ports("tcp", tcp.ports()),
> + Protocol::Udp(udp) => with_ports("udp", udp.ports()),
> + Protocol::Sctp(sctp) => with_ports("sctp", sctp.ports()),
> + Protocol::Dccp(config) => with_ports("dccp", config),
> + Protocol::UdpLite(config) => with_ports("udplite", config),
> + Protocol::Icmp(config) => icmp(
> + "icmp",
> + config.ty().map(Expression::from),
> + config.code().map(Expression::from),
> + ),
> + Protocol::Icmpv6(config) => icmp(
> + "icmpv6",
> + config.ty().map(Expression::from),
> + config.code().map(Expression::from),
> + ),
> + Protocol::Named(name) => vec![l4proto(name.as_str())],
> + Protocol::Numeric(id) => vec![l4proto(*id)],
> + }
> +}
> +
> +#[cfg(feature = "config-ext")]
> +fn with_ports(protocol: &str, config: &Ports) -> Vec<Statement> {
> + let mut statements = vec![l4proto(protocol)];
> + statements.extend(ports(
> + config.sport().map(Expression::from),
> + config.dport().map(Expression::from),
> + ));
> + statements
> +}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH proxmox-firewall] nftables: move protocol match rendering into the lib
2026-09-23 10:00 ` Stefan Hanreich
@ 2026-09-23 11:18 ` Hannes Laimer
0 siblings, 0 replies; 4+ messages in thread
From: Hannes Laimer @ 2026-09-23 11:18 UTC (permalink / raw)
To: Stefan Hanreich, pve-devel
thanks for taking a look!
two comments inline, will prepare a v2
On 2026-09-23 12:00, Stefan Hanreich wrote:
>
>
> On 8/27/26 3:15 PM, Hannes Laimer wrote:
>> The statements selecting a transport protocol, its ports or an ICMP type
>> and code were generated in proxmox-firewall, although which statements
>> select a protocol is nftables knowledge and does not depend on the
>> firewall config. Move the rendering into proxmox-nftables, working on
>> plain expressions, and add an entry point taking the firewall's protocol
>> type to the config extension. The firewall keeps its IP family pinning
>> on top.
>>
>> No functional change intended.
>>
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
..
>> -
>> -impl ToNftRules for Ports {
>> - fn to_nft_rules(&self, rules: &mut Vec<NftRule>, _env: &NftRuleEnv) -> Result<(), Error> {
>> - log::trace!("applying ports: {self:?}");
>> -
>> - for rule in rules {
>> - if let Some(sport) = self.sport() {
>> - log::trace!("applying sport: {sport:?}");
>> -
>> - rule.push(
>> - Match::new_eq(
>> - Expression::from(Payload::field("th", "sport")),
>> - Expression::from(sport),
>> - )
>> - .into(),
>> - )
>> + for statement in protocol::matches(self) {
>> + rule.push(statement);
>> }
>
> nit: could be extend?
>
> generally the whole function reads a bit awkward to me, but I cant think of anything
> better on the spot.
>
yes, something like
```
for rule in rules.iter_mut() {
- if family
- .zip(rule.family())
- .is_some_and(|(wanted, pinned)| wanted != pinned)
- {
- continue;
- }
- for statement in protocol::matches(self) {
- rule.push(statement);
- }
- if let Some(family) = family {
+ if let Some(family) = self.family() {
+ if rule.family().is_some_and(|pinned| pinned != family) {
+ continue;
+ }
rule.set_family(family);
}
+ rule.extend(protocol::matches(self));
}
```
seems better
>> -
>> - if let Some(dport) = self.dport() {
>> - log::trace!("applying dport: {dport:?}");
>> -
>> - rule.push(
>> - Match::new_eq(
>> - Expression::from(Payload::field("th", "dport")),
>> - Expression::from(dport),
>> - )
>> - .into(),
>> - )
>> + if let Some(family) = family {
>> + rule.set_family(family);
>> }
>> }
>>
..
>> +
>> +/// Selects an ICMP flavour by type and code. Matching either already implies the protocol, so
>> +/// only a bare match needs the explicit `l4proto` test.
>> +pub fn icmp(protocol: &str, ty: Option<Expression>, code: Option<Expression>) -> Vec<Statement> {
>> + if ty.is_none() && code.is_none() {
>> + return vec![l4proto(protocol)];
>> + }
>
> nit: is it necessary to short circuit rather than just do this and have the two if statements below?
>
with `meta l4proto` present nft doesnt add l3 checks that it would
otherwise for an `icmpv6` payload, basically
```
nft add rule inet t c meta l4proto icmpv6 icmpv6 code 4
nft add rule inet t c icmpv6 code 4
```
and `nft --debug=netlink list ruleset` shows the diff
I guess emitting `meta nfproto ipv6` ourself could work as well(?),
but for that we'd have to know the table family here..
> let mut statements = vec![l4proto(protocol)];
>
> iirc, nftables simplifies this when creating the rule anyway, so it shouldn't be less efficient in the
> ruleset:
>
> $ nft 'add rule inet test test-chain meta l4proto icmpv6 icmpv6 code 4'
> $ nft 'list chain inet test test-chain'
>
> chain test-chain {
> icmpv6 code 4
> }
>
>
>
>> + let mut statements = Vec::new();
>> + if let Some(ty) = ty {
>> + statements.push(Match::new_eq(Payload::field(protocol, "type"), ty).into());
>> + }
>> + if let Some(code) = code {
>> + statements.push(Match::new_eq(Payload::field(protocol, "code"), code).into());
>> + }
>> + statements
>> +}
>> +
>> +/// The statements selecting `protocol`, in evaluation order.
>> +#[cfg(feature = "config-ext")]
>> +pub fn matches(protocol: &Protocol) -> Vec<Statement> {
>> + match protocol {
>> + Protocol::Tcp(tcp) => with_ports("tcp", tcp.ports()),
>> + Protocol::Udp(udp) => with_ports("udp", udp.ports()),
>> + Protocol::Sctp(sctp) => with_ports("sctp", sctp.ports()),
>> + Protocol::Dccp(config) => with_ports("dccp", config),
>> + Protocol::UdpLite(config) => with_ports("udplite", config),
>> + Protocol::Icmp(config) => icmp(
>> + "icmp",
>> + config.ty().map(Expression::from),
>> + config.code().map(Expression::from),
>> + ),
>> + Protocol::Icmpv6(config) => icmp(
>> + "icmpv6",
>> + config.ty().map(Expression::from),
>> + config.code().map(Expression::from),
>> + ),
>> + Protocol::Named(name) => vec![l4proto(name.as_str())],
>> + Protocol::Numeric(id) => vec![l4proto(*id)],
>> + }
>> +}
>> +
>> +#[cfg(feature = "config-ext")]
>> +fn with_ports(protocol: &str, config: &Ports) -> Vec<Statement> {
>> + let mut statements = vec![l4proto(protocol)];
>> + statements.extend(ports(
>> + config.sport().map(Expression::from),
>> + config.dport().map(Expression::from),
>> + ));
>> + statements
>> +}
>
>
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* applied: [PATCH proxmox-firewall] nftables: move protocol match rendering into the lib
2026-08-27 13:15 [PATCH proxmox-firewall] nftables: move protocol match rendering into the lib Hannes Laimer
2026-09-23 10:00 ` Stefan Hanreich
@ 2026-09-23 19:15 ` Thomas Lamprecht
1 sibling, 0 replies; 4+ messages in thread
From: Thomas Lamprecht @ 2026-09-23 19:15 UTC (permalink / raw)
To: pve-devel, Hannes Laimer
On Thu, 27 Aug 2026 15:15:24 +0200, Hannes Laimer wrote:
> The statements selecting a transport protocol, its ports or an ICMP type
> and code were generated in proxmox-firewall, although which statements
> select a protocol is nftables knowledge and does not depend on the
> firewall config. Move the rendering into proxmox-nftables, working on
> plain expressions, and add an entry point taking the firewall's protocol
> type to the config extension. The firewall keeps its IP family pinning
> on top.
>
> [...]
Applied, thanks!
[1/1] nftables: move protocol match rendering into the lib
commit: 9840e166b7109e3aed00293e326d88f19b4bd391
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-09-23 19:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-27 13:15 [PATCH proxmox-firewall] nftables: move protocol match rendering into the lib Hannes Laimer
2026-09-23 10:00 ` Stefan Hanreich
2026-09-23 11:18 ` Hannes Laimer
2026-09-23 19:15 ` applied: " Thomas Lamprecht
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.