netdev - Re: VRF IPV6: Icmp6InMsgs incremented on VRF device instead of ingress VLAN device.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Tue, 26 Mar 2019 08:45:49 -0600
From:   David Ahern <dsahern@...il.com>
To:     Sukumar Gopalakrishnan <sukumarg1973@...il.com>
Cc:     netdev@...r.kernel.org
Subject: Re: VRF IPV6: Icmp6InMsgs incremented on VRF device instead of
 ingress VLAN device.

On 3/25/19 3:17 AM, Sukumar Gopalakrishnan wrote:
> PROPOSED FIX:
> =============
> 
> skb->dev is pointing to vrf device (vrf_258) for ingressing IPV6 global
> address instead of VLAN interface . Get the ingress interface from
> IP6CB(head)->iif and increment ICMP counters on the VLAN interface
> instead of VRF device.
> 
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index 63225d1405e3..f4889a14c087 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -806,7 +806,7 @@ out:
> 
>  static int icmpv6_rcv(struct sk_buff *skb)
>  {
> -       struct net_device *dev = skb->dev;
> +       struct net_device *dev = dev_get_by_index_rcu(&init_net,
> icmp6_iif(skb));

you don't need to go from device to index to device and init_net is not
always right. I think something like this would be better:

static struct net_device *icmp6_dev(const struct sk_buff *skb)
{
        struct net_device *dev = skb->dev;

        /* for local traffic to local address, skb dev is the loopback
         * device. Check if there is a dst attached to the skb and if so
         * get the real device index. Same is needed for replies to a link
         * local address on a device enslaved to an L3 master device
         */
        if (unlikely(dev->ifindex == LOOPBACK_IFINDEX ||
                     netif_is_l3_master(skb->dev))) {
                const struct rt6_info *rt6 = skb_rt6_info(skb);

                if (rt6)
                        dev = rt6->rt6i_idev->dev;
        }

        return dev;
}

static int icmp6_iif(const struct sk_buff *skb)
{
        return icmp6_dev(skb)->ifindex;
}

and the you change icmpv6_rcv():

	struct net_device *dev = icmp6_dev(skb);

that avoids the dev to index to dev and the need for a namespace reference.

>         struct inet6_dev *idev = __in6_dev_get(dev);
>         const struct in6_addr *saddr, *daddr;
>         struct icmp6hdr *hdr;
> 
> diff --git a/net/ipv6/reassembly.c b/net/ipv6/reassembly.c
> index 846012eae526..b18f7901b1c3 100644
> --- a/net/ipv6/reassembly.c
> +++ b/net/ipv6/reassembly.c
> @@ -506,7 +506,8 @@ static int ip6_frag_reasm(struct frag_queue *fq, struct
> sk_buff *prev,
>                            skb_network_header_len(head));
> 
>         rcu_read_lock();
> -       __IP6_INC_STATS(net, __in6_dev_get(dev), IPSTATS_MIB_REASMOKS);
> +       __IP6_INC_STATS(net, __in6_dev_get((dev_get_by_index_rcu(net,
> +               IP6CB(head)->iif))), IPSTATS_MIB_REASMOKS);

and here I think this is simpler if you just update dev:
	dev = dev_get_by_index_rcu(net, IP6CB(head)->iif);

and leave the __IP6_INC_STATS as is. Also, I think that should be
inet6_iif(head) and not IP6CB(head)->iif.


>         rcu_read_unlock(); 
>         fq->q.fragments = NULL;
>         fq->q.fragments_tail = NULL; 
> 
> Ingress device's counter is getting incremented correctly.
> 
> /proc/2429/net/dev_snmp6 # cat v6_vl_F4253 | grep Icmp6InMsgs
> Icmp6InMsgs                             1540
> /proc/2429/net/dev_snmp6 # cat vrf_258 | grep Icmp6InMsgs
> Icmp6InMsgs                             0
>