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

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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <47b13aef-29bb-46e6-92bc-8fd24391b05a@uliege.be>
Date: Sat, 17 Aug 2024 14:57:39 +0200
From: Justin Iurman <justin.iurman@...ege.be>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, dsahern@...nel.org,
 edumazet@...gle.com, pabeni@...hat.com, linux-kernel@...r.kernel.org,
 justin.iurman@...ege.be
Subject: Re: [PATCH net-next v2 2/2] net: ipv6: ioam6: new feature tunsrc

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);
>>