[PATCH net-next 0/1] netfilter fix for net-next

[PATCH net-next 0/1] netfilter fix for net-next

[Florian Westphal]

Hello,

This is a late bug fix for the *net-next* tree to make nftables
"fib" expression play nice with VRF devices.

This was broken since day 1 (v4.10) so I don't see a compelling reason
to push this via net at the last minute.

Please consider pulling this change from
  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf-next.git

----------------------------------------------------------------
The following changes since commit b9a5cbf8ba24e88071a97a51a09ef5cdf0d1f6a1:

  Merge branch 'sfc-tc-offload' (2022-09-28 09:43:22 +0100)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/netfilter/nf-next.git master

for you to fetch changes up to 2a8a7c0eaa8747c16aa4a48d573aa920d5c00a5c:

  netfilter: nft_fib: Fix for rpath check with VRF devices (2022-09-28 13:33:26 +0200)

----------------------------------------------------------------
Phil Sutter (1):
      netfilter: nft_fib: Fix for rpath check with VRF devices

 net/ipv4/netfilter/nft_fib_ipv4.c | 3 +++
 net/ipv6/netfilter/nft_fib_ipv6.c | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

[PATCH 1/1] netfilter: nft_fib: Fix for rpath check with VRF devices

[Florian Westphal]

From: Phil Sutter <phil@nwl.cc>

Analogous to commit b575b24b8eee3 ("netfilter: Fix rpfilter
dropping vrf packets by mistake") but for nftables fib expression:
Add special treatment of VRF devices so that typical reverse path
filtering via 'fib saddr . iif oif' expression works as expected.

Fixes: f6d0cbcf09c50 ("netfilter: nf_tables: add fib expression")
Signed-off-by: Phil Sutter <phil@nwl.cc>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/ipv4/netfilter/nft_fib_ipv4.c | 3 +++
 net/ipv6/netfilter/nft_fib_ipv6.c | 6 +++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c
index b75cac69bd7e..7ade04ff972d 100644
--- a/net/ipv4/netfilter/nft_fib_ipv4.c
+++ b/net/ipv4/netfilter/nft_fib_ipv4.c
@@ -83,6 +83,9 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs,
 	else
 		oif = NULL;
 
+	if (priv->flags & NFTA_FIB_F_IIF)
+		fl4.flowi4_oif = l3mdev_master_ifindex_rcu(oif);
+
 	if (nft_hook(pkt) == NF_INET_PRE_ROUTING &&
 	    nft_fib_is_loopback(pkt->skb, nft_in(pkt))) {
 		nft_fib_store_result(dest, priv, nft_in(pkt));
diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c
index 8970d0b4faeb..1d7e520d9966 100644
--- a/net/ipv6/netfilter/nft_fib_ipv6.c
+++ b/net/ipv6/netfilter/nft_fib_ipv6.c
@@ -41,6 +41,9 @@ static int nft_fib6_flowi_init(struct flowi6 *fl6, const struct nft_fib *priv,
 	if (ipv6_addr_type(&fl6->daddr) & IPV6_ADDR_LINKLOCAL) {
 		lookup_flags |= RT6_LOOKUP_F_IFACE;
 		fl6->flowi6_oif = get_ifindex(dev ? dev : pkt->skb->dev);
+	} else if ((priv->flags & NFTA_FIB_F_IIF) &&
+		   (netif_is_l3_master(dev) || netif_is_l3_slave(dev))) {
+		fl6->flowi6_oif = dev->ifindex;
 	}
 
 	if (ipv6_addr_type(&fl6->saddr) & IPV6_ADDR_UNICAST)
@@ -197,7 +200,8 @@ void nft_fib6_eval(const struct nft_expr *expr, struct nft_regs *regs,
 	if (rt->rt6i_flags & (RTF_REJECT | RTF_ANYCAST | RTF_LOCAL))
 		goto put_rt_err;
 
-	if (oif && oif != rt->rt6i_idev->dev)
+	if (oif && oif != rt->rt6i_idev->dev &&
+	    l3mdev_master_ifindex_rcu(rt->rt6i_idev->dev) != oif->ifindex)
 		goto put_rt_err;
 
 	nft_fib_store_result(dest, priv, rt->rt6i_idev->dev);
-- 
2.35.1

Re: [PATCH 1/1] netfilter: nft_fib: Fix for rpath check with VRF devices

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Florian Westphal <fw@strlen.de>:

On Wed, 28 Sep 2022 13:39:08 +0200 you wrote:
> From: Phil Sutter <phil@nwl.cc>
> 
> Analogous to commit b575b24b8eee3 ("netfilter: Fix rpfilter
> dropping vrf packets by mistake") but for nftables fib expression:
> Add special treatment of VRF devices so that typical reverse path
> filtering via 'fib saddr . iif oif' expression works as expected.
> 
> [...]

Here is the summary with links:
  - [1/1] netfilter: nft_fib: Fix for rpath check with VRF devices
    https://git.kernel.org/netdev/net-next/c/2a8a7c0eaa87

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 1/1] netfilter: nft_fib: Fix for rpath check with VRF devices

[Guillaume Nault]

On Wed, Sep 28, 2022 at 01:39:08PM +0200, Florian Westphal wrote:
> From: Phil Sutter <phil@nwl.cc>
> 
> Analogous to commit b575b24b8eee3 ("netfilter: Fix rpfilter
> dropping vrf packets by mistake") but for nftables fib expression:
> Add special treatment of VRF devices so that typical reverse path
> filtering via 'fib saddr . iif oif' expression works as expected.
> 
> Fixes: f6d0cbcf09c50 ("netfilter: nf_tables: add fib expression")
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/ipv4/netfilter/nft_fib_ipv4.c | 3 +++
>  net/ipv6/netfilter/nft_fib_ipv6.c | 6 +++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c
> index b75cac69bd7e..7ade04ff972d 100644
> --- a/net/ipv4/netfilter/nft_fib_ipv4.c
> +++ b/net/ipv4/netfilter/nft_fib_ipv4.c
> @@ -83,6 +83,9 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs,
>  	else
>  		oif = NULL;
>  
> +	if (priv->flags & NFTA_FIB_F_IIF)
> +		fl4.flowi4_oif = l3mdev_master_ifindex_rcu(oif);
> +

Shouldn't we set .flowi4_l3mdev instead of .flowi4_oif?

>  	if (nft_hook(pkt) == NF_INET_PRE_ROUTING &&
>  	    nft_fib_is_loopback(pkt->skb, nft_in(pkt))) {
>  		nft_fib_store_result(dest, priv, nft_in(pkt));
> diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c
> index 8970d0b4faeb..1d7e520d9966 100644
> --- a/net/ipv6/netfilter/nft_fib_ipv6.c
> +++ b/net/ipv6/netfilter/nft_fib_ipv6.c
> @@ -41,6 +41,9 @@ static int nft_fib6_flowi_init(struct flowi6 *fl6, const struct nft_fib *priv,
>  	if (ipv6_addr_type(&fl6->daddr) & IPV6_ADDR_LINKLOCAL) {
>  		lookup_flags |= RT6_LOOKUP_F_IFACE;
>  		fl6->flowi6_oif = get_ifindex(dev ? dev : pkt->skb->dev);
> +	} else if ((priv->flags & NFTA_FIB_F_IIF) &&
> +		   (netif_is_l3_master(dev) || netif_is_l3_slave(dev))) {
> +		fl6->flowi6_oif = dev->ifindex;
>  	}
>  
>  	if (ipv6_addr_type(&fl6->saddr) & IPV6_ADDR_UNICAST)
> @@ -197,7 +200,8 @@ void nft_fib6_eval(const struct nft_expr *expr, struct nft_regs *regs,
>  	if (rt->rt6i_flags & (RTF_REJECT | RTF_ANYCAST | RTF_LOCAL))
>  		goto put_rt_err;
>  
> -	if (oif && oif != rt->rt6i_idev->dev)
> +	if (oif && oif != rt->rt6i_idev->dev &&
> +	    l3mdev_master_ifindex_rcu(rt->rt6i_idev->dev) != oif->ifindex)
>  		goto put_rt_err;
>  
>  	nft_fib_store_result(dest, priv, rt->rt6i_idev->dev);
> -- 
> 2.35.1
> 

Re: [PATCH 1/1] netfilter: nft_fib: Fix for rpath check with VRF devices

[Florian Westphal]

Guillaume Nault <gnault@redhat.com> wrote:

[ CC David Ahern ]

> On Wed, Sep 28, 2022 at 01:39:08PM +0200, Florian Westphal wrote:
> > From: Phil Sutter <phil@nwl.cc>
> > 
> > Analogous to commit b575b24b8eee3 ("netfilter: Fix rpfilter
> > dropping vrf packets by mistake") but for nftables fib expression:
> > Add special treatment of VRF devices so that typical reverse path
> > filtering via 'fib saddr . iif oif' expression works as expected.
> > 
> > Fixes: f6d0cbcf09c50 ("netfilter: nf_tables: add fib expression")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  net/ipv4/netfilter/nft_fib_ipv4.c | 3 +++
> >  net/ipv6/netfilter/nft_fib_ipv6.c | 6 +++++-
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c
> > index b75cac69bd7e..7ade04ff972d 100644
> > --- a/net/ipv4/netfilter/nft_fib_ipv4.c
> > +++ b/net/ipv4/netfilter/nft_fib_ipv4.c
> > @@ -83,6 +83,9 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs,
> >  	else
> >  		oif = NULL;
> >  
> > +	if (priv->flags & NFTA_FIB_F_IIF)
> > +		fl4.flowi4_oif = l3mdev_master_ifindex_rcu(oif);
> > +
> 
> Shouldn't we set .flowi4_l3mdev instead of .flowi4_oif?

No idea.
db53cd3d88dc328dea2e968c9c8d3b4294a8a674 sets both.
rp_filter modules in iptables only set flowi(6)_oif.

David, can you give advice on what the correct fix is?

Then we could change all users in netfilter at once rather than the
current collection of random-looking guesses...

Re: [PATCH 1/1] netfilter: nft_fib: Fix for rpath check with VRF devices

[David Ahern]

On 9/29/22 9:21 AM, Florian Westphal wrote:
> Guillaume Nault <gnault@redhat.com> wrote:
> 
> [ CC David Ahern ]
> 
>> On Wed, Sep 28, 2022 at 01:39:08PM +0200, Florian Westphal wrote:
>>> From: Phil Sutter <phil@nwl.cc>
>>>
>>> Analogous to commit b575b24b8eee3 ("netfilter: Fix rpfilter
>>> dropping vrf packets by mistake") but for nftables fib expression:
>>> Add special treatment of VRF devices so that typical reverse path
>>> filtering via 'fib saddr . iif oif' expression works as expected.
>>>
>>> Fixes: f6d0cbcf09c50 ("netfilter: nf_tables: add fib expression")
>>> Signed-off-by: Phil Sutter <phil@nwl.cc>
>>> Signed-off-by: Florian Westphal <fw@strlen.de>
>>> ---
>>>  net/ipv4/netfilter/nft_fib_ipv4.c | 3 +++
>>>  net/ipv6/netfilter/nft_fib_ipv6.c | 6 +++++-
>>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/net/ipv4/netfilter/nft_fib_ipv4.c b/net/ipv4/netfilter/nft_fib_ipv4.c
>>> index b75cac69bd7e..7ade04ff972d 100644
>>> --- a/net/ipv4/netfilter/nft_fib_ipv4.c
>>> +++ b/net/ipv4/netfilter/nft_fib_ipv4.c
>>> @@ -83,6 +83,9 @@ void nft_fib4_eval(const struct nft_expr *expr, struct nft_regs *regs,
>>>  	else
>>>  		oif = NULL;
>>>  
>>> +	if (priv->flags & NFTA_FIB_F_IIF)
>>> +		fl4.flowi4_oif = l3mdev_master_ifindex_rcu(oif);
>>> +
>>
>> Shouldn't we set .flowi4_l3mdev instead of .flowi4_oif?
> 
> No idea.
> db53cd3d88dc328dea2e968c9c8d3b4294a8a674 sets both.
> rp_filter modules in iptables only set flowi(6)_oif.
> 
> David, can you give advice on what the correct fix is?
> 
> Then we could change all users in netfilter at once rather than the
> current collection of random-looking guesses...

Old usage is setting flow oif and it gets converted to the L3 device if
there is one. The new usage is flow l3mdev, but I only updated places I
new I was testing.

ie.., use l3mdev in the flow struct if you have a VRF test case for it.

Re: [PATCH 1/1] netfilter: nft_fib: Fix for rpath check with VRF devices

[Guillaume Nault]

On Wed, Sep 28, 2022 at 01:39:08PM +0200, Florian Westphal wrote:
> diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c
> index 8970d0b4faeb..1d7e520d9966 100644
> --- a/net/ipv6/netfilter/nft_fib_ipv6.c
> +++ b/net/ipv6/netfilter/nft_fib_ipv6.c
> @@ -41,6 +41,9 @@ static int nft_fib6_flowi_init(struct flowi6 *fl6, const struct nft_fib *priv,
>  	if (ipv6_addr_type(&fl6->daddr) & IPV6_ADDR_LINKLOCAL) {
>  		lookup_flags |= RT6_LOOKUP_F_IFACE;
>  		fl6->flowi6_oif = get_ifindex(dev ? dev : pkt->skb->dev);
> +	} else if ((priv->flags & NFTA_FIB_F_IIF) &&
> +		   (netif_is_l3_master(dev) || netif_is_l3_slave(dev))) {
> +		fl6->flowi6_oif = dev->ifindex;
>  	}

I'm not very familiar with nft code, but it seems dev can be NULL here,
so netif_is_l3_master() can dereference a NULL pointer.

Shouldn't we test dev in the 'else if' condition?

Re: [PATCH 1/1] netfilter: nft_fib: Fix for rpath check with VRF devices

[Florian Westphal]

Guillaume Nault <gnault@redhat.com> wrote:
> On Wed, Sep 28, 2022 at 01:39:08PM +0200, Florian Westphal wrote:
> > diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c
> > index 8970d0b4faeb..1d7e520d9966 100644
> > --- a/net/ipv6/netfilter/nft_fib_ipv6.c
> > +++ b/net/ipv6/netfilter/nft_fib_ipv6.c
> > @@ -41,6 +41,9 @@ static int nft_fib6_flowi_init(struct flowi6 *fl6, const struct nft_fib *priv,
> >  	if (ipv6_addr_type(&fl6->daddr) & IPV6_ADDR_LINKLOCAL) {
> >  		lookup_flags |= RT6_LOOKUP_F_IFACE;
> >  		fl6->flowi6_oif = get_ifindex(dev ? dev : pkt->skb->dev);
> > +	} else if ((priv->flags & NFTA_FIB_F_IIF) &&
> > +		   (netif_is_l3_master(dev) || netif_is_l3_slave(dev))) {
> > +		fl6->flowi6_oif = dev->ifindex;
> >  	}
> 
> I'm not very familiar with nft code, but it seems dev can be NULL here,
> so netif_is_l3_master() can dereference a NULL pointer.

No, this should never be NULL, NFTA_FIB_F_IIF is restricted to
input/prerouting chains.

> Shouldn't we test dev in the 'else if' condition?

We could do that in case it makes review easier.

Re: [PATCH 1/1] netfilter: nft_fib: Fix for rpath check with VRF devices

[Guillaume Nault]

On Fri, Sep 30, 2022 at 04:47:52PM +0200, Florian Westphal wrote:
> Guillaume Nault <gnault@redhat.com> wrote:
> > On Wed, Sep 28, 2022 at 01:39:08PM +0200, Florian Westphal wrote:
> > > diff --git a/net/ipv6/netfilter/nft_fib_ipv6.c b/net/ipv6/netfilter/nft_fib_ipv6.c
> > > index 8970d0b4faeb..1d7e520d9966 100644
> > > --- a/net/ipv6/netfilter/nft_fib_ipv6.c
> > > +++ b/net/ipv6/netfilter/nft_fib_ipv6.c
> > > @@ -41,6 +41,9 @@ static int nft_fib6_flowi_init(struct flowi6 *fl6, const struct nft_fib *priv,
> > >  	if (ipv6_addr_type(&fl6->daddr) & IPV6_ADDR_LINKLOCAL) {
> > >  		lookup_flags |= RT6_LOOKUP_F_IFACE;
> > >  		fl6->flowi6_oif = get_ifindex(dev ? dev : pkt->skb->dev);
> > > +	} else if ((priv->flags & NFTA_FIB_F_IIF) &&
> > > +		   (netif_is_l3_master(dev) || netif_is_l3_slave(dev))) {
> > > +		fl6->flowi6_oif = dev->ifindex;
> > >  	}
> > 
> > I'm not very familiar with nft code, but it seems dev can be NULL here,
> > so netif_is_l3_master() can dereference a NULL pointer.
> 
> No, this should never be NULL, NFTA_FIB_F_IIF is restricted to
> input/prerouting chains.

Thanks, I didn't realise that.

> > Shouldn't we test dev in the 'else if' condition?
> 
> We could do that in case it makes review easier.

Then if it's just to help reviewers, a small comment should be enough.