| Message ID | 012e6863d0103d8dda1932d56427d1b5ba2b9619.1611304190.git.lukas@wunner.de |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | Netfilter egress hook | expand |
On Fri, Jan 22, 2021 at 4:44 AM Lukas Wunner <lukas@wunner.de> wrote: > > From: Pablo Neira Ayuso <pablo@netfilter.org> > > Add egress hook for AF_PACKET sockets that have the PACKET_QDISC_BYPASS > socket option set to on, which allows packets to escape without being > filtered in the egress path. > > This patch only updates the AF_PACKET path, it does not update > dev_direct_xmit() so the XDP infrastructure has a chance to bypass > Netfilter. > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > [lukas: acquire rcu_read_lock, fix typos, rebase] > Signed-off-by: Lukas Wunner <lukas@wunner.de> Isn't the point of PACKET_QDISC_BYPASS to skip steps like this?
On Fri, Jan 22, 2021 at 11:13:19AM -0500, Willem de Bruijn wrote: > On Fri, Jan 22, 2021 at 4:44 AM Lukas Wunner <lukas@wunner.de> wrote: > > Add egress hook for AF_PACKET sockets that have the PACKET_QDISC_BYPASS > > socket option set to on, which allows packets to escape without being > > filtered in the egress path. > > > > This patch only updates the AF_PACKET path, it does not update > > dev_direct_xmit() so the XDP infrastructure has a chance to bypass > > Netfilter. > > Isn't the point of PACKET_QDISC_BYPASS to skip steps like this? I suppose PACKET_QDISC_BYPASS "was introduced to bypass qdisc, not to bypass everything." (The quote is taken from this message by Eric Dumazet: https://lore.kernel.org/netfilter-devel/a9006cf7-f4ba-81b1-fca1-fd2e97939fdc@gmail.com/ ) Thanks, Lukas
On Sun, Jan 24, 2021 at 6:14 AM Lukas Wunner <lukas@wunner.de> wrote: > > On Fri, Jan 22, 2021 at 11:13:19AM -0500, Willem de Bruijn wrote: > > On Fri, Jan 22, 2021 at 4:44 AM Lukas Wunner <lukas@wunner.de> wrote: > > > Add egress hook for AF_PACKET sockets that have the PACKET_QDISC_BYPASS > > > socket option set to on, which allows packets to escape without being > > > filtered in the egress path. > > > > > > This patch only updates the AF_PACKET path, it does not update > > > dev_direct_xmit() so the XDP infrastructure has a chance to bypass > > > Netfilter. > > > > Isn't the point of PACKET_QDISC_BYPASS to skip steps like this? > > I suppose PACKET_QDISC_BYPASS "was introduced to bypass qdisc, > not to bypass everything." > > (The quote is taken from this message by Eric Dumazet: > https://lore.kernel.org/netfilter-devel/a9006cf7-f4ba-81b1-fca1-fd2e97939fdc@gmail.com/ > ) I see. I don't understand the value of a short-cut fast path if we start chipping away at its characteristic feature. It also bypasses sch_handle_egress and packet sockets. This new feature seems broadly similar to the first?
On Sun, Jan 24, 2021 at 11:18:00AM -0500, Willem de Bruijn wrote: > On Sun, Jan 24, 2021 at 6:14 AM Lukas Wunner <lukas@wunner.de> wrote: > > On Fri, Jan 22, 2021 at 11:13:19AM -0500, Willem de Bruijn wrote: > > > On Fri, Jan 22, 2021 at 4:44 AM Lukas Wunner <lukas@wunner.de> wrote: > > > > Add egress hook for AF_PACKET sockets that have the PACKET_QDISC_BYPASS > > > > socket option set to on, which allows packets to escape without being > > > > filtered in the egress path. > > > > > > > > This patch only updates the AF_PACKET path, it does not update > > > > dev_direct_xmit() so the XDP infrastructure has a chance to bypass > > > > Netfilter. > > > > > > Isn't the point of PACKET_QDISC_BYPASS to skip steps like this? > > > > I suppose PACKET_QDISC_BYPASS "was introduced to bypass qdisc, > > not to bypass everything." > > > > (The quote is taken from this message by Eric Dumazet: > > https://lore.kernel.org/netfilter-devel/a9006cf7-f4ba-81b1-fca1-fd2e97939fdc@gmail.com/ > > ) > > I see. I don't understand the value of a short-cut fast path if we > start chipping away at its characteristic feature. The point is to filter traffic coming in through af_packet. Exempting PACKET_QDISC_BYPASS from filtering would open up a trivial security hole. Thanks, Lukas
On Sat, Jan 30, 2021 at 11:26 AM Lukas Wunner <lukas@wunner.de> wrote: > > On Sun, Jan 24, 2021 at 11:18:00AM -0500, Willem de Bruijn wrote: > > On Sun, Jan 24, 2021 at 6:14 AM Lukas Wunner <lukas@wunner.de> wrote: > > > On Fri, Jan 22, 2021 at 11:13:19AM -0500, Willem de Bruijn wrote: > > > > On Fri, Jan 22, 2021 at 4:44 AM Lukas Wunner <lukas@wunner.de> wrote: > > > > > Add egress hook for AF_PACKET sockets that have the PACKET_QDISC_BYPASS > > > > > socket option set to on, which allows packets to escape without being > > > > > filtered in the egress path. > > > > > > > > > > This patch only updates the AF_PACKET path, it does not update > > > > > dev_direct_xmit() so the XDP infrastructure has a chance to bypass > > > > > Netfilter. > > > > > > > > Isn't the point of PACKET_QDISC_BYPASS to skip steps like this? > > > > > > I suppose PACKET_QDISC_BYPASS "was introduced to bypass qdisc, > > > not to bypass everything." > > > > > > (The quote is taken from this message by Eric Dumazet: > > > https://lore.kernel.org/netfilter-devel/a9006cf7-f4ba-81b1-fca1-fd2e97939fdc@gmail.com/ > > > ) > > > > I see. I don't understand the value of a short-cut fast path if we > > start chipping away at its characteristic feature. > > The point is to filter traffic coming in through af_packet. > Exempting PACKET_QDISC_BYPASS from filtering would open up a > trivial security hole. Sure. But that argument is no different for TC_EGRESS. That's why packet sockets require CAP_NET_RAW. It is perhaps unfortunately that it is ns_capable instead of capable. But there is nothing netfilter specific about this.
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 6bbc7a448593..6dca6ead1162 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -89,6 +89,7 @@ #endif #include <linux/bpf.h> #include <net/compat.h> +#include <linux/netfilter_netdev.h> #include "internal.h" @@ -239,8 +240,42 @@ struct packet_skb_cb { static void __fanout_unlink(struct sock *sk, struct packet_sock *po); static void __fanout_link(struct sock *sk, struct packet_sock *po); +#ifdef CONFIG_NETFILTER_EGRESS +static noinline struct sk_buff *nf_hook_direct_egress(struct sk_buff *skb) +{ + struct sk_buff *next, *head = NULL, *tail; + int rc; + + rcu_read_lock(); + for (; skb != NULL; skb = next) { + next = skb->next; + skb_mark_not_on_list(skb); + + if (!nf_hook_egress(skb, &rc, skb->dev)) + continue; + + if (!head) + head = skb; + else + tail->next = skb; + + tail = skb; + } + rcu_read_unlock(); + + return head; +} +#endif + static int packet_direct_xmit(struct sk_buff *skb) { +#ifdef CONFIG_NETFILTER_EGRESS + if (nf_hook_egress_active()) { + skb = nf_hook_direct_egress(skb); + if (!skb) + return NET_XMIT_DROP; + } +#endif return dev_direct_xmit(skb, packet_pick_tx_queue(skb)); }