From c37a21ea2906aaafec82b76f4ae5d1aae81ac057 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 14 Jan 2022 09:35:53 +0100 Subject: [PATCH] Revert "platform: add bpf filter to ignore routes from routing daemons" The socket BPF filter operates on the entire message (skb). One netlink message (for RTM_NEWROUTE) usually contains a list of routes (struct nlmsghdr), but the BPF filter was only looking at the first route to decide whether to throw away the entire message. That is wrong. This causes that we miss routes. It also means that the response to our RTM_GETROUTE dump request could be filtered and we poll/wait (unsuccessfully) for it: [1641829930.4111] platform-linux: netlink: read: wait for ACK for sequence number 26... To get this right, the BPF filter would have to look at all routes in the list to find out whether there are any we want to receive (and if there are any, to pass the entire message, regardless that is might also contain routes we want to ignore). As BPF does not support loops, that is not trivial. Arguably, we still could do something where we look at a bounded, unrolled number of routes, in the hope that there are not more routes in one message that we support. The problem is that this BPF filter is supposed to filter out massive amounts of routes, so most messages will be filled to the brim with routes and we would have to expect a high number of routes in the RTM_NEWROUTE that need checking. We could still ignore routes in _new_from_nl_route(), to enter the cache. That means, we would catch them later than with the BPF filter, but still before a lot of the overhead of handling them. That probably will be done next, but for now revert the commit. This reverts commit e9ca5583e5f37f922e0b000a8cff2c2813ce2e69. https://bugzilla.redhat.com/show_bug.cgi?id=2037411 --- src/libnm-platform/nm-linux-platform.c | 9 ----- src/libnm-platform/nm-netlink.c | 50 -------------------------- src/libnm-platform/nm-netlink.h | 2 -- 3 files changed, 61 deletions(-) diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 7f65b0d78d..296fa49ccb 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -3409,11 +3409,6 @@ _new_from_nl_route(struct nlmsghdr *nlh, gboolean id_only) if (!NM_IN_SET(rtm->rtm_type, RTN_UNICAST, RTN_LOCAL)) return NULL; - /* A BPF filter on the netlink socket already enforces this, but - * check it also here in case the filter couldn't be installed. */ - if (rtm->rtm_protocol > RTPROT_STATIC && !NM_IN_SET(rtm->rtm_protocol, RTPROT_DHCP, RTPROT_RA)) - return NULL; - if (nlmsg_parse_arr(nlh, sizeof(struct rtmsg), tb, policy) < 0) return NULL; @@ -9548,10 +9543,6 @@ constructed(GObject *_object) nm_assert(!nle); } - nle = nl_socket_set_rtprot_filter(priv->nlh); - if (nle) - _LOGW("error adding routing protocol filter: %s (%d)", nm_strerror(nle), -nle); - fd = nl_socket_get_fd(priv->nlh); _LOGD("Netlink socket for events established: port=%u, fd=%d", diff --git a/src/libnm-platform/nm-netlink.c b/src/libnm-platform/nm-netlink.c index 09369ef8d0..949514536f 100644 --- a/src/libnm-platform/nm-netlink.c +++ b/src/libnm-platform/nm-netlink.c @@ -9,7 +9,6 @@ #include #include -#include /*****************************************************************************/ @@ -1046,55 +1045,6 @@ nl_socket_set_ext_ack(struct nl_sock *sk, gboolean enable) return 0; } -int -nl_socket_set_rtprot_filter(struct nl_sock *sk) -{ - struct sock_filter filter[] = { - /* - * if (h->nlmsg_type != RTM_NEWROUTE && h->nlmsg_type != RTM_DELROUTE) - * return KEEP; - */ - BPF_STMT(BPF_LD | BPF_ABS | BPF_H, offsetof(struct nlmsghdr, nlmsg_type)), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, htons(RTM_NEWROUTE), 2, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, htons(RTM_DELROUTE), 1, 0), - BPF_STMT(BPF_RET | BPF_K, 0xFFFF), - - /* - * if (rtm->rtm_protocol <= RTPROT_STATIC) - * return KEEP; - */ - BPF_STMT(BPF_LD | BPF_ABS | BPF_B, - sizeof(struct nlmsghdr) + offsetof(struct rtmsg, rtm_protocol)), - BPF_JUMP(BPF_JMP | BPF_JGT | BPF_K, RTPROT_STATIC, 1, 0), - BPF_STMT(BPF_RET | BPF_K, 0xFFFF), - - /* - * if (rtm->rtm_protocol == RTPROT_DHCP || rtm->rtm_protocol == RTPROT_RA) - * return KEEP; - */ - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, RTPROT_DHCP, 1, 0), - BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, RTPROT_RA, 0, 1), - BPF_STMT(BPF_RET | BPF_K, 0xFFFF), - - /* return DROP; */ - BPF_STMT(BPF_RET | BPF_K, 0), - }; - struct sock_fprog prog = { - .len = sizeof(filter) / sizeof(filter[0]), - .filter = filter, - }; - int err; - - if (sk->s_fd == -1) - return -NME_NL_BAD_SOCK; - - err = setsockopt(sk->s_fd, SOL_SOCKET, SO_ATTACH_FILTER, &prog, sizeof(prog)); - if (err < 0) - return -nm_errno_from_native(errno); - - return 0; -} - void nl_socket_disable_msg_peek(struct nl_sock *sk) { diff --git a/src/libnm-platform/nm-netlink.h b/src/libnm-platform/nm-netlink.h index 6d5aedf807..ac3159bf5f 100644 --- a/src/libnm-platform/nm-netlink.h +++ b/src/libnm-platform/nm-netlink.h @@ -568,8 +568,6 @@ int nl_wait_for_ack(struct nl_sock *sk, const struct nl_cb *cb); int nl_socket_set_ext_ack(struct nl_sock *sk, gboolean enable); -int nl_socket_set_rtprot_filter(struct nl_sock *sk); - /*****************************************************************************/ void *genlmsg_put(struct nl_msg *msg,