[PATCH nf] netfilter: br_netfilter: do not skip all hooks with 0 priority

[PATCH nf] netfilter: br_netfilter: do not skip all hooks with 0 priority

[Florian Westphal]

When br_netfilter module is loaded, skbs may be diverted to the
ipv4/ipv6 hooks, just like as if we were routing.

Unfortunately, bridge filter hooks with priority 0 may be skipped
in this case.

Example:
1. an nftables bridge ruleset is loaded, with a prerouting
   hook that has priority 0.
2. interface is added to the bridge.
3. no tcp packet is ever seen by the bridge prerouting hook.
4. flush the ruleset
5. load the bridge ruleset again.
6. tcp packets are processed as expected.

After 1) the only registered hook is the bridge prerouting hook, but its
not called yet because the bridge hasn't been brought up yet.

After 2), hook order is:
   0 br_nf_pre_routing // br_netfilter internal hook
   0 chain bridge f prerouting // nftables bridge ruleset

The packet is diverted to br_nf_pre_routing.
If call-iptables is off, the nftables bridge ruleset is called as expected.

But if its enabled, br_nf_hook_thresh() will skip it because it assumes
that all 0-priority hooks had been called previously in bridge context.

To avoid this, check for the br_nf_pre_routing hook itself, we need to
resume directly after it, even if this hook has a priority of 0.

Unfortunately, this still results in different packet flow.
With this fix, the eval order after in 3) is:
1. br_nf_pre_routing
2. ip(6)tables (if enabled)
3. nftables bridge

but after 5 its the much saner:
1. nftables bridge
2. br_nf_pre_routing
3. ip(6)tables (if enabled)

Unfortunately I don't see a solution here:
It would be possible to move br_nf_pre_routing to a higher priority
so that it will be called later in the pipeline, but this also impacts
ebtables evaluation order, and would still result in this very ordering
problem for all nftables-bridge hooks with the same priority as the
br_nf_pre_routing one.

Searching back through the git history I don't think this has
ever behaved in any other way, hence, no fixes-tag.

Reported-by: Radim Hrazdil <rhrazdil@redhat.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/bridge/br_netfilter_hooks.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index 4fd882686b04..ff4779036649 100644
--- a/net/bridge/br_netfilter_hooks.c
+++ b/net/bridge/br_netfilter_hooks.c
@@ -1012,9 +1012,24 @@ int br_nf_hook_thresh(unsigned int hook, struct net *net,
 		return okfn(net, sk, skb);
 
 	ops = nf_hook_entries_get_hook_ops(e);
-	for (i = 0; i < e->num_hook_entries &&
-	      ops[i]->priority <= NF_BR_PRI_BRNF; i++)
-		;
+	for (i = 0; i < e->num_hook_entries; i++) {
+		/* These hooks have already been called */
+		if (ops[i]->priority < NF_BR_PRI_BRNF)
+			continue;
+
+		/* These hooks have not been called yet, run them. */
+		if (ops[i]->priority > NF_BR_PRI_BRNF)
+			break;
+
+		/* take a closer look at NF_BR_PRI_BRNF. */
+		if (ops[i]->hook == br_nf_pre_routing) {
+			/* This hook diverted the skb to this function,
+			 * hooks after this have not been run yet.
+			 */
+			i++;
+			break;
+		}
+	}
 
 	nf_hook_state_init(&state, hook, NFPROTO_BRIDGE, indev, outdev,
 			   sk, net, okfn);
-- 
2.35.1

Re: [PATCH nf] netfilter: br_netfilter: do not skip all hooks with 0 priority

[Pablo Neira Ayuso]

On Tue, Jun 21, 2022 at 06:26:03PM +0200, Florian Westphal wrote:
> When br_netfilter module is loaded, skbs may be diverted to the
> ipv4/ipv6 hooks, just like as if we were routing.
> 
> Unfortunately, bridge filter hooks with priority 0 may be skipped
> in this case.
> 
> Example:
> 1. an nftables bridge ruleset is loaded, with a prerouting
>    hook that has priority 0.
> 2. interface is added to the bridge.
> 3. no tcp packet is ever seen by the bridge prerouting hook.
> 4. flush the ruleset
> 5. load the bridge ruleset again.
> 6. tcp packets are processed as expected.
> 
> After 1) the only registered hook is the bridge prerouting hook, but its
> not called yet because the bridge hasn't been brought up yet.
> 
> After 2), hook order is:
>    0 br_nf_pre_routing // br_netfilter internal hook
>    0 chain bridge f prerouting // nftables bridge ruleset
> 
> The packet is diverted to br_nf_pre_routing.
> If call-iptables is off, the nftables bridge ruleset is called as expected.
> 
> But if its enabled, br_nf_hook_thresh() will skip it because it assumes
> that all 0-priority hooks had been called previously in bridge context.
> 
> To avoid this, check for the br_nf_pre_routing hook itself, we need to
> resume directly after it, even if this hook has a priority of 0.
> 
> Unfortunately, this still results in different packet flow.
> With this fix, the eval order after in 3) is:
> 1. br_nf_pre_routing
> 2. ip(6)tables (if enabled)
> 3. nftables bridge
> 
> but after 5 its the much saner:
> 1. nftables bridge
> 2. br_nf_pre_routing
> 3. ip(6)tables (if enabled)
> 
> Unfortunately I don't see a solution here:
> It would be possible to move br_nf_pre_routing to a higher priority
> so that it will be called later in the pipeline, but this also impacts
> ebtables evaluation order, and would still result in this very ordering
> problem for all nftables-bridge hooks with the same priority as the
> br_nf_pre_routing one.
> 
> Searching back through the git history I don't think this has
> ever behaved in any other way, hence, no fixes-tag.

I don't see a better solution either.

Not related to this patch, more of an open question:

Why are people still using br_netfilter with nftables? Is there any
usecase that is not natively covered by nftables itself? Probably
insufficient of documentation?

br_netfilter was added because iptables provided a lot more features
than ebtables. These days this is not longer true, since they are in
feature parity.

Re: [PATCH nf] netfilter: br_netfilter: do not skip all hooks with 0 priority

[Pablo Neira Ayuso]

On Tue, Jun 21, 2022 at 06:26:03PM +0200, Florian Westphal wrote:
> When br_netfilter module is loaded, skbs may be diverted to the
> ipv4/ipv6 hooks, just like as if we were routing.
> 
> Unfortunately, bridge filter hooks with priority 0 may be skipped
> in this case.
> 
> Example:
> 1. an nftables bridge ruleset is loaded, with a prerouting
>    hook that has priority 0.
> 2. interface is added to the bridge.
> 3. no tcp packet is ever seen by the bridge prerouting hook.
> 4. flush the ruleset
> 5. load the bridge ruleset again.
> 6. tcp packets are processed as expected.
> 
> After 1) the only registered hook is the bridge prerouting hook, but its
> not called yet because the bridge hasn't been brought up yet.
> 
> After 2), hook order is:
>    0 br_nf_pre_routing // br_netfilter internal hook
>    0 chain bridge f prerouting // nftables bridge ruleset
> 
> The packet is diverted to br_nf_pre_routing.
> If call-iptables is off, the nftables bridge ruleset is called as expected.
> 
> But if its enabled, br_nf_hook_thresh() will skip it because it assumes
> that all 0-priority hooks had been called previously in bridge context.
> 
> To avoid this, check for the br_nf_pre_routing hook itself, we need to
> resume directly after it, even if this hook has a priority of 0.
> 
> Unfortunately, this still results in different packet flow.
> With this fix, the eval order after in 3) is:
> 1. br_nf_pre_routing
> 2. ip(6)tables (if enabled)
> 3. nftables bridge
> 
> but after 5 its the much saner:
> 1. nftables bridge
> 2. br_nf_pre_routing
> 3. ip(6)tables (if enabled)
> 
> Unfortunately I don't see a solution here:
> It would be possible to move br_nf_pre_routing to a higher priority
> so that it will be called later in the pipeline, but this also impacts
> ebtables evaluation order, and would still result in this very ordering
> problem for all nftables-bridge hooks with the same priority as the
> br_nf_pre_routing one.
> 
> Searching back through the git history I don't think this has
> ever behaved in any other way, hence, no fixes-tag.

Applied to nf, thanks