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: <F074B3B5-1B07-490F-87B8-887E2EFB32F3@cumulusnetworks.com>
Date:   Fri, 24 Jul 2020 19:24:35 +0300
From:   nikolay@...ulusnetworks.com
To:     Stephen Hemminger <stephen@...workplumber.org>,
        George Shuklin <amarao@...vers.com>
CC:     netdev@...r.kernel.org, jiri@...nulli.us, amarao@...vers.com
Subject: Re: [RFT iproute2] iplink_bridge: scale all time values by USER_HZ

On 24 July 2020 19:15:17 EEST, Stephen Hemminger <stephen@...workplumber.org> wrote:
>
>The bridge portion of ip command was not scaling so the
>values were off.
>
>The netlink API's for setting and reading timers all conform
>to the kernel standard of scaling the values by USER_HZ (100).
>
>Fixes: 28d84b429e4e ("add bridge master device support")
>Fixes: 7f3d55922645 ("iplink: bridge: add support for
>IFLA_BR_MCAST_MEMBERSHIP_INTVL")
>Fixes: 10082a253fb2 ("iplink: bridge: add support for
>IFLA_BR_MCAST_LAST_MEMBER_INTVL")
>Fixes: 1f2244b851dd ("iplink: bridge: add support for
>IFLA_BR_MCAST_QUERIER_INTVL")
>Signed-off-by: Stephen Hemminger <stephen@...workplumber.org>
>---

While I agree this should have been done from the start, it's too late to change. 
We'll break everyone using these commands. 
We have been discussing to add _ms version of all these which do the proper scaling. I'd prefer that, it's least disruptive
to users. 

Every user of the old commands scales the values by now. 


>
>Compile tested only.
>
>
> ip/iplink_bridge.c | 45 ++++++++++++++++++++++++++-------------------
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
>diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c
>index 3e81aa059cb3..48495a08c484 100644
>--- a/ip/iplink_bridge.c
>+++ b/ip/iplink_bridge.c
>@@ -24,6 +24,7 @@
> 
> static unsigned int xstats_print_attr;
> static int filter_index;
>+static unsigned int hz;
> 
> static void print_explain(FILE *f)
> {
>@@ -85,19 +86,22 @@ static int bridge_parse_opt(struct link_util *lu,
>int argc, char **argv,
> {
> 	__u32 val;
> 
>+	if (!hz)
>+		hz = get_user_hz();
>+
> 	while (argc > 0) {
> 		if (matches(*argv, "forward_delay") == 0) {
> 			NEXT_ARG();
> 			if (get_u32(&val, *argv, 0))
> 				invarg("invalid forward_delay", *argv);
> 
>-			addattr32(n, 1024, IFLA_BR_FORWARD_DELAY, val);
>+			addattr32(n, 1024, IFLA_BR_FORWARD_DELAY, val * hz);
> 		} else if (matches(*argv, "hello_time") == 0) {
> 			NEXT_ARG();
> 			if (get_u32(&val, *argv, 0))
> 				invarg("invalid hello_time", *argv);
> 
>-			addattr32(n, 1024, IFLA_BR_HELLO_TIME, val);
>+			addattr32(n, 1024, IFLA_BR_HELLO_TIME, val * hz);
> 		} else if (matches(*argv, "max_age") == 0) {
> 			NEXT_ARG();
> 			if (get_u32(&val, *argv, 0))
>@@ -109,7 +113,7 @@ static int bridge_parse_opt(struct link_util *lu,
>int argc, char **argv,
> 			if (get_u32(&val, *argv, 0))
> 				invarg("invalid ageing_time", *argv);
> 
>-			addattr32(n, 1024, IFLA_BR_AGEING_TIME, val);
>+			addattr32(n, 1024, IFLA_BR_AGEING_TIME, val * hz);
> 		} else if (matches(*argv, "stp_state") == 0) {
> 			NEXT_ARG();
> 			if (get_u32(&val, *argv, 0))
>@@ -266,7 +270,7 @@ static int bridge_parse_opt(struct link_util *lu,
>int argc, char **argv,
> 				       *argv);
> 
> 			addattr64(n, 1024, IFLA_BR_MCAST_LAST_MEMBER_INTVL,
>-				  mcast_last_member_intvl);
>+				  mcast_last_member_intvl * hz);
> 		} else if (matches(*argv, "mcast_membership_interval") == 0) {
> 			__u64 mcast_membership_intvl;
> 
>@@ -276,7 +280,7 @@ static int bridge_parse_opt(struct link_util *lu,
>int argc, char **argv,
> 				       *argv);
> 
> 			addattr64(n, 1024, IFLA_BR_MCAST_MEMBERSHIP_INTVL,
>-				  mcast_membership_intvl);
>+				  mcast_membership_intvl * hz);
> 		} else if (matches(*argv, "mcast_querier_interval") == 0) {
> 			__u64 mcast_querier_intvl;
> 
>@@ -286,7 +290,7 @@ static int bridge_parse_opt(struct link_util *lu,
>int argc, char **argv,
> 				       *argv);
> 
> 			addattr64(n, 1024, IFLA_BR_MCAST_QUERIER_INTVL,
>-				  mcast_querier_intvl);
>+				  mcast_querier_intvl * hz);
> 		} else if (matches(*argv, "mcast_query_interval") == 0) {
> 			__u64 mcast_query_intvl;
> 
>@@ -296,7 +300,7 @@ static int bridge_parse_opt(struct link_util *lu,
>int argc, char **argv,
> 				       *argv);
> 
> 			addattr64(n, 1024, IFLA_BR_MCAST_QUERY_INTVL,
>-				  mcast_query_intvl);
>+				  mcast_query_intvl * hz);
> 		} else if (!matches(*argv, "mcast_query_response_interval")) {
> 			__u64 mcast_query_resp_intvl;
> 
>@@ -306,7 +310,7 @@ static int bridge_parse_opt(struct link_util *lu,
>int argc, char **argv,
> 				       *argv);
> 
> 			addattr64(n, 1024, IFLA_BR_MCAST_QUERY_RESPONSE_INTVL,
>-				  mcast_query_resp_intvl);
>+				  mcast_query_resp_intvl * hz);
> 		} else if (!matches(*argv, "mcast_startup_query_interval")) {
> 			__u64 mcast_startup_query_intvl;
> 
>@@ -316,7 +320,7 @@ static int bridge_parse_opt(struct link_util *lu,
>int argc, char **argv,
> 				       *argv);
> 
> 			addattr64(n, 1024, IFLA_BR_MCAST_STARTUP_QUERY_INTVL,
>-				  mcast_startup_query_intvl);
>+				  mcast_startup_query_intvl * hz);
> 		} else if (matches(*argv, "mcast_stats_enabled") == 0) {
> 			__u8 mcast_stats_enabled;
> 
>@@ -407,29 +411,32 @@ static void bridge_print_opt(struct link_util
>*lu, FILE *f, struct rtattr *tb[])
> 	if (!tb)
> 		return;
> 
>+	if (!hz)
>+		hz = get_user_hz();
>+
> 	if (tb[IFLA_BR_FORWARD_DELAY])
> 		print_uint(PRINT_ANY,
> 			   "forward_delay",
> 			   "forward_delay %u ",
>-			   rta_getattr_u32(tb[IFLA_BR_FORWARD_DELAY]));
>+			   rta_getattr_u32(tb[IFLA_BR_FORWARD_DELAY]) / hz);
> 
> 	if (tb[IFLA_BR_HELLO_TIME])
> 		print_uint(PRINT_ANY,
> 			   "hello_time",
> 			   "hello_time %u ",
>-			   rta_getattr_u32(tb[IFLA_BR_HELLO_TIME]));
>+			   rta_getattr_u32(tb[IFLA_BR_HELLO_TIME]) / hz);
> 
> 	if (tb[IFLA_BR_MAX_AGE])
> 		print_uint(PRINT_ANY,
> 			   "max_age",
> 			   "max_age %u ",
>-			   rta_getattr_u32(tb[IFLA_BR_MAX_AGE]));
>+			   rta_getattr_u32(tb[IFLA_BR_MAX_AGE]) / hz);
> 
> 	if (tb[IFLA_BR_AGEING_TIME])
> 		print_uint(PRINT_ANY,
> 			   "ageing_time",
> 			   "ageing_time %u ",
>-			   rta_getattr_u32(tb[IFLA_BR_AGEING_TIME]));
>+			   rta_getattr_u32(tb[IFLA_BR_AGEING_TIME]) / hz);
> 
> 	if (tb[IFLA_BR_STP_STATE])
> 		print_uint(PRINT_ANY,
>@@ -605,37 +612,37 @@ static void bridge_print_opt(struct link_util
>*lu, FILE *f, struct rtattr *tb[])
> 		print_lluint(PRINT_ANY,
> 			     "mcast_last_member_intvl",
> 			     "mcast_last_member_interval %llu ",
>-			     rta_getattr_u64(tb[IFLA_BR_MCAST_LAST_MEMBER_INTVL]));
>+			     rta_getattr_u64(tb[IFLA_BR_MCAST_LAST_MEMBER_INTVL]) / hz);
> 
> 	if (tb[IFLA_BR_MCAST_MEMBERSHIP_INTVL])
> 		print_lluint(PRINT_ANY,
> 			     "mcast_membership_intvl",
> 			     "mcast_membership_interval %llu ",
>-			     rta_getattr_u64(tb[IFLA_BR_MCAST_MEMBERSHIP_INTVL]));
>+			     rta_getattr_u64(tb[IFLA_BR_MCAST_MEMBERSHIP_INTVL]) / hz);
> 
> 	if (tb[IFLA_BR_MCAST_QUERIER_INTVL])
> 		print_lluint(PRINT_ANY,
> 			     "mcast_querier_intvl",
> 			     "mcast_querier_interval %llu ",
>-			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERIER_INTVL]));
>+			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERIER_INTVL]) / hz);
> 
> 	if (tb[IFLA_BR_MCAST_QUERY_INTVL])
> 		print_lluint(PRINT_ANY,
> 			     "mcast_query_intvl",
> 			     "mcast_query_interval %llu ",
>-			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERY_INTVL]));
>+			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERY_INTVL]) / hz);
> 
> 	if (tb[IFLA_BR_MCAST_QUERY_RESPONSE_INTVL])
> 		print_lluint(PRINT_ANY,
> 			     "mcast_query_response_intvl",
> 			     "mcast_query_response_interval %llu ",
>-			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERY_RESPONSE_INTVL]));
>+			     rta_getattr_u64(tb[IFLA_BR_MCAST_QUERY_RESPONSE_INTVL]) / hz);
> 
> 	if (tb[IFLA_BR_MCAST_STARTUP_QUERY_INTVL])
> 		print_lluint(PRINT_ANY,
> 			     "mcast_startup_query_intvl",
> 			     "mcast_startup_query_interval %llu ",
>-			     rta_getattr_u64(tb[IFLA_BR_MCAST_STARTUP_QUERY_INTVL]));
>+			     rta_getattr_u64(tb[IFLA_BR_MCAST_STARTUP_QUERY_INTVL]) / hz);
> 
> 	if (tb[IFLA_BR_MCAST_STATS_ENABLED])
> 		print_uint(PRINT_ANY,


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ