Re: [PATCH net-next v2 2/2] net: ipv6: ioam6: new feature tunsrc

[Justin Iurman]

On 8/16/24 19:35, Jakub Kicinski wrote:
> On Tue, 13 Aug 2024 14:27:23 +0200 Justin Iurman wrote:
> > This patch provides a new feature (i.e., "tunsrc") for the tunnel (i.e.,
> > "encap") mode of ioam6. Just like seg6 already does, except it is
> > attached to a route. The "tunsrc" is optional: when not provided (by
> > default), the automatic resolution is applied. Using "tunsrc" when
> > possible has a benefit: performance. See the comparison:
> >   - before (= "encap" mode): https://ibb.co/bNCzvf7
> >   - after (= "encap" mode with "tunsrc"): https://ibb.co/PT8L6yq
>
> No need to extend the selftests ?

Hi Jakub,

I've been wondering too... Currently, it doesn't check the IPv6 header 
and only focuses on the IOAM header+data. So I came to the conclusion 
that the selftests should not necessarily be updated for that. Although 
I'm not closing the door to a future update to include some extra IPv6 
header checks. I just think it's not required *now* and in this series.

I'll post -v3 to address your comments below (thanks, by the way!).

Cheers,
Justin

> > diff --git a/include/uapi/linux/ioam6_iptunnel.h b/include/uapi/linux/ioam6_iptunnel.h
> > index 38f6a8fdfd34..6cdbd0da7ad8 100644
> > --- a/include/uapi/linux/ioam6_iptunnel.h
> > +++ b/include/uapi/linux/ioam6_iptunnel.h
> > @@ -50,6 +50,13 @@ enum {
> >   	IOAM6_IPTUNNEL_FREQ_K,		/* u32 */
> >   	IOAM6_IPTUNNEL_FREQ_N,		/* u32 */
> >   
> > +	/* Tunnel src address.
> > +	 * For encap,auto modes.
> > +	 * Optional (automatic if
> > +	 * not provided).
>
> The wrapping of this text appears excessive
>
> > +	 */
> > +	IOAM6_IPTUNNEL_SRC,		/* struct in6_addr */
> > +
> >   	__IOAM6_IPTUNNEL_MAX,
> >   };
>
> > @@ -178,6 +186,23 @@ static int ioam6_build_state(struct net *net, struct nlattr *nla,
> >   	ilwt->freq.n = freq_n;
> >   
> >   	ilwt->mode = mode;
> > +
> > +	if (!tb[IOAM6_IPTUNNEL_SRC]) {
> > +		ilwt->has_tunsrc = false;
> > +	} else {
> > +		ilwt->has_tunsrc = true;
> > +		ilwt->tunsrc = nla_get_in6_addr(tb[IOAM6_IPTUNNEL_SRC]);
> > +
> > +		if (ipv6_addr_any(&ilwt->tunsrc)) {
> > +			dst_cache_destroy(&ilwt->cache);
> > +			kfree(lwt);
>
> Let's put the cleanup at the end of the function, and use a goto / label
> to jump there.
>
> > +			NL_SET_ERR_MSG_ATTR(extack, tb[IOAM6_IPTUNNEL_SRC],
> > +					    "invalid tunnel source address");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> >   	if (tb[IOAM6_IPTUNNEL_DST])
> >   		ilwt->tundst = nla_get_in6_addr(tb[IOAM6_IPTUNNEL_DST]);
> >   
> > @@ -257,6 +282,8 @@ static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
> >   
> >   static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
> >   			  struct ioam6_lwt_encap *tuninfo,
> > +			  bool has_tunsrc,
> > +			  struct in6_addr *tunsrc,
> >   			  struct in6_addr *tundst)
> >   {
> >   	struct dst_entry *dst = skb_dst(skb);
> > @@ -286,8 +313,13 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
> >   	hdr->nexthdr = NEXTHDR_HOP;
> >   	hdr->payload_len = cpu_to_be16(skb->len - sizeof(*hdr));
> >   	hdr->daddr = *tundst;
> > -	ipv6_dev_get_saddr(net, dst->dev, &hdr->daddr,
> > -			   IPV6_PREFER_SRC_PUBLIC, &hdr->saddr);
> > +
> > +	if (has_tunsrc) {
> > +		memcpy(&hdr->saddr, tunsrc, sizeof(*tunsrc));
> > +	} else {
> > +		ipv6_dev_get_saddr(net, dst->dev, &hdr->daddr,
> > +				   IPV6_PREFER_SRC_PUBLIC, &hdr->saddr);
> > +	}
>
> single statement branches, no need for {}
>
> >   	skb_postpush_rcsum(skb, hdr, len);
> >