[PATCH nf-next] netfilter: Dissect flow after packet mangling

[PATCH nf-next] netfilter: Dissect flow after packet mangling

[Ido Schimmel]

From: Ido Schimmel <idosch@nvidia.com>

Netfilter tries to reroute mangled packets as a different route might
need to be used following the mangling. When this happens, netfilter
does not populate the IP protocol, the source port and the destination
port in the flow key. Therefore, FIB rules that match on these fields
are ignored and packets can be misrouted.

Solve this by dissecting the outer flow and populating the flow key
before rerouting the packet. Note that flow dissection only happens when
FIB rules that match on these fields are installed, so in the common
case there should not be a penalty.

Reported-by: Michal Soltys <msoltyspl@yandex.pl>
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
Targeting at nf-next since this use case never worked.
---
 net/ipv4/netfilter.c | 2 ++
 net/ipv6/netfilter.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/net/ipv4/netfilter.c b/net/ipv4/netfilter.c
index 7c841037c533..aff707988e23 100644
--- a/net/ipv4/netfilter.c
+++ b/net/ipv4/netfilter.c
@@ -25,6 +25,7 @@ int ip_route_me_harder(struct net *net, struct sock *sk, struct sk_buff *skb, un
 	__be32 saddr = iph->saddr;
 	__u8 flags;
 	struct net_device *dev = skb_dst(skb)->dev;
+	struct flow_keys flkeys;
 	unsigned int hh_len;
 
 	sk = sk_to_full_sk(sk);
@@ -48,6 +49,7 @@ int ip_route_me_harder(struct net *net, struct sock *sk, struct sk_buff *skb, un
 		fl4.flowi4_oif = l3mdev_master_ifindex(dev);
 	fl4.flowi4_mark = skb->mark;
 	fl4.flowi4_flags = flags;
+	fib4_rules_early_flow_dissect(net, skb, &fl4, &flkeys);
 	rt = ip_route_output_key(net, &fl4);
 	if (IS_ERR(rt))
 		return PTR_ERR(rt);
diff --git a/net/ipv6/netfilter.c b/net/ipv6/netfilter.c
index ab9a279dd6d4..6ab710b5a1a8 100644
--- a/net/ipv6/netfilter.c
+++ b/net/ipv6/netfilter.c
@@ -24,6 +24,7 @@ int ip6_route_me_harder(struct net *net, struct sock *sk_partial, struct sk_buff
 {
 	const struct ipv6hdr *iph = ipv6_hdr(skb);
 	struct sock *sk = sk_to_full_sk(sk_partial);
+	struct flow_keys flkeys;
 	unsigned int hh_len;
 	struct dst_entry *dst;
 	int strict = (ipv6_addr_type(&iph->daddr) &
@@ -38,6 +39,7 @@ int ip6_route_me_harder(struct net *net, struct sock *sk_partial, struct sk_buff
 	};
 	int err;
 
+	fib6_rules_early_flow_dissect(net, skb, &fl6, &flkeys);
 	dst = ip6_route_output(net, sk, &fl6);
 	err = dst->error;
 	if (err) {
-- 
2.30.2

Re: [PATCH nf-next] netfilter: Dissect flow after packet mangling

[David Ahern]

On 4/11/21 1:32 PM, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@nvidia.com>
> 
> Netfilter tries to reroute mangled packets as a different route might
> need to be used following the mangling. When this happens, netfilter
> does not populate the IP protocol, the source port and the destination
> port in the flow key. Therefore, FIB rules that match on these fields
> are ignored and packets can be misrouted.
> 
> Solve this by dissecting the outer flow and populating the flow key
> before rerouting the packet. Note that flow dissection only happens when
> FIB rules that match on these fields are installed, so in the common
> case there should not be a penalty.
> 
> Reported-by: Michal Soltys <msoltyspl@yandex.pl>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> ---
> Targeting at nf-next since this use case never worked.
> ---
>  net/ipv4/netfilter.c | 2 ++
>  net/ipv6/netfilter.c | 2 ++
>  2 files changed, 4 insertions(+)
> 

Once this goes in, can you add tests to one of the selftest scripts
(e.g., fib_rule_tests.sh)?

Re: [PATCH nf-next] netfilter: Dissect flow after packet mangling

[Ido Schimmel]

On Sun, Apr 11, 2021 at 06:18:05PM -0700, David Ahern wrote:
> On 4/11/21 1:32 PM, Ido Schimmel wrote:
> > From: Ido Schimmel <idosch@nvidia.com>
> > 
> > Netfilter tries to reroute mangled packets as a different route might
> > need to be used following the mangling. When this happens, netfilter
> > does not populate the IP protocol, the source port and the destination
> > port in the flow key. Therefore, FIB rules that match on these fields
> > are ignored and packets can be misrouted.
> > 
> > Solve this by dissecting the outer flow and populating the flow key
> > before rerouting the packet. Note that flow dissection only happens when
> > FIB rules that match on these fields are installed, so in the common
> > case there should not be a penalty.
> > 
> > Reported-by: Michal Soltys <msoltyspl@yandex.pl>
> > Signed-off-by: Ido Schimmel <idosch@nvidia.com>
> > ---
> > Targeting at nf-next since this use case never worked.
> > ---
> >  net/ipv4/netfilter.c | 2 ++
> >  net/ipv6/netfilter.c | 2 ++
> >  2 files changed, 4 insertions(+)
> > 
> 
> Once this goes in, can you add tests to one of the selftest scripts
> (e.g., fib_rule_tests.sh)?

Yes. I used Michal's scripts from here [1] to test. Will try to simplify
it for a test case.

[1] https://lore.kernel.org/netdev/6b707dde-c6f0-ca3e-e817-a09c1e6b3f00@yandex.pl/

Re: [PATCH nf-next] netfilter: Dissect flow after packet mangling

[Michal Soltys]

On 4/12/21 8:51 AM, Ido Schimmel wrote:
> On Sun, Apr 11, 2021 at 06:18:05PM -0700, David Ahern wrote:
>> On 4/11/21 1:32 PM, Ido Schimmel wrote:
>>> From: Ido Schimmel <idosch@nvidia.com>
>>> <cut>
>>>
>>
>> Once this goes in, can you add tests to one of the selftest scripts
>> (e.g., fib_rule_tests.sh)?
> 
> Yes. I used Michal's scripts from here [1] to test. Will try to simplify
> it for a test case.
> 
> [1] https://lore.kernel.org/netdev/6b707dde-c6f0-ca3e-e817-a09c1e6b3f00@yandex.pl/
> 

Regarding those scripts:

- the commented out `-j TOS --set-tos 0x02` falls into ECN bits, so it's 
somewhat incorrect/obsolete
- the uidrange selector (that was also ignored) is missing in the 
sequence of ip rules

Re: [PATCH nf-next] netfilter: Dissect flow after packet mangling

[Ido Schimmel]

On Mon, Apr 12, 2021 at 03:28:21PM +0200, Michal Soltys wrote:
> On 4/12/21 8:51 AM, Ido Schimmel wrote:
> > On Sun, Apr 11, 2021 at 06:18:05PM -0700, David Ahern wrote:
> > > On 4/11/21 1:32 PM, Ido Schimmel wrote:
> > > > From: Ido Schimmel <idosch@nvidia.com>
> > > > <cut>
> > > > 
> > > 
> > > Once this goes in, can you add tests to one of the selftest scripts
> > > (e.g., fib_rule_tests.sh)?
> > 
> > Yes. I used Michal's scripts from here [1] to test. Will try to simplify
> > it for a test case.
> > 
> > [1] https://lore.kernel.org/netdev/6b707dde-c6f0-ca3e-e817-a09c1e6b3f00@yandex.pl/
> > 
> 
> Regarding those scripts:
> 
> - the commented out `-j TOS --set-tos 0x02` falls into ECN bits, so it's
> somewhat incorrect/obsolete
> - the uidrange selector (that was also ignored) is missing in the sequence
> of ip rules

I verified that with the patch, after adding mangling rules with
ip{,6}tables, packets continue to flow via right2. Can you test the
patch and verify it works as you expect?

Re: [PATCH nf-next] netfilter: Dissect flow after packet mangling

[Michal Soltys]

On 4/12/21 8:18 PM, Ido Schimmel wrote:
> On Mon, Apr 12, 2021 at 03:28:21PM +0200, Michal Soltys wrote:
>> On 4/12/21 8:51 AM, Ido Schimmel wrote:
>>> On Sun, Apr 11, 2021 at 06:18:05PM -0700, David Ahern wrote:
>>>> On 4/11/21 1:32 PM, Ido Schimmel wrote:
>>>>> From: Ido Schimmel <idosch@nvidia.com>
>>>>> <cut>
>>>>>
>>>>
>>>> Once this goes in, can you add tests to one of the selftest scripts
>>>> (e.g., fib_rule_tests.sh)?
>>>
>>> Yes. I used Michal's scripts from here [1] to test. Will try to simplify
>>> it for a test case.
>>>
>>> [1] https://lore.kernel.org/netdev/6b707dde-c6f0-ca3e-e817-a09c1e6b3f00@yandex.pl/
>>>
>>
>> Regarding those scripts:
>>
>> - the commented out `-j TOS --set-tos 0x02` falls into ECN bits, so it's
>> somewhat incorrect/obsolete
>> - the uidrange selector (that was also ignored) is missing in the sequence
>> of ip rules
> 
> I verified that with the patch, after adding mangling rules with
> ip{,6}tables, packets continue to flow via right2. Can you test the
> patch and verify it works as you expect?
> 

Tested today - all recently added selectors are working correctly. Thanks.