Re: [PATCH net 1/1] net: bridge: guard local finish against missing port

Yuan Tan <[email protected]> wrote:
> > >>> The bridge local receive path may be deferred by netfilter and resumed
> > >>> later. By the time br_handle_local_finish() runs, skb->dev may still be
> > >>> valid while its bridge port association has already been removed.
> > >>>
> > >>> br_handle_local_finish() unconditionally looks up the bridge port from
> > >>> skb->dev and dereferences it for source learning. If the port is no
> > >>> longer attached to the bridge, the lookup returns NULL and the deferred
> > >>> local receive path can no longer rely on the port state being present.
> > You should update the Fixes: tag but also wait 24h before re-posting another
> > patch version.
> 
> Ok we will send the v2 with fix tag f350a0a87374 after 24h

Would you mind exploring an alternative fix for this?

As nfnetlink_queue'd skbs leave rcu read locked section, great
care has to be taken on reinject.

Either bridge could call nf_queue_nf_hook_drop() on bridge
port removal, or nfqnl_reinject() could revalidate that skb->dev
is part of a bridge and munge verdict to NF_DROP in case
the assocication was removed while packet was out.

static bool nfqnl_bridge_port_removed(const struct nf_queue_entry *e)
{
	return e->state.pf == NFPROTO_BRIDGE &&
		!br_port_get_rcu(e->skb->dev) == NULL;
}

static void nfqnl_reinject(struct nf_queue_entry *entry, unsigned int verdict)
{
        const struct nf_ct_hook *ct_hook;

        if (verdict == NF_ACCEPT ||
            verdict == NF_REPEAT ||
            verdict == NF_STOP) {
                unsigned int ct_verdict = verdict;

		....

		if (nfqnl_bridge_port_removed(entry))
			verdict = NF_DROP;

Its more work, but it places the extra checks to where
they are really needed.

Also see related bug fix:  At this time, entire bridge device can go
away while packet is out.

https://patchwork.ozlabs.org/project/netfilter-devel/patch/ca7ee343bbcb44905e1f5b853df2f3a5b7d40548.1778493188.git.royenheart@gmail.com/