From dd81ee04edc3461653f1254084e2294083ccdca6 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Wed, 2 Mar 2016 16:56:27 +0100 Subject: [PATCH 1/5] ifstat, nstat: fix daemon mode Since the relevant code (and it's bugs) is identical in both files, fix them in one go. This patch fixes multiple issues: * Using 'int' for the 'tdiff' variable does not suffice on 64bit systems, the assigned initial time difference makes it wrap and contain a negative value afterwards. Instead use the more appropriate 'time_t' type. * As far as I understood the code, poll() is supposed to time out just at the right time to trigger update_db() in the configured interval. Therefore it's timeout must be set to the desired interval *minus* the time that has already passed since then. * With the last change to the algorithm in place, it does not make sense to call update_db() before returning data to the connected client. Actually, it never does otherwise we could skip the periodic updates in the first place. Signed-off-by: Phil Sutter --- misc/ifstat.c | 9 +++------ misc/nstat.c | 9 +++------ 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/misc/ifstat.c b/misc/ifstat.c index ac5c29c8..694d9839 100644 --- a/misc/ifstat.c +++ b/misc/ifstat.c @@ -589,7 +589,7 @@ static void server_loop(int fd) for (;;) { int status; - int tdiff; + time_t tdiff; struct timeval now; gettimeofday(&now, NULL); @@ -600,7 +600,7 @@ static void server_loop(int fd) tdiff = 0; } - if (poll(&p, 1, tdiff + scan_interval) > 0 + if (poll(&p, 1, scan_interval - tdiff) > 0 && (p.revents&POLLIN)) { int clnt = accept(fd, NULL, NULL); if (clnt >= 0) { @@ -613,11 +613,8 @@ static void server_loop(int fd) close(clnt); } else { FILE *fp = fdopen(clnt, "w"); - if (fp) { - if (tdiff > 0) - update_db(tdiff); + if (fp) dump_raw_db(fp, 0); - } exit(0); } } diff --git a/misc/nstat.c b/misc/nstat.c index 99705286..22b27eba 100644 --- a/misc/nstat.c +++ b/misc/nstat.c @@ -433,7 +433,7 @@ static void server_loop(int fd) for (;;) { int status; - int tdiff; + time_t tdiff; struct timeval now; gettimeofday(&now, NULL); tdiff = T_DIFF(now, snaptime); @@ -442,7 +442,7 @@ static void server_loop(int fd) snaptime = now; tdiff = 0; } - if (poll(&p, 1, tdiff + scan_interval) > 0 + if (poll(&p, 1, scan_interval - tdiff) > 0 && (p.revents&POLLIN)) { int clnt = accept(fd, NULL, NULL); if (clnt >= 0) { @@ -455,11 +455,8 @@ static void server_loop(int fd) close(clnt); } else { FILE *fp = fdopen(clnt, "w"); - if (fp) { - if (tdiff > 0) - update_db(tdiff); + if (fp) dump_kern_db(fp, 0); - } exit(0); } } From 72b365e8e0fd5efe1d5c05d04c25950736635cfb Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Fri, 4 Mar 2016 19:57:28 +0100 Subject: [PATCH 2/5] libnetlink: Double the dump buffer size There have been reports about 'ip addr' printing "Message truncated" on systems with large numbers of VFs. Although I haven't been able to get my hands on hardware suitable to reproduce this, increasing the dump buffer has been reported to resolve the issue. For want of a better idea, just double the buffer size to 32k. Feels like this opportunistic buffer size selection is rather workarounding a design flaw in libnetlink or maybe even the netlink protocol itself. Signed-off-by: Phil Sutter --- lib/libnetlink.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/libnetlink.c b/lib/libnetlink.c index d6b5fd3e..245c4ca2 100644 --- a/lib/libnetlink.c +++ b/lib/libnetlink.c @@ -223,7 +223,7 @@ int rtnl_dump_filter_l(struct rtnl_handle *rth, .msg_iov = &iov, .msg_iovlen = 1, }; - char buf[16384]; + char buf[32768]; int dump_intr = 0; iov.iov_base = buf; From ec0ceeec4954b1a5439ec3684460e8385454de90 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Wed, 2 Mar 2016 12:20:29 +0100 Subject: [PATCH 3/5] tc: pedit: Fix layered op parsing After lookup of the layered op submodule, pedit would pass argv and argc including the layered op identifier at first position which confused the submodule parser. Fix this by calling NEXT_ARG() before calling the parse_peopt() callback. Signed-off-by: Phil Sutter --- tc/m_pedit.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tc/m_pedit.c b/tc/m_pedit.c index 86eb0ca3..26272d3c 100644 --- a/tc/m_pedit.c +++ b/tc/m_pedit.c @@ -422,6 +422,7 @@ parse_munge(int *argc_p, char ***argv_p,struct tc_pedit_sel *sel) p = get_pedit_kind(k); if (NULL == p) goto bad_val; + NEXT_ARG(); res = p->parse_peopt(&argc, &argv, sel,&tkey); if (res < 0) { fprintf(stderr,"bad pedit parsing\n"); From f440e9d8c2f6f0eb2e9fccd8f4d7c42c11ba4979 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Wed, 2 Mar 2016 12:20:30 +0100 Subject: [PATCH 4/5] tc: pedit: Fix parse_cmd() This was horribly broken: * pack_key8() and pack_key16() ... * missed to invert retain value when applying it to the mask, * did not sanitize val by ANDing it with retain, * and ignored the mask which is necessary for 'invert' command. * pack_key16() did not convert mask to network byte order. * Changing the retain value for 'invert' or 'retain' operation seems just plain wrong. * While here, also got rid of unnecessary offset sanitization in pack_key32(). * Simplify code a bit by always assigning the local mask variable to tkey->mask before calling any of the pack_key*() variants. Signed-off-by: Phil Sutter --- tc/m_pedit.c | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/tc/m_pedit.c b/tc/m_pedit.c index 26272d3c..a7960d52 100644 --- a/tc/m_pedit.c +++ b/tc/m_pedit.c @@ -153,8 +153,6 @@ pack_key32(__u32 retain,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey) tkey->val = htonl(tkey->val & retain); tkey->mask = htonl(tkey->mask | ~retain); - /* jamal remove this - it is not necessary given the if check above */ - tkey->off &= ~3; return pack_key(sel,tkey); } @@ -177,11 +175,8 @@ pack_key16(__u32 retain,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey) } stride = 8 * ind; - tkey->val = htons(tkey->val); - tkey->val <<= stride; - tkey->mask <<= stride; - retain <<= stride; - tkey->mask = retain|m[ind]; + tkey->val = htons(tkey->val & retain) << stride; + tkey->mask = (htons(tkey->mask | ~retain) << stride) | m[ind]; tkey->off &= ~3; @@ -205,10 +200,8 @@ pack_key8(__u32 retain,struct tc_pedit_sel *sel,struct tc_pedit_key *tkey) ind = tkey->off & 3; stride = 8 * ind; - tkey->val <<= stride; - tkey->mask <<= stride; - retain <<= stride; - tkey->mask = retain|m[ind]; + tkey->val = (tkey->val & retain) << stride; + tkey->mask = ((tkey->mask | ~retain) << stride) | m[ind]; tkey->off &= ~3; @@ -269,13 +262,13 @@ parse_cmd(int *argc_p, char ***argv_p, __u32 len, int type,__u32 retain,struct t o = 0xFFFFFFFF; if (matches(*argv, "invert") == 0) { - retain = val = mask = o; + val = mask = o; } else if (matches(*argv, "set") == 0) { NEXT_ARG(); if (parse_val(&argc, &argv, &val, type)) return -1; } else if (matches(*argv, "preserve") == 0) { - retain = mask = o; + retain = 0; } else { if (matches(*argv, "clear") != 0) return -1; @@ -291,19 +284,17 @@ parse_cmd(int *argc_p, char ***argv_p, __u32 len, int type,__u32 retain,struct t } tkey->val = val; + tkey->mask = mask; if (len == 1) { - tkey->mask = 0xFF; res = pack_key8(retain,sel,tkey); goto done; } if (len == 2) { - tkey->mask = mask; res = pack_key16(retain,sel,tkey); goto done; } if (len == 4) { - tkey->mask = mask; res = pack_key32(retain,sel,tkey); goto done; } From 338b003bcc22a62c98b84dbe5e491cae84dbb03c Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Wed, 2 Mar 2016 12:20:31 +0100 Subject: [PATCH 5/5] tc: pedit: Fix retain value for ihl adjustments Since the IP Header Length field is just half a byte, adjust retain to only match these bits so the Version field is not overwritten by accident. The whole concept is actually broken due to dependency on endianness which pedit ignores. Signed-off-by: Phil Sutter --- tc/p_ip.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tc/p_ip.c b/tc/p_ip.c index 08fdbaa4..10e4bebc 100644 --- a/tc/p_ip.c +++ b/tc/p_ip.c @@ -58,7 +58,7 @@ parse_ip(int *argc_p, char ***argv_p,struct tc_pedit_sel *sel,struct tc_pedit_ke if (strcmp(*argv, "ihl") == 0) { NEXT_ARG(); tkey->off = 0; - res = parse_cmd(&argc, &argv, 1, TU32,RU8,sel,tkey); + res = parse_cmd(&argc, &argv, 1, TU32,0x0f,sel,tkey); goto done; } if (strcmp(*argv, "protocol") == 0) {