netdev - Re: [PATCH iproute2-next v2] ip: add AMT 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: <57982aae-d473-b2c5-22a4-b3b530202c68@gmail.com>
Date:   Fri, 29 Oct 2021 01:23:51 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     David Ahern <dsahern@...il.com>, stephen@...workplumber.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH iproute2-next v2] ip: add AMT support

Hi David,
Thank you for your review!

On 10/29/21 12:18 AM, David Ahern wrote:
 > On 10/25/21 11:28 PM, Taehee Yoo wrote:
 >> +static void amt_print_opt(struct link_util *lu, FILE *f, struct 
rtattr *tb[])
 >> +{
 >> +	if (!tb)
 >> +		return;
 >> +
 >> +	if (tb[IFLA_AMT_MODE]) {
 >> +		print_string(PRINT_ANY,
 >> +			     "mode",
 >> +			     "%s ",
 >> +			     modename[rta_getattr_u32(tb[IFLA_AMT_MODE])]);
 >> +	}
 >> +
 >> +	if (tb[IFLA_AMT_GATEWAY_PORT]) {
 >> +		print_uint(PRINT_ANY,
 >> +			   "gateway_port",
 >> +			   "gateway_port %u ",
 >> +			   rta_getattr_be16(tb[IFLA_AMT_GATEWAY_PORT]));
 >> +	}
 >> +
 >> +	if (tb[IFLA_AMT_RELAY_PORT]) {
 >> +		print_uint(PRINT_ANY,
 >> +			   "relay_port",
 >> +			   "relay_port %u ",
 >> +			   rta_getattr_be16(tb[IFLA_AMT_RELAY_PORT]));
 >> +	}
 >> +
 >> +	if (tb[IFLA_AMT_LOCAL_IP]) {
 >> +		__be32 addr = rta_getattr_u32(tb[IFLA_AMT_LOCAL_IP]);
 >> +
 >> +		if (addr)
 >> +			print_string(PRINT_ANY,
 >> +				     "local",
 >> +				     "local %s ",
 >> +				     format_host(AF_INET, 4, &addr));
 >
 > if you only print the address when it is non-zero, why send it at all?
 > ie., kernel side can default to 0 (any address) but that value should
 > not be allowed or sent as part of the API. Same with the remote and
 > discovery addresses.
 >

Thanks, I will remove this check and I will add ipv4_is_zeronet() to the 
kernel.

 >> +	}
 >> +
 >> +	if (tb[IFLA_AMT_REMOTE_IP]) {
 >> +		__be32 addr = rta_getattr_u32(tb[IFLA_AMT_REMOTE_IP]);
 >> +
 >> +		if (addr)
 >> +			print_string(PRINT_ANY,
 >> +				     "remote",
 >> +				     "remote %s ",
 >> +				     format_host(AF_INET, 4, &addr));
 >> +	}
 >> +
 >> +	if (tb[IFLA_AMT_DISCOVERY_IP]) {
 >> +		__be32 addr = rta_getattr_u32(tb[IFLA_AMT_DISCOVERY_IP]);
 >> +
 >> +		if (addr) {
 >> +			print_string(PRINT_ANY,
 >> +				     "discovery",
 >> +				     "discovery %s ",
 >> +				     format_host(AF_INET, 4, &addr));
 >> +		}
 >> +	}
 >> +
 >> +	if (tb[IFLA_AMT_LINK]) {
 >> +		unsigned int link = rta_getattr_u32(tb[IFLA_AMT_LINK]);
 >> +
 >> +		if (link)
 >> +			print_string(PRINT_ANY, "link", "dev %s ",
 >> +				     ll_index_to_name(link));
 >
 > similar argument here: IFLA_AMT_LINK should only be sent if an actual
 > link association is configured.
 >

Thanks, I will remove it too.

 >> +	}
 >> +
 >> +	if (tb[IFLA_AMT_MAX_TUNNELS]) {
 >> +		unsigned int tunnels = rta_getattr_u32(tb[IFLA_AMT_MAX_TUNNELS]);
 >> +
 >> +		if (tunnels)
 >> +			print_uint(PRINT_ANY, "max_tunnels", "max_tunnels %u ",
 >> +				   rta_getattr_u32(tb[IFLA_AMT_MAX_TUNNELS]));
 >
 > and here.
 >

here too.

 >> +	}
 >> +}
 >> +
 >
 >

Thanks a lot for your detailed review!
I will send the v3 patch soon.

Thanks,
Taehee