Remarks inline. Mostly ACK.
I’ll post an updated version soon. (I’ve also added a check for UDP in
dco_check_option_conflict_ce().
On 10 Aug 2022, at 18:32, Gert Doering wrote:
> On Mon, Aug 08, 2022 at 04:34:23PM +0200, Kristof Provost via Openvpn-devel
> wrote:
>> diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
>> new file mode 100644
>> index 00000000..a8a53fe3
>> --- /dev/null
>> +++ b/src/openvpn/dco_freebsd.c
>> @@ -0,0 +1,636 @@
> [..]
>> +static nvlist_t *
>> +sockaddr_to_nvlist(const struct sockaddr *sa)
>> +{
> [..]
>> + default:
>> + abort();
>
> please use "ASSERT(0);" here. It will do the same thing, but if ever
> hit, it will print a __FILE__, __LINE__ message to the log, so it's
> easier for us to see *where* it triggered a "this must never happen"
> condition.
>
Fixed.
>> +int
>> +dco_new_peer(dco_context_t *dco, unsigned int peerid, int sd,
>> + struct sockaddr *localaddr, struct sockaddr *remoteaddr,
>> + struct in_addr *remote_in4, struct in6_addr *remote_in6)
>> +{
>> + struct ifdrv drv;
>> + nvlist_t *nvl;
>> + int ret;
>> +
>> + nvl = nvlist_create(0);
>
> We use C99 these days, so this could be written as
>
>> + nvlist_t *nvl = nvlist_create(0);
>
> but this is a matter of personal preference. Both are fine, just wanted
> to point out that the option exists.
>
That’s FreeBSD’s style(9) showing here. If it’s acceptable to OpenVPN I’m just
going to keep it as-is.
>> +
>> + nvlist_add_number(nvl, "fd", sd);
>> + nvlist_add_number(nvl, "peerid", peerid);
>> +
>> + bzero(&drv, sizeof(drv));
>> + snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
>> + drv.ifd_cmd = OVPN_NEW_PEER;
>> + drv.ifd_data = nvlist_pack(nvl, &drv.ifd_len);
>> +
>> + ret = ioctl(dco->fd, SIOCSDRVSPEC, &drv);
>> + free(drv.ifd_data);
>
> What happens should nvlist_pack() fail here? Manpage says it returns
> NULL, will the ioctl() handle this gracefully, or will something crash?
>
That’d really only happen if a memory allocation fails, and given modern OS’s
overcommit behaviour that’s also not going to happen. (If you manage to run the
system out of memory malloc() will still give you virtual memory, but the OOM
killer will come to visit when you get around to actually using it.)
> (If the ioctl() returns something like EINVAL in this case, perfectly fine)
>
The copyin() in the kernel will fail, and that’d return an error, without
crashing things.
So, yes, that’s what’d happen.
>> + nvlist_destroy(nvl);
>> + if (ret)
>> + {
>> + msg(D_DCO, "Failed to create new peer %d", errno);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>
> Most functions in OpenVPN that do use a "ret" variable would look
> more like this:
>
>> + if (ret)
>> + {
>> + msg(D_DCO, "Failed to create new peer %d", errno);
>> + }
>> +
>> + return ret;
>> +}
>
> we do have all sorts of variants (*sigh*), but since you also used both
> versions, let's go for this one.
>
Fixed.
>> + msg(D_DCO, "Failed to create new peer %d", errno);
>
> this can be written as
>
>> + msg(D_DCO|M_ERRNO, "Failed to create new peer");
>
> M_ERRNO will automatically append the strerror(errno) stuff.
>
Oh neat. I’ve made that change.
> D_DCO is "--verb 3", so that message won't be visible in normal operation
> - if we consider this an error, then it should be
>
>> + msg(M_WARN|M_ERRNO, "Failed to create new peer");
>
> instead (or if it's "fatal error, give up" -> M_ERR).
>
These are generally fatal errors, yes.
There are follow-up error messages from the calling code that do get logged,
but they’re usually less clear about what the actual problem is. I’ll see about
changing D_DCO -> M_ERR or M_WARN.
> Now, I'm not sure if accessing errno after the free() call is guaranteed
> to be save - so maybe move the msg() call up?
>
I’m pretty sure free() doesn’t mess with errno (as in, I’ve looked at the
jemalloc code), but it’s better to not have to check.
Happily, thanks to your earlier suggestion it’s much easier to just move the
free after the log, so I’ll do that.
>> +bool
>> +ovpn_dco_init(int mode, dco_context_t *dco)
>> +{
>> + if (open_fd(dco) < 0)
>> + {
>> + msg(D_DCO, "Failed to open socket");
>
> Same here - if you want this to be heard, D_DCO should be M_WARN|M_ERRNO.
>
> If this is just informational, because the caller will make sufficient
> noise, D_DCO|M_ERRNO is good (but you might want to point out that this
> is the "DCO socket" that couldn't be opened).
>
Yeah, Iv’e gone through and made them all a bit louder. It’s all failure
conditions, so being overly verbose there isn’t really an issue.
>> +static int
>> +create_interface(struct tuntap *tt, const char *dev)
>> +{
>> + int ret;
>> + struct ifreq ifr;
>> +
>> + bzero(&ifr, sizeof(ifr));
>
> There is a convenience macro here, CLEAR(ifr) (which does exactly
> this - #define CLEAR(x) memset(&(x), 0, sizeof(x)).
>
Neat. Changed.
>> + /* Create ovpnx first, then rename it. */
>> + snprintf(ifr.ifr_name, IFNAMSIZ, "ovpn");
>> + ret = ioctl(tt->dco.fd, SIOCIFCREATE2, &ifr);
>> + if (ret)
>> + {
>> + msg(D_DCO, "Failed to create interface %s: %d", ifr.ifr_name,
>> errno);
>> + return ret;
>> + }
>
> -> M_ERRNO
Done.
>
>> + /* Rename */
>> + if (!strcmp(dev, "tun"))
>> + {
>> + ifr.ifr_data = "ovpn";
>> + }
>> + else
>> + {
>> + ifr.ifr_data = (char *)dev;
>> + }
>
> I'm not sure I understand this code part. When would dev be "tun"
> here, triggering a rename to "ovpn"?
>
> The call chain leading to this is (now)
>
> - tun.c:open_tun_dco_generic()
> - open_tun_dco()
> - create_interface(tt,dev)
>
> and tun_dco_generic() should never pass in a bare "tun" name (because
> in that case, it would iterate "tun0, tun1, tun2").
>
> Is this a safeguard against "bad things will happen in kernel land if
> we use the unqualified name of another driver"?
>
No, it’s that OpenVPN lets you pick the interface name, and FreeBSD figures out
which driver it needs to call based on the interface name.
So as long as users decide they want the interface to be ovpnX it’ll be fine,
but if they want ‘vpn0’ or even something silly like ‘vlan1’ the kernel isn’t
going to create an if_ovpn interface.
So we create opvnX first, and then rename it to whatever the user wants it to
be.
>> + ret = ioctl(tt->dco.fd, SIOCSIFNAME, &ifr);
>> + if (ret)
>> + {
>> + /* Delete the created interface again. */
>> + (void)ioctl(tt->dco.fd, SIOCIFDESTROY, &ifr);
>> + msg(D_DCO, "Failed to create interface %s: %d", ifr.ifr_data,
>> errno);
>
> -> M_ERRNO
>
> (there's more, but no need to point them out individually :-) )
>
Fixed.
>> +static int
>> +remove_interface(struct tuntap *tt)
>> +{
>> + int ret;
>> + struct ifreq ifr;
>> +
>> + bzero(&ifr, sizeof(ifr));
>> + snprintf(ifr.ifr_name, IFNAMSIZ, "%s", tt->dco.ifname);
>> +
>> + ret = ioctl(tt->dco.fd, SIOCIFDESTROY, &ifr);
>> + if (ret)
>> + {
>> + msg(D_DCO, "Failed to remove interface %s: %d", ifr.ifr_name,
>> errno);
>> + return ret;
>> + }
>> +
>> + tt->dco.ifname[0] = 0;
>> +
>> + return 0;
>> +}
>
> Maybe always clear the ifname, and do "return ret;" here too?
>
Good idea.
>> +static int
>> +start_tun(dco_context_t *dco)
>> +{
>> + struct ifdrv drv;
>> + int ret;
>> +
>> + bzero(&drv, sizeof(drv));
>
> CLEAR(drv);
>
Fixed.
>> +int
>> +dco_do_read(dco_context_t *dco)
>> +{
>> + struct ifdrv drv;
>> + uint8_t buf[4096];
>> + nvlist_t *nvl;
>> + const uint8_t *pkt;
>> + size_t pktlen;
>> + int ret;
>> +
>> + /* Flush any pending data from the pipe. */
>> + (void)read(dco->pipefd[1], buf, sizeof(buf));
>> +
>> + bzero(&drv, sizeof(drv));
>> + snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
>> + drv.ifd_cmd = OVPN_GET_PKT;
>> + drv.ifd_data = buf;
>> + drv.ifd_len = sizeof(buf);
>> +
>> + ret = ioctl(dco->fd, SIOCGDRVSPEC, &drv);
>> + if (ret)
>> + {
>> + msg(D_DCO, "Failed to read control packet %d", errno);
>> + return errno;
>
> Documentation (dco.h) says "0 on success or a negative error code otherwise",
> so this needs to be "return -errno;" to behave the same as dco_linux
>
Fixed.
>> + }
>> +
>> + nvl = nvlist_unpack(buf, drv.ifd_len, 0);
>> + if (!nvl)
>> + {
>> + msg(D_DCO, "Failed to unpack nvlist");
>> + return EINVAL;
>
> -EINVAL
>
Fixed.
>> +int
>> +dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf)
>> +{
>> + struct ifdrv drv;
>> + nvlist_t *nvl;
>> + int ret;
>> +
>> + nvl = nvlist_create(0);
>> +
>> + nvlist_add_binary(nvl, "packet", BSTR(buf), BLEN(buf));
>> + nvlist_add_number(nvl, "peerid", peer_id);
>> +
>> + bzero(&drv, sizeof(drv));
>> + snprintf(drv.ifd_name, IFNAMSIZ, "%s", dco->ifname);
>> + drv.ifd_cmd = OVPN_SEND_PKT;
>> + drv.ifd_data = nvlist_pack(nvl, &drv.ifd_len);
>> +
>> + ret = ioctl(dco->fd, SIOCSDRVSPEC, &drv);
>> + free(drv.ifd_data);
>> + nvlist_destroy(nvl);
>> + if (ret)
>> + {
>> + msg(D_DCO, "Failed to send control packet %d", errno);
>> + return ret;
>> + }
>> +
>> + return BLEN(buf);
>> +}
>
> The return code of dco_do_write() is not clearly defined in dco.h
> (Antonio, looking at you...!) but the linux version seems to do
> "negative is error, positive is buffer length written", so this should
> be fine. Might need a change to "return -errno".
>
Makes sense. Done.
>> +bool
>> +dco_available(int msglevel)
>> +{
>> + struct if_clonereq ifcr;
>> + char *buf = NULL;
>> + int fd;
>> + int ret;
>> + bool available = false;
>> +
>> + /* Attempt to load the module. Ignore errors, because it might already
>> be
>> + * loaded, or built into the kernel. */
>> + (void)kldload("if_ovpn");
>> +
>> + fd = socket(AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
>> + if (fd < 0)
>> + {
>> + return false;
>> + }
>> +
>> + bzero(&ifcr, sizeof(ifcr));
>
> CLEAR(ifcr)
>
Fixed.
>> + ret = ioctl(fd, SIOCIFGCLONERS, &ifcr);
>> + if (ret != 0)
>> + {
>> + goto out;
>> + }
>> +
>> + buf = malloc(ifcr.ifcr_total * IFNAMSIZ);
>> +
>> + ifcr.ifcr_count = ifcr.ifcr_total;
>> + ifcr.ifcr_buffer = buf;
>> + ret = ioctl(fd, SIOCIFGCLONERS, &ifcr);
>> + if (ret != 0)
>> + {
>> + goto out;
>> + }
>> +
>> + for (int i = 0; i < ifcr.ifcr_total; i++)
>> + {
>> + if (strcmp(buf + (i * IFNAMSIZ), "openvpn") == 0)
>
> This looks fairly magic to me - am I reading this right that the first
> call returns a number of "somethings", then we malloc(something * IFNAMSIZ),
> and the second call returns a list of strings that describe "something",
> and if there is one of them named "openvpn", we know that DCO is
> available?
>
Correct. The first call is to figure out how much space we need, we then
allocate that and get the list.
> Why is it called "openvpn", not "ovpn" (if_ovpn, man ovpn)?
>
That’s what the driver chose to do. It’s a bit cheap of me to blame the driver
author, but he’s an ass anyway ;)
More seriously, that’s mostly because that also ensures that the interfaces are
automatically in the ‘openvpn’ netif group, which is something that makes
pfsense’s life easier.
> (A few lines of comment on what SIOCIFGCLONERS does - or which manpage
> to look at - would be appreciated here :-) )
>
It’s a BSD-ism. Some types of interfaces (essentially any interface that’s not
actual hardware) is created through ‘cloning’. The relevant code is in
sys/net/if_clone.c in the kernel.
You can list them with “ifconfig -C”.
We use it as a way to figure out if DCO is supported or not. We could
potentially also enumerate the loaded kernel modules and check for if_ovpn.ko,
but that’d break if someone built that into the kernel.
>> +const char *
>> +dco_get_supported_ciphers()
>> +{
>> + return "none:AES-256-GCM:AES-128-GCM:CHACHA20-POLY1305";
>> +}
>
> Is "none" indeed supported? I find that useful to test, but Arne and
> Antonio refuse(d) to support it in linux DCO :-)
>
It is, yes, although I have to admit that it’s certainly the least tested mode.
(Because if you really want to tunnel without crypto you can just use if_gif,
without needing the OpenVPN userspace code.)
>> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
>> index 55c939c4..14ad24fa 100644
>> --- a/src/openvpn/forward.c
>> +++ b/src/openvpn/forward.c
>
> ... these are all very straightforward "LINUX" -> "LINUX || FREEBSD" :-)
>
If at some point someone adds OpenBSD or NetBSD or Illumos or .. it’s going to
be worth thinking about a ‘PLATFORM_HAS_DCO’, or ‘UNIX_Y_HAS_DCO’ or similar
define for these.
>> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
>> index 54f7d72c..e84d5a27 100644
>> --- a/src/openvpn/tun.c
>> +++ b/src/openvpn/tun.c
> [..]
>> @@ -2917,20 +2917,24 @@ void
>> open_tun(const char *dev, const char *dev_type, const char *dev_node,
>> struct tuntap *tt,
>> openvpn_net_ctx_t *ctx)
>> {
>> - open_tun_generic(dev, dev_type, dev_node, tt);
>> -
>> - if (tt->fd >= 0 && tt->type == DEV_TYPE_TUN)
>> - {
>> - int i = IFF_POINTOPOINT | IFF_MULTICAST;
>> + if (tun_dco_enabled(tt)) {
>> + open_tun_dco_generic(dev, dev_type, tt, ctx);
>> + } else {
>> + open_tun_generic(dev, dev_type, dev_node, tt);
>
> This one will upset the whitespace dragon and it will eat all of
> the patch :-)
>
> OpenVPN bracket convention requires this to be:
>
> + if (tun_dco_enabled(tt))
> + {
> + open_tun_dco_generic(dev, dev_type, tt, ctx);
> + }
> + else
> + {
> + open_tun_generic(dev, dev_type, dev_node, tt);
>
> (I run uncrustify on merge, so this would get fixed - but it would
> be good to have it properly formatted right away)
>
Whoops. I thought I’d been good about following local style, but I seem to have
missed one. Fixed.
Best regards,
Kristof
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
