| Message ID | 20200626201330.325840-1-ndev@hwipl.net |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | wireguard: problem sending via libpcap's packet socket | expand |
Hi Hans, On Fri, Jun 26, 2020 at 2:14 PM Hans Wippel <ndev@hwipl.net> wrote: > while toying around with sending packets with libpcap, I noticed that it > did not work with a wireguard interface in contrast to my regular > ethernet interface. Thanks for letting me know. I'll try to repro and will look if this is common behavior for all virtual drivers, or simply a bug in WireGuard that I need to address. If it is the latter, your patch below isn't quite correct; we'll probably address this instead by simply setting skb->protocol in xmit by peaking at the header, if skb->protocol is zero, and then keeping the rest of the logic the same elsewhere. Jason
Hi Hans,
Your test program appears to be doing:
socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL)) = 3
sendto(3, "E\0\0+\0\0@\0@\21\267o\300\250\1\1\300\250\1\1\4\322\4\322\0\0272\221\1\2\3\4"...,
43, 0, NULL, 0) = 43
This means we're calling into af_packet's packet_sendmsg->packet_snd,
which appears to call packet_parse_headers(skb, sock):
static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
{
if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) &&
sock->type == SOCK_RAW) {
skb_reset_mac_header(skb);
skb->protocol = dev_parse_header_protocol(skb);
}
skb_probe_transport_header(skb);
}
So the question is, why isn't skb->protocol set on the packet that
makes it to wg_xmit?
Adding some printks, it looks like the result of:
pr_err("SARU %s:%d\n", __FILE__, __LINE__);
skb_reset_mac_header(skb);
skb->protocol = dev_parse_header_protocol(skb);
pr_err("%d\n", skb->protocol);
is:
[ 0.430754] SARU net/packet/af_packet.c:1864
[ 0.431454] 0
So digging a bit further, dev_parse_header_protocol:
static inline __be16 dev_parse_header_protocol(const struct sk_buff *skb)
{
const struct net_device *dev = skb->dev;
if (!dev->header_ops || !dev->header_ops->parse_protocol)
return 0;
return dev->header_ops->parse_protocol(skb);
}
Apparently the issue is that wireguard doesn't implement any
header_ops. I fixed that in this commit here:
https://git.zx2c4.com/wireguard-linux/commit/?id=73b20c384a8bc498c6b8950672003410ed6016da
In my tests, that commit appears to fix the problem exposed by your
test case. I'll probably wait a few days to think about this some more
and make sure this is correct before submitting, but it seems likely
that this will take care of the issue.
Thanks for the report and easy test case!
Jason
Hi again Hans, A few remarks: although gre implements header_ops, it looks like various parts of the networking stack change behavior based on it. I'm still analyzing that to understand the extent of the effects. Something like <https://git.zx2c4.com/wireguard-linux/commit/?id=40c24fd379edc1668087111506ed3d0928052fe0> would work, but I'm not thrilled by it. Further research is needed. However, one thing I noticed is that other layer 3 tunnels don't seem to be a fan of libpcap. For example, try injecting a packet into an ipip interface. You'll hit exactly the same snag for skb->protocol==0. So, if I do go the route of the first option -- adding a header_ops -- maybe I'll be inclined to make a shared l3_header_ops struct that can be shared between things, and fix up all of these at once. Alternatively, it might turn out to be that, because this is broken for other layer 3 devices, it's meant to be broken here. But I hope that won't be the conclusion. Jason
Hi Jason, thanks for quick replies and patches! For whatever it's worth, I have attached another simple test program to this mail. It opens AF_PACKET sockets without libpcap and (incorrectly) sets the protocol to 0. It works with my ethernet device. Looking at the code, it should also work with the patches you posted. Maybe it is useful for testing some (stupid) corner case. Hans
On Sat, Jun 27, 2020 at 1:58 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi again Hans, > > A few remarks: although gre implements header_ops, it looks like > various parts of the networking stack change behavior based on it. I'm > still analyzing that to understand the extent of the effects. > Something like <https://git.zx2c4.com/wireguard-linux/commit/?id=40c24fd379edc1668087111506ed3d0928052fe0> > would work, but I'm not thrilled by it. Further research is needed. > > However, one thing I noticed is that other layer 3 tunnels don't seem > to be a fan of libpcap. For example, try injecting a packet into an > ipip interface. You'll hit exactly the same snag for skb->protocol==0. Not setting skb protocol when sending over packet sockets causes many headaches. Besides packet_parse_headers, virtio_net_hdr_to_skb also tries to infer it. Packet sockets give various options to configure it explicitly: by choosing that protocol in socket(), bind() or, preferably, by passing it as argument to sendmsg. The socket/bind argument also configures the filter to receive packets, so for send-only sockets it is especially useful to choose ETH_P_NONE (0) there. This is not an "incorrect" option. Libpcap does have a pcap_set_protocol function, but it is fairly recent, so few processes will likely be using it. And again it is still not ideal if a socket is opened only for transmit. header_ops looks like the best approach to me, too. The protocol field needs to reflect the protocol of the *outer* packet, of course, but if I read wg_allowedips_lookup_dst correctly, wireguard maintains the same outer protocol as the inner protocol, no sit (6-in-4) and such.
On Sun, Jun 28, 2020 at 2:04 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Sat, Jun 27, 2020 at 1:58 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > Hi again Hans, > > > > A few remarks: although gre implements header_ops, it looks like > > various parts of the networking stack change behavior based on it. I'm > > still analyzing that to understand the extent of the effects. > > Something like <https://git.zx2c4.com/wireguard-linux/commit/?id=40c24fd379edc1668087111506ed3d0928052fe0> > > would work, but I'm not thrilled by it. Further research is needed. > > > > However, one thing I noticed is that other layer 3 tunnels don't seem > > to be a fan of libpcap. For example, try injecting a packet into an > > ipip interface. You'll hit exactly the same snag for skb->protocol==0. > > Not setting skb protocol when sending over packet sockets causes many > headaches. Besides packet_parse_headers, virtio_net_hdr_to_skb also > tries to infer it. > > Packet sockets give various options to configure it explicitly: by > choosing that protocol in socket(), bind() or, preferably, by passing > it as argument to sendmsg. The socket/bind argument also configures > the filter to receive packets, so for send-only sockets it is > especially useful to choose ETH_P_NONE (0) there. This is not an > "incorrect" option. > > Libpcap does have a pcap_set_protocol function, but it is fairly > recent, so few processes will likely be using it. And again it is > still not ideal if a socket is opened only for transmit. > > header_ops looks like the best approach to me, too. The protocol field > needs to reflect the protocol of the *outer* packet, of course, but if > I read wg_allowedips_lookup_dst correctly, wireguard maintains the > same outer protocol as the inner protocol, no sit (6-in-4) and such. WireGuard does allow 6-in-4 and 4-in-6 actually. But parse_protocol is only ever called on the inner packet. The only code paths leading to it are af_packet-->ndo_start_xmit, and ndo_start_xmit examines skb->protocol of that inner packet, which means it entirely concerns the inner packet. And generally, for wireguard, userspace only ever deals with the inner packet. That inner packet then gets encrypted and poked at in strange ways, and then the encrypted blob of sludge gets put into a udp packet and sent some place. So I'm quite sure that the behavior just committed is right. And from writing a few libpcap examples, things seem to be working very well, including Hans' example. Hans - if you want to try out davem's net.git tree, you can see if this is working properly for you. Jason
On Tue, 30 Jun 2020 21:05:27 -0600 "Jason A. Donenfeld" <Jason@zx2c4.com> wrote: > On Sun, Jun 28, 2020 at 2:04 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > On Sat, Jun 27, 2020 at 1:58 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > Hi again Hans, > > > > > > A few remarks: although gre implements header_ops, it looks like > > > various parts of the networking stack change behavior based on it. I'm > > > still analyzing that to understand the extent of the effects. > > > Something like <https://git.zx2c4.com/wireguard-linux/commit/?id=40c24fd379edc1668087111506ed3d0928052fe0> > > > would work, but I'm not thrilled by it. Further research is needed. > > > > > > However, one thing I noticed is that other layer 3 tunnels don't seem > > > to be a fan of libpcap. For example, try injecting a packet into an > > > ipip interface. You'll hit exactly the same snag for skb->protocol==0. > > > > Not setting skb protocol when sending over packet sockets causes many > > headaches. Besides packet_parse_headers, virtio_net_hdr_to_skb also > > tries to infer it. > > > > Packet sockets give various options to configure it explicitly: by > > choosing that protocol in socket(), bind() or, preferably, by passing > > it as argument to sendmsg. The socket/bind argument also configures > > the filter to receive packets, so for send-only sockets it is > > especially useful to choose ETH_P_NONE (0) there. This is not an > > "incorrect" option. > > > > Libpcap does have a pcap_set_protocol function, but it is fairly > > recent, so few processes will likely be using it. And again it is > > still not ideal if a socket is opened only for transmit. > > > > header_ops looks like the best approach to me, too. The protocol field > > needs to reflect the protocol of the *outer* packet, of course, but if > > I read wg_allowedips_lookup_dst correctly, wireguard maintains the > > same outer protocol as the inner protocol, no sit (6-in-4) and such. > > WireGuard does allow 6-in-4 and 4-in-6 actually. But parse_protocol is > only ever called on the inner packet. The only code paths leading to > it are af_packet-->ndo_start_xmit, and ndo_start_xmit examines > skb->protocol of that inner packet, which means it entirely concerns > the inner packet. And generally, for wireguard, userspace only ever > deals with the inner packet. That inner packet then gets encrypted and > poked at in strange ways, and then the encrypted blob of sludge gets > put into a udp packet and sent some place. So I'm quite sure that the > behavior just committed is right. > > And from writing a few libpcap examples, things seem to be working > very well, including Hans' example. > > Hans - if you want to try out davem's net.git tree, you can see if > this is working properly for you. I just tested it and everything seems to work now. Thanks :) Hans
> > header_ops looks like the best approach to me, too. The protocol field > > needs to reflect the protocol of the *outer* packet, of course, but if > > I read wg_allowedips_lookup_dst correctly, wireguard maintains the > > same outer protocol as the inner protocol, no sit (6-in-4) and such. > > WireGuard does allow 6-in-4 and 4-in-6 actually. But parse_protocol is > only ever called on the inner packet. The only code paths leading to > it are af_packet-->ndo_start_xmit, and ndo_start_xmit examines > skb->protocol of that inner packet, which means it entirely concerns > the inner packet. Of course, you are right. This inspects the packet before passing to the device ndo_start_xmit, so before any encapsulation would take place. > And generally, for wireguard, userspace only ever > deals with the inner packet. That inner packet then gets encrypted and > poked at in strange ways, and then the encrypted blob of sludge gets > put into a udp packet and sent some place. So I'm quite sure that the > behavior just committed is right. > > And from writing a few libpcap examples, things seem to be working > very well, including Hans' example. Definitely. Thanks again.
diff --git a/drivers/net/wireguard/allowedips.c b/drivers/net/wireguard/allowedips.c index 3725e9cd85f4..08e16907fde5 100644 --- a/drivers/net/wireguard/allowedips.c +++ b/drivers/net/wireguard/allowedips.c @@ -5,6 +5,7 @@ #include "allowedips.h" #include "peer.h" +#include "queueing.h" static void swap_endian(u8 *dst, const u8 *src, u8 bits) { @@ -356,9 +357,9 @@ int wg_allowedips_read_node(struct allowedips_node *node, u8 ip[16], u8 *cidr) struct wg_peer *wg_allowedips_lookup_dst(struct allowedips *table, struct sk_buff *skb) { - if (skb->protocol == htons(ETH_P_IP)) + if (wg_examine_packet_protocol(skb) == htons(ETH_P_IP)) return lookup(table->root4, 32, &ip_hdr(skb)->daddr); - else if (skb->protocol == htons(ETH_P_IPV6)) + else if (wg_examine_packet_protocol(skb) == htons(ETH_P_IPV6)) return lookup(table->root6, 128, &ipv6_hdr(skb)->daddr); return NULL; } diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c index a8f151b1b5fa..baed429b2ed1 100644 --- a/drivers/net/wireguard/device.c +++ b/drivers/net/wireguard/device.c @@ -132,10 +132,10 @@ static netdev_tx_t wg_xmit(struct sk_buff *skb, struct net_device *dev) peer = wg_allowedips_lookup_dst(&wg->peer_allowedips, skb); if (unlikely(!peer)) { ret = -ENOKEY; - if (skb->protocol == htons(ETH_P_IP)) + if (wg_examine_packet_protocol(skb) == htons(ETH_P_IP)) net_dbg_ratelimited("%s: No peer has allowed IPs matching %pI4\n", dev->name, &ip_hdr(skb)->daddr); - else if (skb->protocol == htons(ETH_P_IPV6)) + else if (wg_examine_packet_protocol(skb) == htons(ETH_P_IPV6)) net_dbg_ratelimited("%s: No peer has allowed IPs matching %pI6\n", dev->name, &ipv6_hdr(skb)->daddr); goto err; diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h index c58df439dbbe..36badcf8a11d 100644 --- a/drivers/net/wireguard/queueing.h +++ b/drivers/net/wireguard/queueing.h @@ -84,7 +84,8 @@ static inline __be16 wg_examine_packet_protocol(struct sk_buff *skb) static inline bool wg_check_packet_protocol(struct sk_buff *skb) { __be16 real_protocol = wg_examine_packet_protocol(skb); - return real_protocol && skb->protocol == real_protocol; + return real_protocol == htons(ETH_P_IP) || + real_protocol == htons(ETH_P_IPV6); } static inline void wg_reset_packet(struct sk_buff *skb, bool encapsulating)
Hi, while toying around with sending packets with libpcap, I noticed that it did not work with a wireguard interface in contrast to my regular ethernet interface. It seems like libpcap does not set a protocol on the packet socket and the following checks "skb->protocol == real_protocol", "skb->protocol == htons(ETH_P_IP)" and "skb->protocol == htons(ETH_P_IPV6)" in the functions wg_check_packet_protocol(), wg_xmit() and wg_allowedips_lookup_dst() fail because skb->protocol is 0. Is this intended behaviour? Should these checks be changed in the kernel or should this rather be reported to libpcap? Just in case, you can find changes that made it work for me below (probably not the best solution). Again, if this is not the right place to ask, just let me know and I am sorry for the noise. Hans Signed-off-by: Hans Wippel <ndev@hwipl.net> --- drivers/net/wireguard/allowedips.c | 5 +++-- drivers/net/wireguard/device.c | 4 ++-- drivers/net/wireguard/queueing.h | 3 ++- 3 files changed, 7 insertions(+), 5 deletions(-)