Received-SPF: pass (google.com: domain of linux-kernel+bounces-20752-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 9 Jan 2024 12:12:28 +0100
From: Florian Westphal <fw@strlen.de>
To: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
Cc: Florian Westphal <fw@strlen.de>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	kernel@openvz.org
Subject: Re: [PATCH] neighbour: purge nf_bridged skb from foreign device neigh
Message-ID: <20240109111228.GA7664@breakpoint.cc>
References: <20240108085232.95437-1-ptikhomirov@virtuozzo.com>
 <20240108111504.GA23297@breakpoint.cc>
 <a84b2797-2008-45d6-9ca3-c72666d3c419@virtuozzo.com>
 <07490c75-86c3-4488-8adb-7740b14feb30@virtuozzo.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <07490c75-86c3-4488-8adb-7740b14feb30@virtuozzo.com>
User-Agent: Mutt/1.10.1 (2018-07-13)

Pavel Tikhomirov <ptikhomirov@virtuozzo.com> wrote:
> index f980edfdd2783..105fbdb029261 100644
> --- a/include/linux/netfilter_bridge.h
> +++ b/include/linux/netfilter_bridge.h
> @@ -56,11 +56,15 @@ static inline int nf_bridge_get_physoutif(const struct
> sk_buff *skb)
>  }
> 
>  static inline struct net_device *
> -nf_bridge_get_physindev(const struct sk_buff *skb)
> +nf_bridge_get_physindev_rcu(const struct sk_buff *skb)
>  {
>         const struct nf_bridge_info *nf_bridge = nf_bridge_info_get(skb);
> +       struct net_device *dev;
> 
> -       return nf_bridge ? nf_bridge->physindev : NULL;
> +       if (!nf_bridge || !skb->dev)
> +               return 0;
> +
> +       return dev_get_by_index_rcu(skb->dev->net, nf_bridge->physindev_if);

You could use dev_net(skb->dev), yes.

Or create a preparation patch that does:

-nf_bridge_get_physindev(const struct sk_buff *skb)
+nf_bridge_get_physindev(const struct sk_buff *skb, struct net *net)

(all callers have a struct net available).

No need to rename the function, see below.

> -       br_indev = nf_bridge_get_physindev(oldskb);
> +       rcu_read_lock_bh();
> +       br_indev = nf_bridge_get_physindev_rcu(oldskb);

No need for rcu read lock, all netfilter hooks run inside
rcu_read_lock().

> Does it sound good?

Yes, seems ok to me.

> Or maybe instead we can have extra physindev_if field in addition to
> existing physindev to only do dev_get_by_index_rcu inside
> br_nf_pre_routing_finish_bridge_slow to doublecheck the ->physindev link?
> 
> Sorry in advance if I'm missing anything obvious.

Alternative would be to add a 'br_nf_unreg_serno' that gets incremented
from brnf_device_event(), then store that in nf_bridge_info struct and
compare to current value before net_device deref. If not equal, toss skb.

Problem is that we'd need some indirection to retrieve the current
value, otherwise places like nfnetlink_log() gain a module dependency on
br_netfilter :-(

We'd likely need
const atomic_t *br_nf_unreg_serno __read_mostly;
EXPORT_SYMBOL_GPL(br_nf_unreg_serno);

in net/netfilter/core.c for this, then set/clear the
pointer from br_netfilter_hooks.c.

I can't say/don't know which of the two options is better/worse.

s/struct net_device */int// has the benefit of shrinking nf_bridge_info,
so I'd try that first.