| Message ID | 20220812130657.29899-8-a@unstable.cc |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ovpn-dco: leftovers | expand |
Hi, On Fri, Aug 12, 2022 at 03:06:54PM +0200, Antonio Quartulli wrote: > On Windows the high level API should still use the link_socket object to > read and write packets. For this reason, even if dco_installed is true, > we still need to rely on the classic link_socket object. > > Signed-off-by: Antonio Quartulli <a@unstable.cc> > --- > src/openvpn/dco_win.c | 4 ++-- > src/openvpn/forward.c | 23 ++++++++++++++++++++++- > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c > index f1160c7d..18ce9f3a 100644 > --- a/src/openvpn/dco_win.c > +++ b/src/openvpn/dco_win.c > @@ -355,14 +355,14 @@ dco_available(int msglevel) > int > dco_do_read(dco_context_t *dco) > { > - /* no-op on windows */ > + ASSERT(false); > return 0; > } I think this ASSERT(0) should go into the patch that introduces dco_win.c - this is just a few patches upstream, so introducing it and then changing it right again sounds like a bit of needless commit noise... > diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c > index 650f7c59..8af41072 100644 > --- a/src/openvpn/forward.c > +++ b/src/openvpn/forward.c > @@ -1601,6 +1601,27 @@ process_ip_header(struct context *c, unsigned int flags, struct buffer *buf) > } > } > > +/* Depending on the platform, we may have to not use the DCO socket, even if DCO > + * is being used for a specific link. > + * > + * This happens with Windows, where the standard link_socket API have to be used > + * also with DCO. > + * > + * For this reason we must make the right decision and not always look at > + * dco_installed. Note that on Windows the dco_installed field is still supposed > + * to be true, because it is used in the lower level code to use the proper API > + * (socket vs handle). This is why we need this function with some ifdef sauce > + */ This comment could use a bit of rewording, like /* Linux-like DCO implementations pass the socket to the kernel and * disallow usage of it from userland, so (control) packets sent and * received by OpenVPN need to go through the DCO interface. * * Windows DCO needs the control packets to be sent via the normal * Socket API. * * Hide that complexity (... especially if more platforms show up * in future...) in a small inline function */ > +static bool > +should_use_dco_socket(struct link_socket *sock) > +{ > +#if defined(TARGET_LINUX) > + return sock->info.dco_installed; > +#else > + return false; > +#endif > +} ... which really should be "inline", no? ;-) **WARNING** - this breaks FreeBSD DCO, so with FreeBSD DCO merged now, it needs to be #if defined(TARGET_LINUX) || defined(TARGET_FREEBSD). gert
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c index f1160c7d..18ce9f3a 100644 --- a/src/openvpn/dco_win.c +++ b/src/openvpn/dco_win.c @@ -355,14 +355,14 @@ dco_available(int msglevel) int dco_do_read(dco_context_t *dco) { - /* no-op on windows */ + ASSERT(false); return 0; } int dco_do_write(dco_context_t *dco, int peer_id, struct buffer *buf) { - /* no-op on windows */ + ASSERT(false); return 0; } diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 650f7c59..8af41072 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -1601,6 +1601,27 @@ process_ip_header(struct context *c, unsigned int flags, struct buffer *buf) } } +/* Depending on the platform, we may have to not use the DCO socket, even if DCO + * is being used for a specific link. + * + * This happens with Windows, where the standard link_socket API have to be used + * also with DCO. + * + * For this reason we must make the right decision and not always look at + * dco_installed. Note that on Windows the dco_installed field is still supposed + * to be true, because it is used in the lower level code to use the proper API + * (socket vs handle). This is why we need this function with some ifdef sauce + */ +static bool +should_use_dco_socket(struct link_socket *sock) +{ +#if defined(TARGET_LINUX) + return sock->info.dco_installed; +#else + return false; +#endif +} + /* * Input: c->c2.to_link */ @@ -1674,7 +1695,7 @@ process_outgoing_link(struct context *c) socks_preprocess_outgoing_link(c, &to_addr, &size_delta); /* Send packet */ - if (c->c2.link_socket->info.dco_installed) + if (should_use_dco_socket(c->c2.link_socket)) { size = dco_do_write(&c->c1.tuntap->dco, c->c2.tls_multi->peer_id,
On Windows the high level API should still use the link_socket object to read and write packets. For this reason, even if dco_installed is true, we still need to rely on the classic link_socket object. Signed-off-by: Antonio Quartulli <a@unstable.cc> --- src/openvpn/dco_win.c | 4 ++-- src/openvpn/forward.c | 23 ++++++++++++++++++++++- 2 files changed, 24 insertions(+), 3 deletions(-)