diff mbox series

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

Message ID 20220928113908.4525-2-fw@strlen.de (mailing list archive)
State Accepted
Commit 2a8a7c0eaa8747c16aa4a48d573aa920d5c00a5c
Delegated to: Netdev Maintainers
Headers show
Series [1/1] netfilter: nft_fib: Fix for rpath check with VRF devices | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers fail 1 blamed authors not CCed: pablo@netfilter.org; 5 maintainers not CCed: dsahern@kernel.org pablo@netfilter.org yoshfuji@linux-ipv6.org coreteam@netfilter.org kadlec@netfilter.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 27 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Florian Westphal Sept. 28, 2022, 11:39 a.m. UTC 
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(-)

Comments

patchwork-bot+netdevbpf@kernel.org Sept. 28, 2022, 5:40 p.m. UTC | #1 
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!
Guillaume Nault Sept. 29, 2022, 4:10 p.m. UTC | #2 
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
>
Florian Westphal Sept. 29, 2022, 4:21 p.m. UTC | #3 
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...
David Ahern Sept. 29, 2022, 5:54 p.m. UTC | #4 
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.
Guillaume Nault Sept. 30, 2022, 2:10 p.m. UTC | #5 
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?
Florian Westphal Sept. 30, 2022, 2:47 p.m. UTC | #6 
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.
Guillaume Nault Sept. 30, 2022, 3:08 p.m. UTC | #7 
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.
diff mbox series

Patch

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);