netdev - Re: [net-next v2 1/1] net: seg6: Add support for SRv6 Headend Reduced Encapsulation

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 10 Jun 2022 21:21:08 +0300
From:   Anton Makarov <antonmakarov11235@...il.com>
To:     Andrea Mayer <andrea.mayer@...roma2.it>
Cc:     Anton Makarov <antonmakarov11235@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, david.lebrun@...ouvain.be,
        netdev@...r.kernel.org,
        Stefano Salsano <stefano.salsano@...roma2.it>,
        Paolo Lungaroni <paolo.lungaroni@...roma2.it>,
        Ahmed Abdelsalam <ahabdels.dev@...il.com>
Subject: Re: [net-next v2 1/1] net: seg6: Add support for SRv6 Headend
 Reduced Encapsulation

Hi Andrea,
Thank you very much for your feedback! Plese look at my response inline
and let me know what you think about that. Many thanks!

On Fri, 10 Jun 2022 13:59:58 +0200
Andrea Mayer <andrea.mayer@...roma2.it> wrote:

> Hi Anton,
> please see my comments inline, thanks.
> 
> On Thu,  9 Jun 2022 16:27:50 +0300
> Anton Makarov <antonmakarov11235@...il.com> wrote:
> 
> > SRv6 Headend H.Encaps.Red and H.Encaps.L2.Red behaviors are implemented
> > accordingly to RFC 8986. The H.Encaps.Red is an optimization of
> > The H.Encaps behavior. The H.Encaps.L2.Red is an optimization of
> > the H.Encaps.L2 behavior. Both new behaviors reduce the length of
> > the SRH by excluding the first SID in the SRH of the pushed IPv6 header.
> > The first SID is only placed in the Destination Address field
> > of the pushed IPv6 header.
> > 
> > The push of the SRH is omitted when the SRv6 Policy only contains
> > one segment.
> > 
> > Signed-off-by: Anton Makarov <anton.makarov11235@...il.com>
> > 
> > ...
> >  
> > +/* encapsulate an IPv6 packet within an outer IPv6 header with reduced SRH */
> > +int seg6_do_srh_encap_red(struct sk_buff *skb, struct ipv6_sr_hdr *osrh, int proto)
> > +{
> > +	struct dst_entry *dst = skb_dst(skb);
> > +	struct net *net = dev_net(dst->dev);
> > +	struct ipv6hdr *hdr, *inner_hdr6;
> > +	struct iphdr *inner_hdr4;
> > +	struct ipv6_sr_hdr *isrh;
> > +	int hdrlen = 0, tot_len, err;
> 
> I suppose we should stick with the reverse XMAS tree code style.

Sure, no problem.

> 
> > +	__be32 flowlabel = 0;
> 
> this initialization is unnecessary since the variable is accessed for the first
> time in writing, later in the code.

Sorry, missed this extra action. You are correct.

> 
> > +	if (osrh->first_segment > 0)
> > +		hdrlen = (osrh->hdrlen - 1) << 3;
> > +
> > +	tot_len = hdrlen + sizeof(struct ipv6hdr);
> > +
> > +	err = skb_cow_head(skb, tot_len + skb->mac_len);
> > +	if (unlikely(err))
> > +		return err;
> > +
> > +	inner_hdr6 = ipv6_hdr(skb);
> > +	inner_hdr4 = ip_hdr(skb);
> 
> inner_hdr4 is only used in the *if* block that follows later on.

Do you mean it has to be defined inside *if* block and assigned via
inner_ip_hdr()?

> 
> > +	flowlabel = seg6_make_flowlabel(net, skb, inner_hdr6);
> > +
> > +	skb_push(skb, tot_len);
> > +	skb_reset_network_header(skb);
> > +	skb_mac_header_rebuild(skb);
> > +	hdr = ipv6_hdr(skb);
> > +
> > +	memset(skb->cb, 0, sizeof(skb->cb));
> 
> is there a specific reason why we should consider the whole CB size and not
> only the part covered by the struct inet6_skb_parm?

Oh yes, memset(IP6CB(skb), 0, sizeof(*IP6CB(skb))) would be better. You
are correct.

> 
> > +	IP6CB(skb)->iif = skb->skb_iif;
> > +
> > +	if (skb->protocol == htons(ETH_P_IPV6)) {
> > +		ip6_flow_hdr(hdr, ip6_tclass(ip6_flowinfo(inner_hdr6)),
> > +			     flowlabel);
> > +		hdr->hop_limit = inner_hdr6->hop_limit;
> > +	} else if (skb->protocol == htons(ETH_P_IP)) {
> > +		ip6_flow_hdr(hdr, (unsigned int) inner_hdr4->tos, flowlabel);
> > +		hdr->hop_limit = inner_hdr4->ttl;
> > +	}
> > +
> 
> Don't IPv4 and IPv6 cover all possible cases?

In fact while case SEG6_IPTUN_MODE_ENCAP in seg6_do_srh() does
preliminary check of protocol value, case SEG6_IPTUN_MODE_L2ENCAP does
not. So potentially skb->protocol can be of any value. Although
additional check brings extra impact on performance, sure.

> 
> > +	skb->protocol = htons(ETH_P_IPV6);
> > +
> > +	hdr->daddr = osrh->segments[osrh->first_segment];
> > +	hdr->version = 6;
> > +
> > +	if (osrh->first_segment > 0) {
> > +		hdr->nexthdr = NEXTHDR_ROUTING;
> > +
> > +		isrh = (void *)hdr + sizeof(struct ipv6hdr);
> > +		memcpy(isrh, osrh, hdrlen);
> > +
> > +		isrh->nexthdr = proto;
> > +		isrh->first_segment--;
> > +		isrh->hdrlen -= 2;
> > +	} else {
> > +		hdr->nexthdr = proto;
> > +	}
> > +
> > +	set_tun_src(net, dst->dev, &hdr->daddr, &hdr->saddr);
> > +
> > +#ifdef CONFIG_IPV6_SEG6_HMAC
> > +	if (osrh->first_segment > 0 && sr_has_hmac(isrh)) {
> > +		err = seg6_push_hmac(net, &hdr->saddr, isrh);
> > +		if (unlikely(err))
> > +			return err;
> > +	}
> > +#endif
> > +
> 
> When there is only one SID and HMAC is configured, the SRH is not kept.
> Aren't we losing information this way?

Yes, but HMAC is just an optional part of SRH. RFC 8986 allows us to
omit entire SRH in reduced encapsulation when the SRv6 Policy only
contains one segment. And it seems to be the most usefull approach as
far as:
1) About all hardware implementations do not procede HMAC at all
2) Too many networking guys have a great concern about huge overhead of
SRv6 in compare with MPLS, so they are not happy to get extra 256 bits
3) If one consider HMAC mandatory then there is still basic (not
reduced) encapsulation option

What do you think about it?

> 
> Andrea

Anton