Re: [PATCH nf v3 1/1] bridge: br_netfilter: pin bridge device while NFQUEUE holds fake dst

[Florian Westphal <fw@strlen.de>]

Ren Wei <n05ec@lzu.edu.cn> wrote:
> The bridge netfilter fake rtable is embedded in struct net_bridge and is
> attached to bridged packets with skb_dst_set_noref(). If such a packet is
> queued to NFQUEUE, __nf_queue() upgrades that fake dst with
> skb_dst_force().

Ok, I think I understand why this mess exists. Ideally we could rip out
the fake rtable and alloc it as separate object with distinct lifetime,
this FAKE_RTABLE crap needs to die...  But I understand its more
intrusive / harder to fix it that way (performance considerations don't
matter however, br_netfilter can be pessimized).

> +#if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
> +static struct net_device *nf_queue_bridge_dev(const struct sk_buff *skb,
> +					      const struct nf_hook_state *state)
> +{
> +	struct dst_entry *dst = skb_dst(skb);
> +	struct net_device *dev;
> +
> +	if (state->pf != NFPROTO_BRIDGE || !nf_bridge_info_exists(skb))
> +		return NULL;
> +

I guess what you are saying is that if br_netfilter hack is on,
skb->dst can be fake rtable while packet is sent to nfnetlink_queue
from a *bridge* family hook where in/outdev are the physical ports
yet skb->dev isn't the bridge device either.  The forced ref on the
dst is useless in that case, because netdevice_removal frees the
net_device regardless of the fake rtable dst entries refcounts.

If thats correct, could you please streamline this patch slightly?

Something like this (totally untested and misses dev_put part); and
that comment might be a bit more verbose.

diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -84,6 +84,8 @@ static void __nf_queue_entry_init_physdevs(struct nf_queue_entry *entry)
 {
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
        const struct sk_buff *skb = entry->skb;
+       struct dst_entry *dst = skb_dst(skb);
+       struct net_device *dev = NULL;
 
        if (nf_bridge_info_exists(skb)) {
                entry->physin = nf_bridge_get_physindev(skb, entry->state.net);
@@ -92,6 +94,17 @@ static void __nf_queue_entry_init_physdevs(struct nf_queue_entry *entry)
                entry->physin = NULL;
                entry->physout = NULL;
        }
+
+       if (dst && (dst->flags & DST_FAKE_RTABLE)) {
+               dev = dst_dev_rcu(dst);
+               if (dev == blackhole_netdev) [ Q: Is that really needed? I don't think so ]
+                       dev = NULL;
+       }
+
+       /* Must hold reference on the bridge device: the fake rtable
+        * is embedded within, dst_hold() is not sufficient.
+        */
+       entry->br_dev = dev;
 #endif
 }
 
@@ -108,6 +121,7 @@ bool nf_queue_entry_get_refs(struct nf_queue_entry *entry)
        dev_hold(state->out);
 
 #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
+       dev_hold(entry->br_dev);
        dev_hold(entry->physin);
        dev_hold(entry->physout);
 #endif

Thanks!