diff mbox series

[Openvpn-devel,v101,2/7] dco-win: check for incompatible options

Message ID 20220813204224.22576-2-a@unstable.cc
State Changes Requested
Headers
Series [Openvpn-devel,v101,1/7] dco-win: introduce low-level code for handling ovpn-dco-win in Windows |

Commit Message

Antonio Quartulli Aug. 13, 2022, 10:42 a.m. UTC 
  At the moment dco-win doesn't support --persist-tun and --server,
so check for these options at startup time.

Signed-off-by: Antonio Quartulli <a@unstable.cc>
Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
Changes from v100:
* improved commit title/message
---
 src/openvpn/dco.c     | 17 +++++++++++++++--
 src/openvpn/options.c |  5 +++++
 2 files changed, 20 insertions(+), 2 deletions(-)

Comments

Lev Stipakov Aug. 13, 2022, 11:34 a.m. UTC | #1 
Again, this cannot be tested yet.

Stared at the code and tested with follow-up commits, looks good and works
as expected.

Acked-by: Lev Stipakov <lstipakov@gmail.com>


la 13. elok. 2022 klo 23.43 Antonio Quartulli (a@unstable.cc) kirjoitti:

> At the moment dco-win doesn't support --persist-tun and --server,
> so check for these options at startup time.
>
> Signed-off-by: Antonio Quartulli <a@unstable.cc>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
> Changes from v100:
> * improved commit title/message
> ---
>  src/openvpn/dco.c     | 17 +++++++++++++++--
>  src/openvpn/options.c |  5 +++++
>  2 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
> index b342bee1..dcb570f9 100644
> --- a/src/openvpn/dco.c
> +++ b/src/openvpn/dco.c
> @@ -221,7 +221,20 @@ dco_update_keys(dco_context_t *dco, struct tls_multi
> *multi)
>  static bool
>  dco_check_option_conflict_platform(int msglevel, const struct options *o)
>  {
> -#if defined(TARGET_LINUX)
> +#if defined(_WIN32)
> +    if (o->mode == MODE_SERVER)
> +    {
> +        msg(msglevel, "Only client and p2p data channel offload is
> supported "
> +            "with ovpn-dco-win.");
> +        return false;
> +    }
> +
> +    if (o->persist_tun)
> +    {
> +        msg(msglevel, "--persist-tun is not supported with
> ovpn-dco-win.");
> +        return false;
> +    }
> +#elif defined(TARGET_LINUX)
>      /* if the device name is fixed, we need to check if an interface with
> this
>       * name already exists. IF it does, it must be a DCO interface,
> otherwise
>       * DCO has to be disabled in order to continue.
> @@ -246,7 +259,7 @@ dco_check_option_conflict_platform(int msglevel, const
> struct options *o)
>                  strerror(-ret), ret);
>          }
>      }
> -#endif /* if defined(TARGET_LINUX) */
> +#endif /* if defined(_WIN32) */
>      return true;
>  }
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 14cb4cc4..cec6cf10 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -2450,6 +2450,11 @@ options_postprocess_verify_ce(const struct options
> *options,
>      {
>          msg(M_USAGE, "--windows-driver wintun requires --dev tun");
>      }
> +
> +    if (options->windows_driver == WINDOWS_DRIVER_DCO)
> +    {
> +        dco_check_option_conflict(M_USAGE, options);
> +    }
>  #endif /* ifdef _WIN32 */
>
>      /*
> --
> 2.35.1
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
Gert Doering Aug. 17, 2022, 10:06 p.m. UTC | #2 
Hi,

On Sat, Aug 13, 2022 at 10:42:19PM +0200, Antonio Quartulli wrote:
> At the moment dco-win doesn't support --persist-tun and --server,
> so check for these options at startup time.

This needs rebasing anyway (due to the startup change), but while at it...

> +
> +    if (options->windows_driver == WINDOWS_DRIVER_DCO)
> +    {
> +        dco_check_option_conflict(M_USAGE, options);
> +    }

... this should read "if (dco_enabled(options))", no?  Patch 3/7 changes
dco_enabled() to do exactly this "if (driver == WINDOWS_DRIVER_DCO)" and
it would make *this* function more clear.

OTOH, the first thing dco_check_options_conflict() does is "check
if DCO is enabled at all", so why check it in the caller again?


Staring at this for a while, I do wonder why we do the "linux DCO"
options check in options_postprocess_mutate(), and the "Win32 DCO"
options check in options_postprocess_verify_ce() (while it does not
look at the actual ->ce entries, and really *really* does not belong).

Can we please clean this up, and have one joint call for all DCO
platforms?  I see no good reason for having two calls to the same
function, doing the same thing, in two different call chains for
different OSes.


Now there is a material difference, if the Win32 invocation finds
a dco-incompatible option, it will exit M_USAGE, while Linux will just
disable DCO - if this is what we want, we should still keep the stuff
closer together, like

    /* check if any option should force disabling DCO */
#if defined(TARGET_LINUX) || defined(TARGET_FREEBSD)
    o->tuntap_options.disable_dco = !dco_check_option_conflict(D_DCO, o)
                                    || !dco_check_startup_option_conflict(D_DCO,
 o);
#else if _WIN32
    /* we can not currently fallback to non-DCO on Windows, so exit M_USAGE
     * if an option conflict is discovered */
    dco_check_option_conflict(M_USAGE, o);
    dco_check_startup_option_conflict(M_USAGE, o);
#endif


gert
Gert Doering Aug. 17, 2022, 11:36 p.m. UTC | #3 
Hi,

On Thu, Aug 18, 2022 at 11:26:38AM +0200, Antonio Quartulli wrote:
> -#if defined(TARGET_LINUX) || defined(TARGET_FREEBSD)
> -    o->tuntap_options.disable_dco = !dco_check_option_conflict(D_DCO, o)
> -                                    || !dco_check_startup_option_conflict(D_DCO, o);
> -#endif
> +    o->tuntap_options.disable_dco = !dco_check_option_conflict(DCO_CHECK_OPTION_LEVEL, o)
> +                                    || !dco_check_startup_option_conflict(DCO_CHECK_OPTION_LEVEL, o);

Nah, this is not good.

This reads as "this is code which will always return, and will log with
some arbitrary log level" (how would a reader know that D_DCO and 
DCO_CHECK_OPTION_LEVEL is not both coming from errror.h?).

Make it explicit, and explain why.

gert
diff mbox series

Patch

diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index b342bee1..dcb570f9 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -221,7 +221,20 @@  dco_update_keys(dco_context_t *dco, struct tls_multi *multi)
 static bool
 dco_check_option_conflict_platform(int msglevel, const struct options *o)
 {
-#if defined(TARGET_LINUX)
+#if defined(_WIN32)
+    if (o->mode == MODE_SERVER)
+    {
+        msg(msglevel, "Only client and p2p data channel offload is supported "
+            "with ovpn-dco-win.");
+        return false;
+    }
+
+    if (o->persist_tun)
+    {
+        msg(msglevel, "--persist-tun is not supported with ovpn-dco-win.");
+        return false;
+    }
+#elif defined(TARGET_LINUX)
     /* if the device name is fixed, we need to check if an interface with this
      * name already exists. IF it does, it must be a DCO interface, otherwise
      * DCO has to be disabled in order to continue.
@@ -246,7 +259,7 @@  dco_check_option_conflict_platform(int msglevel, const struct options *o)
                 strerror(-ret), ret);
         }
     }
-#endif /* if defined(TARGET_LINUX) */
+#endif /* if defined(_WIN32) */
     return true;
 }
 
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 14cb4cc4..cec6cf10 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2450,6 +2450,11 @@  options_postprocess_verify_ce(const struct options *options,
     {
         msg(M_USAGE, "--windows-driver wintun requires --dev tun");
     }
+
+    if (options->windows_driver == WINDOWS_DRIVER_DCO)
+    {
+        dco_check_option_conflict(M_USAGE, options);
+    }
 #endif /* ifdef _WIN32 */
 
     /*