| Message ID | 20211021144857.29714-3-fw@strlen.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vrf: rework interaction with netfilter/conntrack | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Series has a cover letter |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: davem@davemloft.net kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | fail | Errors and warnings before: 2 this patch: 3 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 82 lines checked |
| netdev/build_allmodconfig_warn | fail | Errors and warnings before: 2 this patch: 3 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On Thu, 21 Oct 2021 16:48:57 +0200 Florian Westphal wrote:
> + nf_ct_set(skb, 0, IP_CT_UNTRACKED);
drivers/net/vrf.c:431:32: warning: Using plain integer as NULL pointer
On 21/10/2021 16:48, Florian Westphal wrote: > The VRF driver invokes netfilter for output+postrouting hooks so that users > can create rules that check for 'oif $vrf' rather than lower device name. > > This is a problem when NAT rules are configured. > > To avoid any conntrack involvement in round 1, tag skbs as 'untracked' > to prevent conntrack from picking them up. > > This gets cleared before the packet gets handed to the ip stack so > conntrack will be active on the second iteration. > > For ingress, conntrack has already been done before the packet makes it > to the vrf driver, with this patch egress does connection tracking with > lower/physical device as well. > > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > drivers/net/vrf.c | 28 ++++++++++++++++++++++++---- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c > index bf2fac913942..c813d03159bf 100644 > --- a/drivers/net/vrf.c > +++ b/drivers/net/vrf.c > @@ -35,6 +35,7 @@ > #include <net/l3mdev.h> > #include <net/fib_rules.h> > #include <net/netns/generic.h> > +#include <net/netfilter/nf_conntrack.h> > > #define DRV_NAME "vrf" > #define DRV_VERSION "1.1" > @@ -424,12 +425,26 @@ static int vrf_local_xmit(struct sk_buff *skb, struct net_device *dev, > return NETDEV_TX_OK; > } > > +static void vrf_nf_set_untracked(struct sk_buff *skb) > +{ > + if (skb_get_nfct(skb) == 0) > + nf_ct_set(skb, 0, IP_CT_UNTRACKED); > +} > + > +static void vrf_nf_reset_ct(struct sk_buff *skb) > +{ > + if (skb_get_nfct(skb) == IP_CT_UNTRACKED) > + nf_reset_ct(skb); > +} > + Isn't it possible that skb was marked UNTRACKED before entering this path, by a rule? In such case 'set_untrackd' will do nothing, but 'reset_ct' will clear UNTRACKED status that was set elswhere. It seems wrong, am I missing something? > #if IS_ENABLED(CONFIG_IPV6) > static int vrf_ip6_local_out(struct net *net, struct sock *sk, > struct sk_buff *skb) > { > int err; > > + vrf_nf_reset_ct(skb); > + > err = nf_hook(NFPROTO_IPV6, NF_INET_LOCAL_OUT, net, > sk, skb, NULL, skb_dst(skb)->dev, dst_output); > > @@ -508,6 +523,8 @@ static int vrf_ip_local_out(struct net *net, struct sock *sk, > { > int err; > > + vrf_nf_reset_ct(skb); > + > err = nf_hook(NFPROTO_IPV4, NF_INET_LOCAL_OUT, net, sk, > skb, NULL, skb_dst(skb)->dev, dst_output); > if (likely(err == 1)) > @@ -626,8 +643,7 @@ static void vrf_finish_direct(struct sk_buff *skb) > skb_pull(skb, ETH_HLEN); > } > > - /* reset skb device */ > - nf_reset_ct(skb); > + vrf_nf_reset_ct(skb); > } > > #if IS_ENABLED(CONFIG_IPV6) > @@ -641,7 +657,7 @@ static int vrf_finish_output6(struct net *net, struct sock *sk, > struct neighbour *neigh; > int ret; > > - nf_reset_ct(skb); > + vrf_nf_reset_ct(skb); > > skb->protocol = htons(ETH_P_IPV6); > skb->dev = dev; > @@ -752,6 +768,8 @@ static struct sk_buff *vrf_ip6_out_direct(struct net_device *vrf_dev, > > skb->dev = vrf_dev; > > + vrf_nf_set_untracked(skb); > + > err = nf_hook(NFPROTO_IPV6, NF_INET_LOCAL_OUT, net, sk, > skb, NULL, vrf_dev, vrf_ip6_out_direct_finish); > > @@ -858,7 +876,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s > struct neighbour *neigh; > bool is_v6gw = false; > > - nf_reset_ct(skb); > + vrf_nf_reset_ct(skb); > > /* Be paranoid, rather than too clever. */ > if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) { > @@ -980,6 +998,8 @@ static struct sk_buff *vrf_ip_out_direct(struct net_device *vrf_dev, > > skb->dev = vrf_dev; > > + vrf_nf_set_untracked(skb); > + > err = nf_hook(NFPROTO_IPV4, NF_INET_LOCAL_OUT, net, sk, > skb, NULL, vrf_dev, vrf_ip_out_direct_finish); > >
Eugene Crosser <crosser@average.org> wrote: > > +static void vrf_nf_set_untracked(struct sk_buff *skb) > > +{ > > + if (skb_get_nfct(skb) == 0) > > + nf_ct_set(skb, 0, IP_CT_UNTRACKED); > > +} > > + > > +static void vrf_nf_reset_ct(struct sk_buff *skb) > > +{ > > + if (skb_get_nfct(skb) == IP_CT_UNTRACKED) > > + nf_reset_ct(skb); > > +} > > + > > Isn't it possible that skb was marked UNTRACKED before entering this path, by a > rule? I don't think so, it should be called before any ruleset evaluation has taken place. > In such case 'set_untrackd' will do nothing, but 'reset_ct' will clear > UNTRACKED status that was set elswhere. It seems wrong, am I missing something? No, thats the catch. I can't find a better option. I can add a patch to disable all of the NF_HOOK() invocations from vrf which removes the ability to filter on vrf interface names. The option to add a caller_id to nf_hook_state struct (so conntrack/nat can detect when they are called from the vrf hooks) either needs copypastry of entire NF_HOOK* inline functions into vrf (so the 'is-vrf' flag can be enabled) or yet another argument to NF_HOOK(). It also leaks even more 'is vrf' checks into conntrack.
Florian Westphal <fw@strlen.de> wrote: > Eugene Crosser <crosser@average.org> wrote: > > In such case 'set_untrackd' will do nothing, but 'reset_ct' will clear > > UNTRACKED status that was set elswhere. It seems wrong, am I missing something? > > No, thats the catch. I can't find a better option. To clarify, existing code has unconditional reset, so existing rulesets that set 'notrack' in the first (vrf) round do not affect the second round. This feature/bug would remain, which sucks but I can't think of a saner alternative.
diff --git a/drivers/net/vrf.c b/drivers/net/vrf.c index bf2fac913942..c813d03159bf 100644 --- a/drivers/net/vrf.c +++ b/drivers/net/vrf.c @@ -35,6 +35,7 @@ #include <net/l3mdev.h> #include <net/fib_rules.h> #include <net/netns/generic.h> +#include <net/netfilter/nf_conntrack.h> #define DRV_NAME "vrf" #define DRV_VERSION "1.1" @@ -424,12 +425,26 @@ static int vrf_local_xmit(struct sk_buff *skb, struct net_device *dev, return NETDEV_TX_OK; } +static void vrf_nf_set_untracked(struct sk_buff *skb) +{ + if (skb_get_nfct(skb) == 0) + nf_ct_set(skb, 0, IP_CT_UNTRACKED); +} + +static void vrf_nf_reset_ct(struct sk_buff *skb) +{ + if (skb_get_nfct(skb) == IP_CT_UNTRACKED) + nf_reset_ct(skb); +} + #if IS_ENABLED(CONFIG_IPV6) static int vrf_ip6_local_out(struct net *net, struct sock *sk, struct sk_buff *skb) { int err; + vrf_nf_reset_ct(skb); + err = nf_hook(NFPROTO_IPV6, NF_INET_LOCAL_OUT, net, sk, skb, NULL, skb_dst(skb)->dev, dst_output); @@ -508,6 +523,8 @@ static int vrf_ip_local_out(struct net *net, struct sock *sk, { int err; + vrf_nf_reset_ct(skb); + err = nf_hook(NFPROTO_IPV4, NF_INET_LOCAL_OUT, net, sk, skb, NULL, skb_dst(skb)->dev, dst_output); if (likely(err == 1)) @@ -626,8 +643,7 @@ static void vrf_finish_direct(struct sk_buff *skb) skb_pull(skb, ETH_HLEN); } - /* reset skb device */ - nf_reset_ct(skb); + vrf_nf_reset_ct(skb); } #if IS_ENABLED(CONFIG_IPV6) @@ -641,7 +657,7 @@ static int vrf_finish_output6(struct net *net, struct sock *sk, struct neighbour *neigh; int ret; - nf_reset_ct(skb); + vrf_nf_reset_ct(skb); skb->protocol = htons(ETH_P_IPV6); skb->dev = dev; @@ -752,6 +768,8 @@ static struct sk_buff *vrf_ip6_out_direct(struct net_device *vrf_dev, skb->dev = vrf_dev; + vrf_nf_set_untracked(skb); + err = nf_hook(NFPROTO_IPV6, NF_INET_LOCAL_OUT, net, sk, skb, NULL, vrf_dev, vrf_ip6_out_direct_finish); @@ -858,7 +876,7 @@ static int vrf_finish_output(struct net *net, struct sock *sk, struct sk_buff *s struct neighbour *neigh; bool is_v6gw = false; - nf_reset_ct(skb); + vrf_nf_reset_ct(skb); /* Be paranoid, rather than too clever. */ if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) { @@ -980,6 +998,8 @@ static struct sk_buff *vrf_ip_out_direct(struct net_device *vrf_dev, skb->dev = vrf_dev; + vrf_nf_set_untracked(skb); + err = nf_hook(NFPROTO_IPV4, NF_INET_LOCAL_OUT, net, sk, skb, NULL, vrf_dev, vrf_ip_out_direct_finish);
The VRF driver invokes netfilter for output+postrouting hooks so that users can create rules that check for 'oif $vrf' rather than lower device name. This is a problem when NAT rules are configured. To avoid any conntrack involvement in round 1, tag skbs as 'untracked' to prevent conntrack from picking them up. This gets cleared before the packet gets handed to the ip stack so conntrack will be active on the second iteration. For ingress, conntrack has already been done before the packet makes it to the vrf driver, with this patch egress does connection tracking with lower/physical device as well. Signed-off-by: Florian Westphal <fw@strlen.de> --- drivers/net/vrf.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)