From: Donald Sharp Date: Wed, 23 May 2018 12:25:51 +0000 (-0400) Subject: zebra: Ignore most netlink notifications from ourselves X-Git-Tag: frr-6.1-dev~401^2~3 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=3575d9e86607b6f83f8f3bef6979fe804bfe6de2;p=mirror%2Ffrr.git zebra: Ignore most netlink notifications from ourselves The BPF filter was an exclusion list of netlink messages we did not want to receive from our self. The problem with this is that the exclusion list was and will be ever growing. So switch the test around to an inclusion list since it is shorter and not growing. Right now this is RTM_NEWADDR and RTM_DELADDR. Change some of the debug messages to error messages so that when something slips through and it is unexpected during development we will see the problem. Also try to improve the documentation about what the filter is doing and leave some breadcrumbs for future developers to know where to change code when new functionality is added. Signed-off-by: Donald Sharp --- diff --git a/zebra/kernel_netlink.c b/zebra/kernel_netlink.c index 6b587dab38..b26f6f83a9 100644 --- a/zebra/kernel_netlink.c +++ b/zebra/kernel_netlink.c @@ -131,8 +131,20 @@ extern struct zebra_privs_t zserv_privs; int netlink_talk_filter(struct sockaddr_nl *snl, struct nlmsghdr *h, ns_id_t ns_id, int startup) { - zlog_warn("netlink_talk: ignoring message type 0x%04x NS %u", - h->nlmsg_type, ns_id); + /* + * This is an error condition that must be handled during + * development. + * + * The netlink_talk_filter function is used for communication + * down the netlink_cmd pipe and we are expecting + * an ack being received. So if we get here + * then we did not receive the ack and instead + * received some other message in an unexpected + * way. + */ + zlog_err("%s: ignoring message type 0x%04x(%s) NS %u", + __PRETTY_FUNCTION__, h->nlmsg_type, + nl_msg_type_to_str(h->nlmsg_type), ns_id); return 0; } @@ -242,6 +254,15 @@ static int netlink_information_fetch(struct sockaddr_nl *snl, return 0; } + /* + * When we handle new message types here + * because we are starting to install them + * then lets check the netlink_install_filter + * and see if we should add the corresponding + * allow through entry there. + * Probably not needed to do but please + * think about it. + */ switch (h->nlmsg_type) { case RTM_NEWROUTE: return netlink_route_change(snl, h, ns_id, startup); @@ -264,9 +285,16 @@ static int netlink_information_fetch(struct sockaddr_nl *snl, case RTM_DELRULE: return netlink_rule_change(snl, h, ns_id, startup); default: - if (IS_ZEBRA_DEBUG_KERNEL) - zlog_debug("Unknown netlink nlmsg_type %d vrf %u\n", - h->nlmsg_type, ns_id); + /* + * If we have received this message then + * we have made a mistake during development + * and we need to write some code to handle + * this message type or not ask for + * it to be sent up to us + */ + zlog_err("Unknown netlink nlmsg_type %s(%d) vrf %u\n", + nl_msg_type_to_str(h->nlmsg_type), h->nlmsg_type, + ns_id); break; } return 0; @@ -283,31 +311,69 @@ static int kernel_read(struct thread *thread) return 0; } -/* Filter out messages from self that occur on listener socket, +/* + * Filter out messages from self that occur on listener socket, * caused by our actions on the command socket + * + * When we add new Netlink message types we probably + * do not need to add them here as that we are filtering + * on the routes we actually care to receive( which is rarer + * then the normal course of operations). We are intentionally + * allowing some messages from ourselves through + * ( I'm looking at you Interface based netlink messages ) + * so that we only had to write one way to handle incoming + * address add/delete changes. */ static void netlink_install_filter(int sock, __u32 pid) { + /* + * BPF_JUMP instructions and where you jump to are based upon + * 0 as being the next statement. So count from 0. Writing + * this down because every time I look at this I have to + * re-remember it. + */ struct sock_filter filter[] = { - /* 0: ldh [4] */ - BPF_STMT(BPF_LD | BPF_ABS | BPF_H, - offsetof(struct nlmsghdr, nlmsg_type)), - /* 1: jeq 0x18 jt 5 jf next */ - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, htons(RTM_NEWROUTE), 3, 0), - /* 2: jeq 0x19 jt 5 jf next */ - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, htons(RTM_DELROUTE), 2, 0), - /* 3: jeq 0x19 jt 5 jf next */ - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, htons(RTM_NEWNEIGH), 1, 0), - /* 4: jeq 0x19 jt 5 jf 8 */ - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, htons(RTM_DELNEIGH), 0, 3), - /* 5: ldw [12] */ + /* + * Logic: + * if (nlmsg_pid == pid) { + * if (the incoming nlmsg_type == + * RTM_NEWADDR | RTM_DELADDR) + * keep this message + * else + * skip this message + * } else + * keep this netlink message + */ + /* + * 0: Load the nlmsg_pid into the BPF register + */ BPF_STMT(BPF_LD | BPF_ABS | BPF_W, offsetof(struct nlmsghdr, nlmsg_pid)), - /* 6: jeq XX jt 7 jf 8 */ - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, htonl(pid), 0, 1), - /* 7: ret 0 (skip) */ + /* + * 1: Compare to pid + */ + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, htonl(pid), 0, 4), + /* + * 2: Load the nlmsg_type into BPF register + */ + BPF_STMT(BPF_LD | BPF_ABS | BPF_H, + offsetof(struct nlmsghdr, nlmsg_type)), + /* + * 3: Compare to RTM_NEWADDR + */ + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, htons(RTM_NEWADDR), 2, 0), + /* + * 4: Compare to RTM_DELADDR + */ + BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, htons(RTM_DELADDR), 1, 0), + /* + * 5: This is the end state of we want to skip the + * message + */ BPF_STMT(BPF_RET | BPF_K, 0), - /* 8: ret 0xffff (keep) */ + /* 6: This is the end state of we want to keep + * the message + */ BPF_STMT(BPF_RET | BPF_K, 0xffff), }; @@ -620,23 +686,6 @@ int netlink_parse_info(int (*filter)(struct sockaddr_nl *, struct nlmsghdr *, h->nlmsg_type, h->nlmsg_len, h->nlmsg_seq, h->nlmsg_pid); - /* skip unsolicited messages originating from command - * socket - * linux sets the originators port-id for {NEW|DEL}ADDR - * messages, - * so this has to be checked here. */ - if (nl != &zns->netlink_cmd - && h->nlmsg_pid == zns->netlink_cmd.snl.nl_pid - && (h->nlmsg_type != RTM_NEWADDR - && h->nlmsg_type != RTM_DELADDR)) { - if (IS_ZEBRA_DEBUG_KERNEL) - zlog_debug( - "netlink_parse_info: %s packet comes from %s", - zns->netlink_cmd.name, - nl->name); - continue; - } - error = (*filter)(&snl, h, zns->ns_id, startup); if (error < 0) { zlog_err("%s filter function error", nl->name);