aparcar / openwrt

Staging tree of Paul Spooren
Other
9 stars 1 forks source link

FS#1425 - iproute2: tc qdisc produces incorrect output #1254

Closed aparcar closed 6 years ago

aparcar commented 6 years ago

None:

AR71xx - Archer c7 v2

tc qdisc output is producing bizarre output:

tc qdisc qdisc noqueue 0: dev lo root refcnt 4486716 qdisc fq_codel 0: dev eth1 root refcnt 4486716 limit 4498840p flows 4536204 quantum 4539856 target 5.0ms interval 100.0ms memory_limit 4Mb ecn qdisc noqueue 0: dev br-lan root refcnt 4486716 qdisc noqueue 0: dev eth1.2 root refcnt 4486716 qdisc noqueue 0: dev br-wifi_guest root refcnt 4486716 qdisc noqueue 0: dev eth1.15 root refcnt 4486716 qdisc noqueue 0: dev wlan1 root refcnt 4486716 qdisc noqueue 0: dev wlan0 root refcnt 4486716 qdisc noqueue 0: dev wlan1-1 root refcnt 4486716 qdisc noqueue 0: dev wlan0-1 root refcnt 4486716

Note refcnt, (and for fq_codel) limit, flows & quantum are all highly unlikely values. For more curiosity factor, if you ask for json output from tc you get the correct results:

tc -j qdisc [{ "kind": "noqueue", "handle": "0:", "dev": "lo", "root": true, "refcnt": 2, "options": {} },{ "kind": "fq_codel", "handle": "0:", "dev": "eth1", "root": true, "refcnt": 2, "options": { "limit": 10240, "flows": 1024, "quantum": 1514, "target": 4999, "interval": 99999, "memory_limit": 4194304, "ecn": true } },{ "kind": "noqueue", "handle": "0:", "dev": "br-lan", "root": true, "refcnt": 2, "options": {} },{ "kind": "noqueue", "handle": "0:", "dev": "eth1.2", "root": true, "refcnt": 2, "options": {} },{ "kind": "noqueue", "handle": "0:", "dev": "br-wifi_guest", "root": true, "refcnt": 2, "options": {} },{ "kind": "noqueue", "handle": "0:", "dev": "eth1.15", "root": true, "refcnt": 2, "options": {} },{ "kind": "noqueue", "handle": "0:", "dev": "wlan1", "root": true, "refcnt": 2, "options": {} },{ "kind": "noqueue", "handle": "0:", "dev": "wlan0", "root": true, "refcnt": 2, "options": {} },{ "kind": "noqueue", "handle": "0:", "dev": "wlan1-1", "root": true, "refcnt": 2, "options": {} },{ "kind": "noqueue", "handle": "0:", "dev": "wlan0-1", "root": true, "refcnt": 2, "options": {} } ]

That suggests the netlink data transfer mechanism used by iproute2/kernel is okay. Instead it looks like iproute2 tc's print_uint(PRINT_ANY.... ) functions are broken in some way, possibly only on some architectures.

The print_* functions are pre-processor expanded functions mixed across lib/json_print.c & include/json_print.h found in iproute2.

I'm afraid I can't work out what's going on/wrong here.

aparcar commented 6 years ago

None:

Forgot to mention - tried compiling with gcc5 - no change.

aparcar commented 6 years ago

None:

Looking at what is output by the preprocessor things look ok at the tc/q_cake.c

void print_color_uint(enum output_type t, enum color_attr color, const char key, const char fmt, uint64_t value); static inline void print_uint (enum output_type t, const char key, const char fmt, uint64_t value) { print_color_uint( t, COLOR_NONE, key, fmt, value); };

Need to see how 'print_colour_uint' expands.

aparcar commented 6 years ago

None:

Right, cracked it and it’s horrible!

print_uint is expanded thus: Note the type of value uint64_t

         void print_color_uint(enum output_type t, enum color_attr color, const char *key, const char *fmt, uint64_t value);

static inline void print_uint (enum output_type t, const char key, const char fmt, uint64_t value) { print_color_uint( t, COLOR_NONE, key, fmt, value); };

So far so good.

print_color_uint expands to:

        void print_color_uint(enum output_type t, enum color_attr color, const char *key, const char *fmt, uint64_t value)

{ if (((t & PRINT_JSON || t & PRINT_ANY) && _jw)) { if (!key) jsonw_uint(_jw, value); else jsonw_uint_field(_jw, key, value); } else if ((!_jw && (t & PRINT_FP || t & PRINT_ANY))) { color_fprintf( (stdout) , color, fmt, value); } };

Again, no issue and we eventually call color_fprintf

int color_fprintf(FILE fp, enum color_attr attr, const char fmt, ...) { int ret = 0; va_list args;

   va_start(args, fmt);

   if (!color_is_enabled || attr == COLOR_NONE) {
           ret = vfprintf(fp, fmt, args);
           goto end;
   }

Now, color_printf is a variable argument list function and as such is dependent upon being told the correct size of argument variables in the fmt variable. And that’s our problem, we’re passing a 64bit integer but telling the format string that it’s ‘int’…which I’m guessing can be variable sizes depending on architecture, as can the endianness.

If we instead do (in q_cake.c)

include

print_uint(PRINT_ANY, "min_transport_size", " min/max transport layer size: %10" PRIu64, stnc->min_trnlen);

it works. This needs sanity checking by a clever person.

aparcar commented 6 years ago

None:

A suitable fix, also sent to upstream https://patchwork.ozlabs.org/patch/887411/