netdev - Re: [PATCH iproute2-next v5] ip-link: add support for nolocalbypass in vxlan

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: <ZGsKOHxuFMSomUUI@shredder>
Date: Mon, 22 May 2023 09:22:48 +0300
From: Ido Schimmel <idosch@...sch.org>
To: Vladimir Nikishkin <vladimir@...ishkin.pw>
Cc: stephen@...workplumber.org, dsahern@...il.com, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, eng.alaamohamedsoliman.am@...il.com,
	gnault@...hat.com, razor@...ckwall.org, idosch@...dia.com,
	liuhangbin@...il.com, eyal.birger@...il.com, jtoppins@...hat.com
Subject: Re: [PATCH iproute2-next v5] ip-link: add support for nolocalbypass
 in vxlan

On Mon, May 22, 2023 at 02:03:37PM +0800, Vladimir Nikishkin wrote:
> 
> Ido Schimmel <idosch@...sch.org> writes:
> 
> > On Sun, May 21, 2023 at 01:49:48PM +0800, Vladimir Nikishkin wrote:
> >> +	if (tb[IFLA_VXLAN_LOCALBYPASS] &&
> >> +	   rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS])) {
> >            ^ Unaligned 
> >
> >> +		print_bool(PRINT_ANY, "localbypass", "localbypass", true);
> >
> > Missing space after "localbypass":
> >
> > # ip -d link show dev vxlan0
> > 10: vxlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
> >     link/ether ce:10:63:1d:f3:a8 brd ff:ff:ff:ff:ff:ff promiscuity 0  allmulti 0 minmtu 68 maxmtu 65535 
> >     vxlan id 10 srcport 0 0 dstport 4789 ttl auto ageing 300 udpcsum localbypassnoudp6zerocsumtx [...]
> >
> > Should be:
> >
> > print_bool(PRINT_ANY, "localbypass", "localbypass ", true);
> >
> >> +	}
> >
> > Parenthesis are unnecessary in this case.
> >
> > I disagree with Stephen's comment about "Use presence as a boolean in
> > JSON". I don't like the fact that we don't have JSON output when
> > "false":
> >
> >  # ip -d -j -p link show dev vxlan0 | jq '.[]["linkinfo"]["info_data"]["localbypass"]'
> >  true
> >  # ip link set dev vxlan0 type vxlan nolocalbypass
> >  # ip -d -j -p link show dev vxlan0 | jq '.[]["linkinfo"]["info_data"]["localbypass"]'
> >  null
> >
> > It's inconsistent with other iproute2 utilities such as "bridge" and
> > looks very much like an oversight in the initial JSON output
> > implementation.
> >
> > IOW, I don't have a problem with the code in v4 or simply:
> >
> > @@ -613,6 +622,10 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
> >                 }
> >         }
> >  
> > +       if (tb[IFLA_VXLAN_LOCALBYPASS])
> > +               print_bool(PRINT_ANY, "localbypass", "localbypass ",
> > +                          rta_getattr_u8(tb[IFLA_VXLAN_LOCALBYPASS]));
> > +
> >         if (tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]) {
> >                 __u8 csum6 = rta_getattr_u8(tb[IFLA_VXLAN_UDP_ZERO_CSUM6_TX]);
> 
> If the two maintainers disagree, I am even more confused.

Stephen and David are the maintainers.

> 
> Shall I restore version 4? Use print_bool unconditionally, and add the
> json context check back into the code?

Please wait for their reply before sending another version. Ultimately
it's their call. It's better to decide now how we want to handle boolean
VXLAN options than repeating this discussion the next time a new option
is added.