netdev - Re: [PATCH net-next 1/2] ipv6: ioam: Add support for the ip6ip6 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <2092322692.108322349.1633015157710.JavaMail.zimbra@uliege.be>
Date:   Thu, 30 Sep 2021 17:19:17 +0200 (CEST)
From:   Justin Iurman <justin.iurman@...ege.be>
To:     David Ahern <dsahern@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        yoshfuji@...ux-ipv6.org, dsahern@...nel.org
Subject: Re: [PATCH net-next 1/2] ipv6: ioam: Add support for the ip6ip6
 encapsulation

>>  static const struct nla_policy ioam6_iptunnel_policy[IOAM6_IPTUNNEL_MAX + 1] = {
>> -	[IOAM6_IPTUNNEL_TRACE]	= NLA_POLICY_EXACT_LEN(sizeof(struct ioam6_trace_hdr)),
>> +	[IOAM6_IPTUNNEL_TRACE]	= NLA_POLICY_EXACT_LEN(sizeof(struct
>> ioam6_iptunnel_trace)),
> 
> you can't do that. Once a kernel is released with a given UAPI, it can
> not be changed. You could go the other way and handle
> 
> struct ioam6_iptunnel_trace {
> +	struct ioam6_trace_hdr trace;
> +	__u8 mode;
> +	struct in6_addr tundst;	/* unused for inline mode */
> +};

Makes sense. But I'm not sure what you mean by "go the other way". Should I handle ioam6_iptunnel_trace as well, in addition to ioam6_trace_hdr, so that the uapi is backward compatible?

> Also, no gaps in uapi. Make sure all holes are stated; an anonymous
> entry is best.

Would something like this do the trick?

struct ioam6_iptunnel_trace {
	struct ioam6_trace_hdr trace;
	__u8 mode;
	union { /* anonymous field only used by both the encap and auto modes */
		struct in6_addr tundst;
	};
};

>>  };
>>  
>> -static int nla_put_ioam6_trace(struct sk_buff *skb, int attrtype,
>> -			       struct ioam6_trace_hdr *trace)
>> -{
>> -	struct ioam6_trace_hdr *data;
>> -	struct nlattr *nla;
>> -	int len;
>> -
>> -	len = sizeof(*trace);
>> -
>> -	nla = nla_reserve(skb, attrtype, len);
>> -	if (!nla)
>> -		return -EMSGSIZE;
>> -
>> -	data = nla_data(nla);
>> -	memcpy(data, trace, len);
>> -
>> -	return 0;
>> -}
>> -
> 
> quite a bit of the change seems like refactoring from existing feature
> to allow the new ones. Please submit refactoring changes as a
> prerequisite patch. The patch that introduces your new feature should be
> focused solely on what is needed to implement that feature.

+1, will do.