From f8176999390f6d9cfbdb838871a318ff91c77702 Mon Sep 17 00:00:00 2001 From: Stephen Hemminger Date: Sun, 29 Nov 2020 09:31:20 -0800 Subject: [PATCH 1/9] devlink: fix uninitialized warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GCC-10 complains about uninitialized variable. devlink.c: In function ‘cmd_dev’: devlink.c:2803:12: warning: ‘val_u32’ may be used uninitialized in this function [-Wmaybe-uninitialized] 2803 | val_u16 = val_u32; | ~~~~~~~~^~~~~~~~~ devlink.c:2747:11: note: ‘val_u32’ was declared here 2747 | uint32_t val_u32; | ^~~~~~~ This is a false positive because it can't figure out the control flow when the parse returns error. Fixes: 2557dca2b028 ("devlink: Add string to uint{8,16,32} conversion for generic parameters") Cc: shalomt@mellanox.com Signed-off-by: Stephen Hemminger --- devlink/devlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/devlink/devlink.c b/devlink/devlink.c index 1ff865bc..ca99732e 100644 --- a/devlink/devlink.c +++ b/devlink/devlink.c @@ -2744,7 +2744,7 @@ static int cmd_dev_param_set(struct dl *dl) struct param_ctx ctx = {}; struct nlmsghdr *nlh; bool conv_exists; - uint32_t val_u32; + uint32_t val_u32 = 0; uint16_t val_u16; uint8_t val_u8; bool val_bool; From 5bdc4e9151a19f0bc8c851cfcca75363abb19fde Mon Sep 17 00:00:00 2001 From: Stephen Hemminger Date: Sun, 29 Nov 2020 09:32:50 -0800 Subject: [PATCH 2/9] bridge: fix string length warning Gcc-10 complains about possible string length overflow. This can't happen Ethernet address format is always limited to 18 characters or less. Just resize the temp buffer. Fixes: 70dfb0b8836d ("iplink: bridge: export bridge_id and designated_root") Cc: nikolay@cumulusnetworks.com Signed-off-by: Stephen Hemminger --- ip/iplink_bridge.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c index 3e81aa05..d12fd055 100644 --- a/ip/iplink_bridge.c +++ b/ip/iplink_bridge.c @@ -74,7 +74,7 @@ static void explain(void) void br_dump_bridge_id(const struct ifla_bridge_id *id, char *buf, size_t len) { - char eaddr[32]; + char eaddr[18]; ether_ntoa_r((const struct ether_addr *)id->addr, eaddr); snprintf(buf, len, "%.2x%.2x.%s", id->prio[0], id->prio[1], eaddr); From 2319db905295fa47c651b52ae09a8d7bf305c6f7 Mon Sep 17 00:00:00 2001 From: Stephen Hemminger Date: Sun, 29 Nov 2020 09:40:49 -0800 Subject: [PATCH 3/9] tc: fix compiler warnings in ip6 pedit Gcc-10 complains about referencing a zero size array. This occurs because the array of keys is actually in the following structure which is part of the overall selector. The original code was safe, but better to just use the key array directly. Fixes: 2d9a8dc439ee ("tc: p_ip6: Support pedit of IPv6 dsfield") Cc: petrm@mellanox.com Signed-off-by: Stephen Hemminger --- tc/p_ip6.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tc/p_ip6.c b/tc/p_ip6.c index 71660c61..83a6ae81 100644 --- a/tc/p_ip6.c +++ b/tc/p_ip6.c @@ -82,7 +82,7 @@ parse_ip6(int *argc_p, char ***argv_p, /* Shift the field by 4 bits on success. */ if (!res) { int nkeys = sel->sel.nkeys; - struct tc_pedit_key *key = &sel->sel.keys[nkeys - 1]; + struct tc_pedit_key *key = &sel->keys[nkeys - 1]; key->mask = htonl(ntohl(key->mask) << 4 | 0xf); key->val = htonl(ntohl(key->val) << 4); From c014983921389dc7880dfe368eb43cb2570f6a4b Mon Sep 17 00:00:00 2001 From: Stephen Hemminger Date: Sun, 29 Nov 2020 09:43:48 -0800 Subject: [PATCH 4/9] misc: fix compiler warning in ifstat and nstat The code here was doing strncpy() in a way that causes gcc 10 warning about possible string overflow. Just use strlcpy() which will null terminate and bound the string as expected. This has existed since start of git era so no Fixes tag. Signed-off-by: Stephen Hemminger --- misc/ifstat.c | 2 +- misc/nstat.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/misc/ifstat.c b/misc/ifstat.c index c05183d7..d4a33429 100644 --- a/misc/ifstat.c +++ b/misc/ifstat.c @@ -251,7 +251,7 @@ static void load_raw_table(FILE *fp) buf[strlen(buf)-1] = 0; if (info_source[0] && strcmp(info_source, buf+1)) source_mismatch = 1; - strncpy(info_source, buf+1, sizeof(info_source)-1); + strlcpy(info_source, buf+1, sizeof(info_source)); continue; } if ((n = malloc(sizeof(*n))) == NULL) diff --git a/misc/nstat.c b/misc/nstat.c index 6fdd316c..ecdd4ce8 100644 --- a/misc/nstat.c +++ b/misc/nstat.c @@ -136,8 +136,7 @@ static void load_good_table(FILE *fp) buf[strlen(buf)-1] = 0; if (info_source[0] && strcmp(info_source, buf+1)) source_mismatch = 1; - info_source[0] = 0; - strncat(info_source, buf+1, sizeof(info_source)-1); + strlcpy(info_source, buf + 1, sizeof(info_source)); continue; } /* idbuf is as big as buf, so this is safe */ From cae2e9291adf298041418bf9fe5f149c612db105 Mon Sep 17 00:00:00 2001 From: Stephen Hemminger Date: Sun, 29 Nov 2020 09:47:44 -0800 Subject: [PATCH 5/9] f_u32: fix compiler gcc-10 compiler warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With gcc-10 it complains about array subscript error. f_u32.c: In function ‘u32_parse_opt’: f_u32.c:1113:24: warning: array subscript 0 is outside the bounds of an interior zero-length array ‘struct tc_u32_key[0]’ [-Wzero-length-bounds] 1113 | hash = sel2.sel.keys[0].val & sel2.sel.keys[0].mask; | ~~~~~~~~~~~~~^~~ In file included from tc_util.h:11, from f_u32.c:26: ../include/uapi/linux/pkt_cls.h:253:20: note: while referencing ‘keys’ 253 | struct tc_u32_key keys[0]; | This is because the keys are actually allocated in the second element of the parent structure. Simplest way to address the warning is to assign directly to the keys in the containing structure. This has always been in iproute2 (pre-git) so no Fixes. Signed-off-by: Stephen Hemminger --- tc/f_u32.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tc/f_u32.c b/tc/f_u32.c index e0a322d5..2ed5254a 100644 --- a/tc/f_u32.c +++ b/tc/f_u32.c @@ -1110,7 +1110,7 @@ static int u32_parse_opt(struct filter_util *qu, char *handle, } NEXT_ARG(); } - hash = sel2.sel.keys[0].val & sel2.sel.keys[0].mask; + hash = sel2.keys[0].val & sel2.keys[0].mask; hash ^= hash >> 16; hash ^= hash >> 8; htid = ((hash % divisor) << 12) | (htid & 0xFFF00000); From c95d63e4fb0c7d7f23bae96488d76b9cf9cd9aee Mon Sep 17 00:00:00 2001 From: Stephen Hemminger Date: Sun, 29 Nov 2020 21:16:50 -0800 Subject: [PATCH 6/9] uapi: update devlink.h Signed-off-by: Stephen Hemminger --- include/uapi/linux/devlink.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index e639a4e5..1414acee 100644 --- a/include/uapi/linux/devlink.h +++ b/include/uapi/linux/devlink.h @@ -526,6 +526,8 @@ enum devlink_attr { DEVLINK_ATTR_RELOAD_STATS_LIMIT, /* u8 */ DEVLINK_ATTR_RELOAD_STATS_VALUE, /* u32 */ DEVLINK_ATTR_REMOTE_RELOAD_STATS, /* nested */ + DEVLINK_ATTR_RELOAD_ACTION_INFO, /* nested */ + DEVLINK_ATTR_RELOAD_ACTION_STATS, /* nested */ /* add new attributes above here, update the policy in devlink.c */ From fb054cb3362158d25088cd4eb26fa713684d6428 Mon Sep 17 00:00:00 2001 From: Stephen Hemminger Date: Sun, 29 Nov 2020 21:16:50 -0800 Subject: [PATCH 7/9] uapi: update devlink.h Signed-off-by: Stephen Hemminger --- include/uapi/linux/devlink.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h index e639a4e5..1414acee 100644 --- a/include/uapi/linux/devlink.h +++ b/include/uapi/linux/devlink.h @@ -526,6 +526,8 @@ enum devlink_attr { DEVLINK_ATTR_RELOAD_STATS_LIMIT, /* u8 */ DEVLINK_ATTR_RELOAD_STATS_VALUE, /* u32 */ DEVLINK_ATTR_REMOTE_RELOAD_STATS, /* nested */ + DEVLINK_ATTR_RELOAD_ACTION_INFO, /* nested */ + DEVLINK_ATTR_RELOAD_ACTION_STATS, /* nested */ /* add new attributes above here, update the policy in devlink.c */ From 975c4944e8d57b9f51960611e2bc2c0da6cd6864 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Fri, 27 Nov 2020 18:06:51 +0000 Subject: [PATCH 8/9] ip/netns: use flock when setting up /run/netns If multiple ip processes are ran at the same time to set up separate network namespaces, and it is the first time so /run/netns has to be set up first, and they end up doing it at the same time, the processes might enter a recursive loop creating thousands of mount points, which might crash the system depending on resources available. Try to take a flock on /run/netns before doing the mount() dance, to ensure this cannot happen. But do not try too hard, and if it fails continue after printing a warning, to avoid introducing regressions. First reported on Debian: https://bugs.debian.org/949235 To reproduce (WARNING: run in a VM to avoid system lockups): for i in {0..9} do strace -e trace=mount -e inject=mount:delay_exit=1000000 ip \ netns add "testnetns$i" 2>&1 | tee "$i.log" & done wait The strace is to ensure the problem always reproduces, to add an artificial synchronization point after the first mount(). Reported-by: Etienne Dechamps Signed-off-by: Luca Boccassi Signed-off-by: Stephen Hemminger --- ip/ipnetns.c | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/ip/ipnetns.c b/ip/ipnetns.c index 14e8e087..3e96d267 100644 --- a/ip/ipnetns.c +++ b/ip/ipnetns.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0 */ #define _ATFILE_SOURCE +#include #include #include #include @@ -801,6 +802,7 @@ static int netns_add(int argc, char **argv, bool create) const char *name; pid_t pid; int fd; + int lock; int made_netns_run_dir_mount = 0; if (create) { @@ -831,12 +833,37 @@ static int netns_add(int argc, char **argv, bool create) * namespace file in one namespace will unmount the network namespace * file in all namespaces allowing the network namespace to be freed * sooner. + * These setup steps need to happen only once, as if multiple ip processes + * try to attempt the same operation at the same time, the mountpoints will + * be recursively created multiple times, eventually causing the system + * to lock up. For example, this has been observed when multiple netns + * namespaces are created in parallel at boot. See: + * https://bugs.debian.org/949235 + * Try to take an exclusive file lock on the top level directory to ensure + * this cannot happen, but proceed nonetheless if it cannot happen for any + * reason. */ + lock = open(NETNS_RUN_DIR, O_RDONLY|O_DIRECTORY, 0); + if (lock < 0) { + fprintf(stderr, "Cannot open netns runtime directory \"%s\": %s\n", + NETNS_RUN_DIR, strerror(errno)); + return -1; + } + if (flock(lock, LOCK_EX) < 0) { + fprintf(stderr, "Warning: could not flock netns runtime directory \"%s\": %s\n", + NETNS_RUN_DIR, strerror(errno)); + close(lock); + lock = -1; + } while (mount("", NETNS_RUN_DIR, "none", MS_SHARED | MS_REC, NULL)) { /* Fail unless we need to make the mount point */ if (errno != EINVAL || made_netns_run_dir_mount) { fprintf(stderr, "mount --make-shared %s failed: %s\n", NETNS_RUN_DIR, strerror(errno)); + if (lock != -1) { + flock(lock, LOCK_UN); + close(lock); + } return -1; } @@ -844,10 +871,18 @@ static int netns_add(int argc, char **argv, bool create) if (mount(NETNS_RUN_DIR, NETNS_RUN_DIR, "none", MS_BIND | MS_REC, NULL)) { fprintf(stderr, "mount --bind %s %s failed: %s\n", NETNS_RUN_DIR, NETNS_RUN_DIR, strerror(errno)); + if (lock != -1) { + flock(lock, LOCK_UN); + close(lock); + } return -1; } made_netns_run_dir_mount = 1; } + if (lock != -1) { + flock(lock, LOCK_UN); + close(lock); + } /* Create the filesystem state */ fd = open(netns_path, O_RDONLY|O_CREAT|O_EXCL, 0); From 755b1c584eeed60767f79fafb935f6ec1f8a4b75 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 2 Dec 2020 17:08:45 +0000 Subject: [PATCH 9/9] tc/mqprio: json-ify output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As reported by a Debian user, mqprio output in json mode is invalid: { "kind": "mqprio", "handle": "8021:", "dev": "enp1s0f0", "root": true, "options": { tc 2 map 0 0 0 1 0 1 0 0 0 0 0 0 0 0 0 0 queues:(0:3) (4:7) mode:channel shaper:dcb} } json-ify it, while trying to maintain the same formatting for standard output. New output: { "kind": "mqprio", "handle": "8001:", "root": true, "options": { "tc": 2, "map": [ 0, 0, 0, 1, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 ], "queues": [ [ 0, 3 ], [ 4, 7 ] ], "mode": "channel", "shaper": "dcb" } } https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=972784 Reported-by: Roméo GINON Signed-off-by: Luca Boccassi Signed-off-by: Stephen Hemminger --- tc/q_mqprio.c | 36 ++++++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 14 deletions(-) diff --git a/tc/q_mqprio.c b/tc/q_mqprio.c index f26ba8d7..5499f621 100644 --- a/tc/q_mqprio.c +++ b/tc/q_mqprio.c @@ -243,13 +243,19 @@ static int mqprio_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt) qopt = RTA_DATA(opt); - fprintf(f, " tc %u map ", qopt->num_tc); + print_uint(PRINT_ANY, "tc", "tc %u ", qopt->num_tc); + open_json_array(PRINT_ANY, is_json_context() ? "map" : "map "); for (i = 0; i <= TC_PRIO_MAX; i++) - fprintf(f, "%u ", qopt->prio_tc_map[i]); - fprintf(f, "\n queues:"); - for (i = 0; i < qopt->num_tc; i++) - fprintf(f, "(%u:%u) ", qopt->offset[i], - qopt->offset[i] + qopt->count[i] - 1); + print_uint(PRINT_ANY, NULL, "%u ", qopt->prio_tc_map[i]); + close_json_array(PRINT_ANY, ""); + open_json_array(PRINT_ANY, is_json_context() ? "queues" : "\n queues:"); + for (i = 0; i < qopt->num_tc; i++) { + open_json_array(PRINT_JSON, NULL); + print_uint(PRINT_ANY, NULL, "(%u:", qopt->offset[i]); + print_uint(PRINT_ANY, NULL, "%u) ", qopt->offset[i] + qopt->count[i] - 1); + close_json_array(PRINT_JSON, NULL); + } + close_json_array(PRINT_ANY, ""); if (len > 0) { struct rtattr *tb[TCA_MQPRIO_MAX + 1]; @@ -262,18 +268,18 @@ static int mqprio_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt) __u16 *mode = RTA_DATA(tb[TCA_MQPRIO_MODE]); if (*mode == TC_MQPRIO_MODE_CHANNEL) - fprintf(f, "\n mode:channel"); + print_string(PRINT_ANY, "mode", "\n mode:%s", "channel"); } else { - fprintf(f, "\n mode:dcb"); + print_string(PRINT_ANY, "mode", "\n mode:%s", "dcb"); } if (tb[TCA_MQPRIO_SHAPER]) { __u16 *shaper = RTA_DATA(tb[TCA_MQPRIO_SHAPER]); if (*shaper == TC_MQPRIO_SHAPER_BW_RATE) - fprintf(f, "\n shaper:bw_rlimit"); + print_string(PRINT_ANY, "shaper", "\n shaper:%s", "bw_rlimit"); } else { - fprintf(f, "\n shaper:dcb"); + print_string(PRINT_ANY, "shaper", "\n shaper:%s", "dcb"); } if (tb[TCA_MQPRIO_MIN_RATE64]) { @@ -287,9 +293,10 @@ static int mqprio_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt) return -1; *(min++) = rta_getattr_u64(r); } - fprintf(f, " min_rate:"); + open_json_array(PRINT_ANY, is_json_context() ? "min_rate" : " min_rate:"); for (i = 0; i < qopt->num_tc; i++) - fprintf(f, "%s ", sprint_rate(min_rate64[i], b1)); + print_string(PRINT_ANY, NULL, "%s ", sprint_rate(min_rate64[i], b1)); + close_json_array(PRINT_ANY, ""); } if (tb[TCA_MQPRIO_MAX_RATE64]) { @@ -303,9 +310,10 @@ static int mqprio_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt) return -1; *(max++) = rta_getattr_u64(r); } - fprintf(f, " max_rate:"); + open_json_array(PRINT_ANY, is_json_context() ? "max_rate" : " max_rate:"); for (i = 0; i < qopt->num_tc; i++) - fprintf(f, "%s ", sprint_rate(max_rate64[i], b1)); + print_string(PRINT_ANY, NULL, "%s ", sprint_rate(max_rate64[i], b1)); + close_json_array(PRINT_ANY, ""); } } return 0;