Development discussion of WireGuard
 help / color / mirror / Atom feed
* Re: wireguard: problem sending via libpcap's packet socket
       [not found] <20200626201330.325840-1-ndev@hwipl.net>
@ 2020-06-26 20:41 ` Jason A. Donenfeld
  2020-06-26 20:42   ` Jason A. Donenfeld
  2020-06-27  0:22   ` Jason A. Donenfeld
  0 siblings, 2 replies; 8+ messages in thread
From: Jason A. Donenfeld @ 2020-06-26 20:41 UTC (permalink / raw)
  To: Hans Wippel; +Cc: WireGuard mailing list, Netdev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: wireguard: problem sending via libpcap's packet socket
  2020-06-26 20:41 ` wireguard: problem sending via libpcap's packet socket Jason A. Donenfeld
@ 2020-06-26 20:42   ` Jason A. Donenfeld
  2020-06-27  0:22   ` Jason A. Donenfeld
  1 sibling, 0 replies; 8+ messages in thread
From: Jason A. Donenfeld @ 2020-06-26 20:42 UTC (permalink / raw)
  To: Hans Wippel; +Cc: WireGuard mailing list

Oh, I meant to ask earlier: if you've got some easy repro code for
this, that'd save a bit of trouble.

Jason

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: wireguard: problem sending via libpcap's packet socket
  2020-06-26 20:41 ` wireguard: problem sending via libpcap's packet socket Jason A. Donenfeld
  2020-06-26 20:42   ` Jason A. Donenfeld
@ 2020-06-27  0:22   ` Jason A. Donenfeld
  2020-06-27  5:58     ` Jason A. Donenfeld
  1 sibling, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2020-06-27  0:22 UTC (permalink / raw)
  To: Hans Wippel; +Cc: WireGuard mailing list, Netdev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: wireguard: problem sending via libpcap's packet socket
  2020-06-27  0:22   ` Jason A. Donenfeld
@ 2020-06-27  5:58     ` Jason A. Donenfeld
  2020-06-28 20:04       ` Willem de Bruijn
  0 siblings, 1 reply; 8+ messages in thread
From: Jason A. Donenfeld @ 2020-06-27  5:58 UTC (permalink / raw)
  To: Hans Wippel; +Cc: WireGuard mailing list, Netdev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: wireguard: problem sending via libpcap's packet socket
  2020-06-27  5:58     ` Jason A. Donenfeld
@ 2020-06-28 20:04       ` Willem de Bruijn
  2020-07-01  3:05         ` Jason A. Donenfeld
  0 siblings, 1 reply; 8+ messages in thread
From: Willem de Bruijn @ 2020-06-28 20:04 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: Hans Wippel, WireGuard mailing list, Netdev

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: wireguard: problem sending via libpcap's packet socket
  2020-06-28 20:04       ` Willem de Bruijn
@ 2020-07-01  3:05         ` Jason A. Donenfeld
  2020-07-01 12:19           ` Hans Wippel
  2020-07-01 16:28           ` Willem de Bruijn
  0 siblings, 2 replies; 8+ messages in thread
From: Jason A. Donenfeld @ 2020-07-01  3:05 UTC (permalink / raw)
  To: Willem de Bruijn; +Cc: Hans Wippel, WireGuard mailing list, Netdev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: wireguard: problem sending via libpcap's packet socket
  2020-07-01  3:05         ` Jason A. Donenfeld
@ 2020-07-01 12:19           ` Hans Wippel
  2020-07-01 16:28           ` Willem de Bruijn
  1 sibling, 0 replies; 8+ messages in thread
From: Hans Wippel @ 2020-07-01 12:19 UTC (permalink / raw)
  To: Jason A. Donenfeld
  Cc: Willem de Bruijn, Hans Wippel, WireGuard mailing list, Netdev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: wireguard: problem sending via libpcap's packet socket
  2020-07-01  3:05         ` Jason A. Donenfeld
  2020-07-01 12:19           ` Hans Wippel
@ 2020-07-01 16:28           ` Willem de Bruijn
  1 sibling, 0 replies; 8+ messages in thread
From: Willem de Bruijn @ 2020-07-01 16:28 UTC (permalink / raw)
  To: Jason A. Donenfeld; +Cc: Hans Wippel, WireGuard mailing list, Netdev

> > 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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-07-13 23:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200626201330.325840-1-ndev@hwipl.net>
2020-06-26 20:41 ` wireguard: problem sending via libpcap's packet socket Jason A. Donenfeld
2020-06-26 20:42   ` Jason A. Donenfeld
2020-06-27  0:22   ` Jason A. Donenfeld
2020-06-27  5:58     ` Jason A. Donenfeld
2020-06-28 20:04       ` Willem de Bruijn
2020-07-01  3:05         ` Jason A. Donenfeld
2020-07-01 12:19           ` Hans Wippel
2020-07-01 16:28           ` Willem de Bruijn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).