netdev - Re: [PATCH iproute2 net-next] iplink: add support for protodown reason

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: <f9a02516-f954-2b97-ed12-8fbad2f2271a@gmail.com>
Date:   Sun, 23 Aug 2020 20:20:31 -0600
From:   David Ahern <dsahern@...il.com>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>, dsahern@...il.com
Cc:     netdev@...r.kernel.org, stephen@...workplumber.org
Subject: Re: [PATCH iproute2 net-next] iplink: add support for protodown
 reason

On 8/20/20 9:52 PM, Roopa Prabhu wrote:
> +void protodown_reason_n2a(int id, char *buf, int len)
> +{
> +	if (id < 0 || id >= PROTODOWN_REASON_NUM_BITS || numeric) {

since the reason is limited to 0-31, id > PROTODOWN_REASON_NUM_BITS
should be an error.

> +		snprintf(buf, len, "%d", id);
> +		return;
> +	}
> +
> +	if (!protodown_reason_init)
> +		protodown_reason_initialize();
> +
> +	if (protodown_reason_tab[id])
> +		snprintf(buf, len, "%s", protodown_reason_tab[id]);
> +	else
> +		snprintf(buf, len, "%d", id);
> +}
> +
> +int protodown_reason_a2n(__u32 *id, const char *arg)
> +{
> +	static char *cache;
> +	static unsigned long res;
> +	char *end;
> +	int i;
> +
> +	if (cache && strcmp(cache, arg) == 0) {
> +		*id = res;
> +		return 0;
> +	}
> +
> +	if (!protodown_reason_init)
> +		protodown_reason_initialize();
> +
> +	for (i = 0; i < PROTODOWN_REASON_NUM_BITS; i++) {
> +		if (protodown_reason_tab[i] &&
> +		    strcmp(protodown_reason_tab[i], arg) == 0) {
> +			cache = protodown_reason_tab[i];
> +			res = i;
> +			*id = res;
> +			return 0;
> +		}
> +	}
> +
> +	res = strtoul(arg, &end, 0);
> +	if (!end || end == arg || *end || res > 255)

same here: res >= PROTODOWN_REASON_NUM_BITS is a failure.

> +		return -1;
> +	*id = res;
> +	return 0;
> +}
> diff --git a/man/man8/ip-link.8.in b/man/man8/ip-link.8.in
> index c6bd2c53..df3dd531 100644
> --- a/man/man8/ip-link.8.in
> +++ b/man/man8/ip-link.8.in
> @@ -75,6 +75,9 @@ ip-link \- network device configuration
>  .br
>  .RB "[ " protodown " { " on " | " off " } ]"
>  .br
> +.RB "[ " protodown_reason
> +.IR PREASON " { " on " | " off " } ]"
> +.br
>  .RB "[ " trailers " { " on " | " off " } ]"
>  .br
>  .RB "[ " txqueuelen
> @@ -1917,6 +1920,13 @@ state on the device. Indicates that a protocol error has been detected
>  on the port. Switch drivers can react to this error by doing a phys
>  down on the switch port.
>  
> +.TP
> +.BR "protodown_reason PREASON on " or " off"
> +set
> +.B PROTODOWN
> +reasons on the device. protodown reason bit names can be enumerated under
> +/etc/iproute2/protodown_reasons.d/.

we should document that 0..31 limit.

> +
>  .TP
>  .BR "dynamic on " or " dynamic off"
>  change the
> 

I wonder how well supported __builtin_ffsl is across architectures ...
would be faster than the 'for (i = 0; reason; i++, reason >>= 1) {' checks.