forwarded bridged packets enqueuing is broken

forwarded bridged packets enqueuing is broken

[Marco Oliverio]

Hi,

We cannot enqueue userspace bridged forwarded packets (neither in the
forward chain nor in the postrouting one):

nft add table bridge t
nft add chain bridge t forward {type filter hook forward priority 0\;}
nft add rule bridge t forward queue

packets from machines other than localhost aren't enqueued at all.

(this is also true for the postrouting chain).

We think the root of the problem is the check introduced by
b60a77386b1d4868f72f6353d35dabe5fbe981f2 (net: make skb_dst_force
return true when dst is refcounted):

modified   net/netfilter/nf_queue.c
@@ -174,6 +174,11 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
 		goto err;
 	}
 
+	if (!skb_dst_force(skb) && state->hook != NF_INET_PRE_ROUTING) {
+		status = -ENETDOWN;
+		goto err;
+	}
+

AFAIU forwarded bridge packets have a null dst entry in the first
place, as they don't enter the ip stack, so skb_dst_force() returns
false. The very same commit suggested to check skb_dst() before
skb_dst_force(), doing that indeed fix the issue for us:

modified   net/netfilter/nf_queue.c
@@ -174,7 +174,7 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
 		goto err;
 	}
 
-	if (!skb_dst_force(skb) && state->hook != NF_INET_PRE_ROUTING) {
+	if (skb_dst(skb) && !skb_dst_force(skb)) {
 		status = -ENETDOWN;
 		goto err;
 	}

This assumes that we shouldn't enqueue the packet if skb_dst_force()
sets not-NULL skb->dst to NULL, but it is safe to do that if skb->dst
was NULL in the first place. It should also cover che PRE_ROUTING hook
case. Is this assumption correct? Are there any side effects we're
missing?

If it is correct and it helps we can send a patch on top of the
netfilter tree.

Greetins
Marco

Re: forwarded bridged packets enqueuing is broken

[Florian Westphal]

Marco Oliverio <marco.oliverio@tanaza.com> wrote:
> We cannot enqueue userspace bridged forwarded packets (neither in the
> forward chain nor in the postrouting one):

[..]

> AFAIU forwarded bridge packets have a null dst entry in the first
> place, as they don't enter the ip stack, so skb_dst_force() returns
> false. The very same commit suggested to check skb_dst() before
> skb_dst_force(), doing that indeed fix the issue for us:
> 
> modified   net/netfilter/nf_queue.c
> @@ -174,7 +174,7 @@ static int __nf_queue(struct sk_buff *skb, const struct nf_hook_state *state,
>  		goto err;
>  	}
>  
> -	if (!skb_dst_force(skb) && state->hook != NF_INET_PRE_ROUTING) {
> +	if (skb_dst(skb) && !skb_dst_force(skb)) {

Looks fine to me, please submit this formally.  Thanks!