From 7c87c7fed18d1162e045c8331cb68fa440bc5728 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Tue, 29 Aug 2017 17:09:45 +0200 Subject: [PATCH 1/4] lib/bpf: Fix bytecode-file parsing The signedness of char type is implementation dependent, and there are architectures on which it is unsigned by default. In that case, the check whether fgetc() returned EOF failed because the return value was assigned an (unsigned) char variable prior to comparison with EOF (which is defined to -1). Fix this by using int as type for 'c' variable, which also matches the declaration of fgetc(). While being at it, fix the parser logic to correctly handle multiple empty lines and consecutive whitespace and tab characters to further improve the parser's robustness. Note that this will still detect double separator characters, so doesn't soften up the parser too much. Fixes: 3da3ebfca85b8 ("bpf: Make bytecode-file reading a little more robust") Cc: Daniel Borkmann Signed-off-by: Phil Sutter Acked-by: Daniel Borkmann --- lib/bpf.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/bpf.c b/lib/bpf.c index c180934a..5fd4928c 100644 --- a/lib/bpf.c +++ b/lib/bpf.c @@ -208,8 +208,9 @@ static int bpf_parse_string(char *arg, bool from_file, __u16 *bpf_len, if (from_file) { size_t tmp_len, op_len = sizeof("65535 255 255 4294967295,"); - char *tmp_string, *pos, c, c_prev = ' '; + char *tmp_string, *pos, c_prev = ' '; FILE *fp; + int c; tmp_len = sizeof("4096,") + BPF_MAXINSNS * op_len; tmp_string = pos = calloc(1, tmp_len); @@ -228,18 +229,20 @@ static int bpf_parse_string(char *arg, bool from_file, __u16 *bpf_len, case '\n': if (c_prev != ',') *(pos++) = ','; + c_prev = ','; break; case ' ': case '\t': if (c_prev != ' ') *(pos++) = c; + c_prev = ' '; break; default: *(pos++) = c; + c_prev = c; } if (pos - tmp_string == tmp_len) break; - c_prev = c; } if (!feof(fp)) { From b75e0f6f4b6b29e7bc2f103cbcfa99bef73527a0 Mon Sep 17 00:00:00 2001 From: Simon Horman Date: Tue, 5 Sep 2017 13:06:24 +0200 Subject: [PATCH 2/4] tc actions: store and dump correct length of user cookies Correct two errors which cancel each other out: * Do not send twice the length of the actual provided by the user to the kernel * Do not dump half the length of the cookie provided by the kernel As the cookie is now stored in the kernel at its correct length rather than double the that length cookies of up to the maximum size of 16 bytes may now be stored rather than a maximum of half that length. Output of dump is the same before and after this change, but the data stored in the kernel is now exactly the cookie rather than the cookie + as many trailing zeros. Before: # tc filter add dev eth0 protocol ip parent ffff: \ flower ip_proto udp action drop \ cookie 0123456789abcdef0123456789abcdef RTNETLINK answers: Invalid argument After: # tc filter add dev eth0 protocol ip parent ffff: \ flower ip_proto udp action drop \ cookie 0123456789abcdef0123456789abcdef # tc filter show dev eth0 ingress eth_type ipv4 ip_proto udp not_in_hw action order 1: gact action drop random type none pass val 0 index 1 ref 1 bind 1 installed 1 sec used 1 sec Action statistics: Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0 cookie len 16 0123456789abcdef0123456789abcdef Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies") Cc: Jamal Hadi Salim Signed-off-by: Simon Horman --- tc/m_action.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tc/m_action.c b/tc/m_action.c index 6ebe85e1..397e9c5d 100644 --- a/tc/m_action.c +++ b/tc/m_action.c @@ -242,7 +242,7 @@ done0: invarg("cookie must be a hex string\n", *argv); - act_ck_len = slen; + act_ck_len = slen / 2; argc--; argv++; } @@ -307,7 +307,7 @@ static int tc_print_one_action(FILE *f, struct rtattr *arg) print_tcstats2_attr(f, tb[TCA_ACT_STATS], "\t", NULL); if (tb[TCA_ACT_COOKIE]) { int strsz = RTA_PAYLOAD(tb[TCA_ACT_COOKIE]); - char b1[strsz+1]; + char b1[strsz * 2 + 1]; fprintf(f, "\n\tcookie len %d %s ", strsz, hexstring_n2a(RTA_DATA(tb[TCA_ACT_COOKIE]), From 1b736dc469dcabd4180848a1f1b3d1fef2b84dbc Mon Sep 17 00:00:00 2001 From: Daniel Borkmann Date: Tue, 5 Sep 2017 02:24:31 +0200 Subject: [PATCH 3/4] bpf: minor cleanups for bpf_trace_pipe Just minor nits, e.g. no need to fflush() and instead of returning right away, just break and close the fd. Signed-off-by: Daniel Borkmann --- lib/bpf.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/bpf.c b/lib/bpf.c index 5fd4928c..7463fdc8 100644 --- a/lib/bpf.c +++ b/lib/bpf.c @@ -569,9 +569,9 @@ int bpf_trace_pipe(void) "/trace", 0, }; + int fd_in, fd_out = STDERR_FILENO; char tpipe[PATH_MAX]; const char *mnt; - int fd; mnt = bpf_find_mntpt("tracefs", TRACEFS_MAGIC, tracefs_mnt, sizeof(tracefs_mnt), tracefs_known_mnts); @@ -582,8 +582,8 @@ int bpf_trace_pipe(void) snprintf(tpipe, sizeof(tpipe), "%s/trace_pipe", mnt); - fd = open(tpipe, O_RDONLY); - if (fd < 0) + fd_in = open(tpipe, O_RDONLY); + if (fd_in < 0) return -1; fprintf(stderr, "Running! Hang up with ^C!\n\n"); @@ -591,15 +591,14 @@ int bpf_trace_pipe(void) static char buff[4096]; ssize_t ret; - ret = read(fd, buff, sizeof(buff) - 1); - if (ret > 0) { - if (write(STDERR_FILENO, buff, ret) != ret) - return -1; - fflush(stderr); - } + ret = read(fd_in, buff, sizeof(buff)); + if (ret > 0 && write(fd_out, buff, ret) == ret) + continue; + break; } - return 0; + close(fd_in); + return -1; } static int bpf_gen_global(const char *bpf_sub_dir) From a0b5b7cf5c05c7aa29c683c52f896c74ab7924b0 Mon Sep 17 00:00:00 2001 From: Daniel Borkmann Date: Tue, 5 Sep 2017 02:24:32 +0200 Subject: [PATCH 4/4] bpf: consolidate dumps to use bpf_dump_prog_info Consolidate dump of prog info to use bpf_dump_prog_info() when possible. Moving forward, we want to have a consistent output for BPF progs when being dumped. E.g. in cls/act case we used to dump tag as a separate netlink attribute before we had BPF_OBJ_GET_INFO_BY_FD bpf(2) command. Move dumping tag into bpf_dump_prog_info() as well, and only dump the netlink attribute for older kernels. Also, reuse bpf_dump_prog_info() for XDP case, so we can dump tag and whether program was jited, which we currently don't show. Signed-off-by: Daniel Borkmann --- include/bpf_util.h | 2 +- ip/ipaddress.c | 6 ++++-- ip/iplink_xdp.c | 19 +++++++++++++++---- ip/xdp.h | 2 +- lib/bpf.c | 12 +++++++++--- tc/f_bpf.c | 8 ++++---- tc/m_bpf.c | 8 ++++---- 7 files changed, 38 insertions(+), 19 deletions(-) diff --git a/include/bpf_util.h b/include/bpf_util.h index 6582ec8c..e818221d 100644 --- a/include/bpf_util.h +++ b/include/bpf_util.h @@ -261,7 +261,7 @@ int bpf_prog_load(enum bpf_prog_type type, const struct bpf_insn *insns, int bpf_prog_attach_fd(int prog_fd, int target_fd, enum bpf_attach_type type); int bpf_prog_detach_fd(int target_fd, enum bpf_attach_type type); -void bpf_dump_prog_info(FILE *f, uint32_t id); +int bpf_dump_prog_info(FILE *f, uint32_t id); #ifdef HAVE_ELF int bpf_send_map_fds(const char *path, const char *obj); diff --git a/ip/ipaddress.c b/ip/ipaddress.c index c9312f06..dbdd839c 100644 --- a/ip/ipaddress.c +++ b/ip/ipaddress.c @@ -837,7 +837,7 @@ int print_linkinfo(const struct sockaddr_nl *who, if (tb[IFLA_MTU]) fprintf(fp, "mtu %u ", rta_getattr_u32(tb[IFLA_MTU])); if (tb[IFLA_XDP]) - xdp_dump(fp, tb[IFLA_XDP]); + xdp_dump(fp, tb[IFLA_XDP], do_link, false); if (tb[IFLA_QDISC]) fprintf(fp, "qdisc %s ", rta_getattr_str(tb[IFLA_QDISC])); if (tb[IFLA_MASTER]) { @@ -951,12 +951,14 @@ int print_linkinfo(const struct sockaddr_nl *who, } } - if ((do_link || show_details) && tb[IFLA_IFALIAS]) { fprintf(fp, "%s alias %s", _SL_, rta_getattr_str(tb[IFLA_IFALIAS])); } + if ((do_link || show_details) && tb[IFLA_XDP]) + xdp_dump(fp, tb[IFLA_XDP], true, true); + if (do_link && show_stats) { fprintf(fp, "%s", _SL_); __print_link_stats(fp, tb); diff --git a/ip/iplink_xdp.c b/ip/iplink_xdp.c index 9ae9ee5d..5aa66fe7 100644 --- a/ip/iplink_xdp.c +++ b/ip/iplink_xdp.c @@ -81,9 +81,10 @@ int xdp_parse(int *argc, char ***argv, struct iplink_req *req, bool generic, return 0; } -void xdp_dump(FILE *fp, struct rtattr *xdp) +void xdp_dump(FILE *fp, struct rtattr *xdp, bool link, bool details) { struct rtattr *tb[IFLA_XDP_MAX + 1]; + __u32 prog_id = 0; __u8 mode; parse_rtattr_nested(tb, IFLA_XDP_MAX, xdp); @@ -94,6 +95,8 @@ void xdp_dump(FILE *fp, struct rtattr *xdp) mode = rta_getattr_u8(tb[IFLA_XDP_ATTACHED]); if (mode == XDP_ATTACHED_NONE) return; + else if (details && link) + fprintf(fp, "%s prog/xdp", _SL_); else if (mode == XDP_ATTACHED_DRV) fprintf(fp, "xdp"); else if (mode == XDP_ATTACHED_SKB) @@ -104,8 +107,16 @@ void xdp_dump(FILE *fp, struct rtattr *xdp) fprintf(fp, "xdp[%u]", mode); if (tb[IFLA_XDP_PROG_ID]) - fprintf(fp, "/id:%u", - rta_getattr_u32(tb[IFLA_XDP_PROG_ID])); + prog_id = rta_getattr_u32(tb[IFLA_XDP_PROG_ID]); + if (!details) { + if (prog_id && !link) + fprintf(fp, "/id:%u", prog_id); + fprintf(fp, " "); + return; + } - fprintf(fp, " "); + if (prog_id) { + fprintf(fp, " "); + bpf_dump_prog_info(fp, prog_id); + } } diff --git a/ip/xdp.h b/ip/xdp.h index ba897a29..1efd591b 100644 --- a/ip/xdp.h +++ b/ip/xdp.h @@ -5,6 +5,6 @@ int xdp_parse(int *argc, char ***argv, struct iplink_req *req, bool generic, bool drv, bool offload); -void xdp_dump(FILE *fp, struct rtattr *tb); +void xdp_dump(FILE *fp, struct rtattr *tb, bool link, bool details); #endif /* __XDP__ */ diff --git a/lib/bpf.c b/lib/bpf.c index 7463fdc8..cfa1f79a 100644 --- a/lib/bpf.c +++ b/lib/bpf.c @@ -179,25 +179,31 @@ static int bpf_prog_info_by_fd(int fd, struct bpf_prog_info *info, return ret; } -void bpf_dump_prog_info(FILE *f, uint32_t id) +int bpf_dump_prog_info(FILE *f, uint32_t id) { struct bpf_prog_info info = {}; uint32_t len = sizeof(info); - int fd, ret; + int fd, ret, dump_ok = 0; + SPRINT_BUF(tmp); fprintf(f, "id %u ", id); fd = bpf_prog_fd_by_id(id); if (fd < 0) - return; + return dump_ok; ret = bpf_prog_info_by_fd(fd, &info, &len); if (!ret && len) { + fprintf(f, "tag %s ", + hexstring_n2a(info.tag, sizeof(info.tag), + tmp, sizeof(tmp))); if (info.jited_prog_len) fprintf(f, "jited "); + dump_ok = 1; } close(fd); + return dump_ok; } static int bpf_parse_string(char *arg, bool from_file, __u16 *bpf_len, diff --git a/tc/f_bpf.c b/tc/f_bpf.c index 2f8d12a6..4fb92093 100644 --- a/tc/f_bpf.c +++ b/tc/f_bpf.c @@ -177,6 +177,7 @@ static int bpf_print_opt(struct filter_util *qu, FILE *f, struct rtattr *opt, __u32 handle) { struct rtattr *tb[TCA_BPF_MAX + 1]; + int dump_ok = 0; if (opt == NULL) return 0; @@ -221,7 +222,9 @@ static int bpf_print_opt(struct filter_util *qu, FILE *f, bpf_print_ops(f, tb[TCA_BPF_OPS], rta_getattr_u16(tb[TCA_BPF_OPS_LEN])); - if (tb[TCA_BPF_TAG]) { + if (tb[TCA_BPF_ID]) + dump_ok = bpf_dump_prog_info(f, rta_getattr_u32(tb[TCA_BPF_ID])); + if (!dump_ok && tb[TCA_BPF_TAG]) { SPRINT_BUF(b); fprintf(f, "tag %s ", @@ -230,9 +233,6 @@ static int bpf_print_opt(struct filter_util *qu, FILE *f, b, sizeof(b))); } - if (tb[TCA_BPF_ID]) - bpf_dump_prog_info(f, rta_getattr_u32(tb[TCA_BPF_ID])); - if (tb[TCA_BPF_POLICE]) { fprintf(f, "\n"); tc_print_police(f, tb[TCA_BPF_POLICE]); diff --git a/tc/m_bpf.c b/tc/m_bpf.c index df559bcc..e3d0a2b1 100644 --- a/tc/m_bpf.c +++ b/tc/m_bpf.c @@ -154,6 +154,7 @@ static int bpf_print_opt(struct action_util *au, FILE *f, struct rtattr *arg) { struct rtattr *tb[TCA_ACT_BPF_MAX + 1]; struct tc_act_bpf *parm; + int dump_ok = 0; if (arg == NULL) return -1; @@ -177,7 +178,9 @@ static int bpf_print_opt(struct action_util *au, FILE *f, struct rtattr *arg) fprintf(f, " "); } - if (tb[TCA_ACT_BPF_TAG]) { + if (tb[TCA_ACT_BPF_ID]) + dump_ok = bpf_dump_prog_info(f, rta_getattr_u32(tb[TCA_ACT_BPF_ID])); + if (!dump_ok && tb[TCA_ACT_BPF_TAG]) { SPRINT_BUF(b); fprintf(f, "tag %s ", @@ -186,9 +189,6 @@ static int bpf_print_opt(struct action_util *au, FILE *f, struct rtattr *arg) b, sizeof(b))); } - if (tb[TCA_ACT_BPF_ID]) - bpf_dump_prog_info(f, rta_getattr_u32(tb[TCA_ACT_BPF_ID])); - print_action_control(f, "default-action ", parm->action, "\n"); fprintf(f, "\tindex %u ref %d bind %d", parm->index, parm->refcnt, parm->bindcnt);