netdev - Re: [PATCH v2 iproute2 net-next] gre6: add collect metadata support

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: <CALDO+SYNiAhk8-P1jwx06e2nd9xpT93=fFWeDcSXqgxihTKk5g@mail.gmail.com>
Date:   Thu, 7 Dec 2017 08:43:47 -0800
From:   William Tu <u9012063@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Stephen Hemminger <stephen@...workplumber.org>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 iproute2 net-next] gre6: add collect metadata support

On Tue, Dec 5, 2017 at 5:40 PM, Daniel Borkmann <daniel@...earbox.net> wrote:
> On 12/06/2017 02:07 AM, Stephen Hemminger wrote:
>> On Tue,  5 Dec 2017 15:10:37 -0800
>> William Tu <u9012063@...il.com> wrote:
>>
>>> diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
>>> index a6a10e577b1f..eb04f887c940 100644
>>> --- a/man/man8/ip-link.8.in
>>> +++ b/man/man8/ip-link.8.in
>>> @@ -755,6 +755,8 @@ the following additional arguments are supported:
>>>  .BI "dscp inherit"
>>>  ] [
>>>  .BI dev " PHYS_DEV "
>>> +] [
>>> +.RB external
>>>  ]
>>>
>>>  .in +8
>>> @@ -833,6 +835,10 @@ or
>>>  .IR 00 ".." ff
>>>  when tunneling non-IP packets. The default value is 00.
>>>
>>> +.sp
>>> +.RB external
>>> +- make this tunnel externally controlled (or not, which is the default).
>>> +
>>>  .in -8
>>
>> I don't have any  direct involvement in offload, so would like some feedback
>> from others that are.
>>
>> Not a big fan of opaque "metadata" what exactly does it mean?
>> Also "external" is already used to mean something else on other parts of
>> the link command. Also the option, and the value in JSON should be the same.
>
> The keyword "external" to set the device into collect metadata mode
> is already used heavily throughout iproute2, e.g. vxlan, geneve, ipip,
> and many other device types. Special casing gre6 to not having it set
> up in such way through "external" would seem quite confusing.
>
>> Please reconsider the naming and resubmit
>>
>> The wording in the man page here could be better
>>
>
Hi Stephen and Daniel,
Thanks for the feedbacks.

For the next version, I will remain using the keyword "external" since
it consists with other tunnels.
As for JSON, since geneve and vxlan is useing "collect_metadata" for
external keyword, should I keep using "collect_metadata" for ip6gre?
root@:~/iproute2# git grep collect_metadata
ip/iplink_geneve.c:             print_bool(PRINT_ANY,
"collect_metadata", "external ", true);
ip/iplink_vxlan.c:              print_bool(PRINT_ANY,
"collect_metadata", "external ", true);

Or I can submit another patch to make them all use "external".
Thanks
William