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]

Date:   Fri, 1 Oct 2021 13:38:44 +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?
> 
> 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.

OK, I see. The problem is ioam6_trace_hdr must be the last entry because of its last field, which is "__u8 data[0]". But, anyway, I could still apply the same kind of logic with the size.

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

By reading your example, I'm not sure we're talking about the same thing. Actually, do you refer to the fact that the ioam6_trace_hdr field must be 8n-aligned? If so, I don't see any static way to do that (i.e., by adding anonymous fields as you did) since it depends on the size of the data field I mentioned above.  The size can either be 8n-aligned already, or 4n-aligned in which case we add a PadN (1 2 0 0) at the end of the data field.