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
Oh, I meant to ask earlier: if you've got some easy repro code for this, that'd save a bit of trouble. 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
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.