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@xxxxxxxxx>
> ---
> 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);
>
>
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
