diff mbox series

[Openvpn-devel] networking/iproute2: support 31-bit ipv4 prefix (RFC 3021)

Message ID 20191103053053.17996-1-tom.ty89@gmail.com
State New
Headers
Series [Openvpn-devel] networking/iproute2: support 31-bit ipv4 prefix (RFC 3021) |

Commit Message

Tom Yan Nov. 2, 2019, 6:30 p.m. UTC 
  ---
 src/openvpn/networking_iproute2.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Antonio Quartulli Nov. 2, 2019, 11:43 p.m. UTC | #1 
Hi Tom,

first of all, thanks a lot for your contribution!

On 03/11/2019 06:30, Tom Yan wrote:
> ---
>  src/openvpn/networking_iproute2.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/openvpn/networking_iproute2.c b/src/openvpn/networking_iproute2.c
> index 1ddeb5cf..4e2435c1 100644
> --- a/src/openvpn/networking_iproute2.c
> +++ b/src/openvpn/networking_iproute2.c
> @@ -98,8 +98,10 @@ net_addr_v4_add(openvpn_net_ctx_t *ctx, const char *iface,
>      const char *addr_str = print_in_addr_t(*addr, 0, &ctx->gc);
>      const char *brd_str = print_in_addr_t(*broadcast, 0, &ctx->gc);
>  
> -    argv_printf(&argv, "%s addr add dev %s %s/%d broadcast %s", iproute_path,
> -                iface, addr_str, prefixlen, brd_str);
> +    argv_printf(&argv, "%s addr add dev %s %s/%d", iproute_path,
> +                iface, addr_str, prefixlen);
> +    if (prefixlen < 31)
> +        argv_printf_cat(&argv, " broadcast %s", brd_str);

Unfortunately I am not able to grasp this fix entirely.. (maybe because
I don't know RFC3021).

Would you mind explaining *why* this is needed?

What is this change fixing? Can you make an example where the actual
code fails and how this change is fixing it?


It would also be very nice if you could add such information to the
commit message, so that it will remain in the git history.


Thanks a lot!
Regards,

>      argv_msg(M_INFO, &argv);
>      openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip addr add failed");
>  
>
Tom Yan Nov. 3, 2019, 12:32 a.m. UTC | #2 
Hello Antonio,

With 31-bit IPv4 prefix, only two addresses can be provided with the
single host bit. Neither the network address nor the broadcast address
makes sense anymore in such "subnet", as it's basically for
point-to-point link. Assigning such address with a broadcast address
will prevent it from working properly anyway.

While we can achieve the equivalence with the soon-to-be-removed p2p
topology, or using a 30-bit prefix to avoid the bug (that "wastes" two
addresses), I don't see why it shouldn't be fixed, as it's a proper
standard (even though it may not be supported on every platform).

Note that this has nothing to do with the net30 topology but the
subnet topology.

Sorry for not having elaborated in the commit message, as I thought
the RFC is well-known and the patch already speaks for itself. If you
are okay with it I can resend it with the above as its commit message.

Regards,
Tom

On Sun, 3 Nov 2019 at 18:43, Antonio Quartulli <a@unstable.cc> wrote:
>
> Hi Tom,
>
> first of all, thanks a lot for your contribution!
>
> On 03/11/2019 06:30, Tom Yan wrote:
> > ---
> >  src/openvpn/networking_iproute2.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/openvpn/networking_iproute2.c b/src/openvpn/networking_iproute2.c
> > index 1ddeb5cf..4e2435c1 100644
> > --- a/src/openvpn/networking_iproute2.c
> > +++ b/src/openvpn/networking_iproute2.c
> > @@ -98,8 +98,10 @@ net_addr_v4_add(openvpn_net_ctx_t *ctx, const char *iface,
> >      const char *addr_str = print_in_addr_t(*addr, 0, &ctx->gc);
> >      const char *brd_str = print_in_addr_t(*broadcast, 0, &ctx->gc);
> >
> > -    argv_printf(&argv, "%s addr add dev %s %s/%d broadcast %s", iproute_path,
> > -                iface, addr_str, prefixlen, brd_str);
> > +    argv_printf(&argv, "%s addr add dev %s %s/%d", iproute_path,
> > +                iface, addr_str, prefixlen);
> > +    if (prefixlen < 31)
> > +        argv_printf_cat(&argv, " broadcast %s", brd_str);
>
> Unfortunately I am not able to grasp this fix entirely.. (maybe because
> I don't know RFC3021).
>
> Would you mind explaining *why* this is needed?
>
> What is this change fixing? Can you make an example where the actual
> code fails and how this change is fixing it?
>
>
> It would also be very nice if you could add such information to the
> commit message, so that it will remain in the git history.
>
>
> Thanks a lot!
> Regards,
>
> >      argv_msg(M_INFO, &argv);
> >      openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip addr add failed");
> >
> >
>
> --
> Antonio Quartulli
Tom Yan Nov. 3, 2019, 1:09 a.m. UTC | #3 
Btw, is there a particular reason that we don't simply use "broadcast
+" and let `ip` handle it? We won't even need to do the prefix length
check if we aren't setting the broadcast address explicitly.

Regards,

On Sun, 3 Nov 2019 at 19:32, Tom Yan <tom.ty89@gmail.com> wrote:
>
> Hello Antonio,
>
> With 31-bit IPv4 prefix, only two addresses can be provided with the
> single host bit. Neither the network address nor the broadcast address
> makes sense anymore in such "subnet", as it's basically for
> point-to-point link. Assigning such address with a broadcast address
> will prevent it from working properly anyway.
>
> While we can achieve the equivalence with the soon-to-be-removed p2p
> topology, or using a 30-bit prefix to avoid the bug (that "wastes" two
> addresses), I don't see why it shouldn't be fixed, as it's a proper
> standard (even though it may not be supported on every platform).
>
> Note that this has nothing to do with the net30 topology but the
> subnet topology.
>
> Sorry for not having elaborated in the commit message, as I thought
> the RFC is well-known and the patch already speaks for itself. If you
> are okay with it I can resend it with the above as its commit message.
>
> Regards,
> Tom
>
> On Sun, 3 Nov 2019 at 18:43, Antonio Quartulli <a@unstable.cc> wrote:
> >
> > Hi Tom,
> >
> > first of all, thanks a lot for your contribution!
> >
> > On 03/11/2019 06:30, Tom Yan wrote:
> > > ---
> > >  src/openvpn/networking_iproute2.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/openvpn/networking_iproute2.c b/src/openvpn/networking_iproute2.c
> > > index 1ddeb5cf..4e2435c1 100644
> > > --- a/src/openvpn/networking_iproute2.c
> > > +++ b/src/openvpn/networking_iproute2.c
> > > @@ -98,8 +98,10 @@ net_addr_v4_add(openvpn_net_ctx_t *ctx, const char *iface,
> > >      const char *addr_str = print_in_addr_t(*addr, 0, &ctx->gc);
> > >      const char *brd_str = print_in_addr_t(*broadcast, 0, &ctx->gc);
> > >
> > > -    argv_printf(&argv, "%s addr add dev %s %s/%d broadcast %s", iproute_path,
> > > -                iface, addr_str, prefixlen, brd_str);
> > > +    argv_printf(&argv, "%s addr add dev %s %s/%d", iproute_path,
> > > +                iface, addr_str, prefixlen);
> > > +    if (prefixlen < 31)
> > > +        argv_printf_cat(&argv, " broadcast %s", brd_str);
> >
> > Unfortunately I am not able to grasp this fix entirely.. (maybe because
> > I don't know RFC3021).
> >
> > Would you mind explaining *why* this is needed?
> >
> > What is this change fixing? Can you make an example where the actual
> > code fails and how this change is fixing it?
> >
> >
> > It would also be very nice if you could add such information to the
> > commit message, so that it will remain in the git history.
> >
> >
> > Thanks a lot!
> > Regards,
> >
> > >      argv_msg(M_INFO, &argv);
> > >      openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip addr add failed");
> > >
> > >
> >
> > --
> > Antonio Quartulli
diff mbox series

Patch

diff --git a/src/openvpn/networking_iproute2.c b/src/openvpn/networking_iproute2.c
index 1ddeb5cf..4e2435c1 100644
--- a/src/openvpn/networking_iproute2.c
+++ b/src/openvpn/networking_iproute2.c
@@ -98,8 +98,10 @@  net_addr_v4_add(openvpn_net_ctx_t *ctx, const char *iface,
     const char *addr_str = print_in_addr_t(*addr, 0, &ctx->gc);
     const char *brd_str = print_in_addr_t(*broadcast, 0, &ctx->gc);
 
-    argv_printf(&argv, "%s addr add dev %s %s/%d broadcast %s", iproute_path,
-                iface, addr_str, prefixlen, brd_str);
+    argv_printf(&argv, "%s addr add dev %s %s/%d", iproute_path,
+                iface, addr_str, prefixlen);
+    if (prefixlen < 31)
+        argv_printf_cat(&argv, " broadcast %s", brd_str);
     argv_msg(M_INFO, &argv);
     openvpn_execve_check(&argv, ctx->es, S_FATAL, "Linux ip addr add failed");