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: <0ce98a52-e9fe-9b5c-68ca-f81c88e021ab@gmail.com>
Date:   Thu, 30 Sep 2021 12:20:21 -0600
From:   David Ahern <dsahern@...il.com>
To:     Justin Iurman <justin.iurman@...ege.be>
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

On 9/30/21 9:19 AM, Justin Iurman wrote:
>>>  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?

by "the other way" I meant let ioam6_trace_hdr be the top element in the
new ioam6_iptunnel_trace struct. If the IOAM6_IPTUNNEL_TRACE size ==
ioam6_trace_hdr then you know it is the legacy argument vs sizeof
ioam6_iptunnel_trace which is the new.

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

By anonymous filling of the holes I meant something like:

struct ioam6_iptunnel_trace {
	struct ioam6_trace_hdr trace;
	__u8 mode;
	__u8 :8;
	__u16 :16;

	struct in6_addr tundst;
};

Use pahole to check that struct for proper alignment of the entries as
desired (4-byte or 8-byte aligned).

> 
>>>  };
>>>  
>>> -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.
>