route add ::/0 ...

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

route add ::/0 ...

YASUOKA Masahiko-3
Hi,

When "::/0" is used as "default",

  # route add ::/0 fe80::1%em0
  add net ::/0: gateway fe80::1%em0: Invalid argument

route command trims the sockaddr to { .len = 2, .family = AF_INET6 }
for "::/0", but rtable_satoplen() refuses it.  I think it should be
accepted.

ok?

Allow sockaddr for prefix length be trimmed before the key(address)
field.  Actually "route" command trims at the address family field for
"::/0"

Index: sys/net/rtable.c
===================================================================
RCS file: /cvs/src/sys/net/rtable.c,v
retrieving revision 1.69
diff -u -p -r1.69 rtable.c
--- sys/net/rtable.c 21 Jun 2019 17:11:42 -0000 1.69
+++ sys/net/rtable.c 28 Jun 2020 11:30:54 -0000
@@ -887,8 +887,8 @@ rtable_satoplen(sa_family_t af, struct s
 
  ap = (uint8_t *)((uint8_t *)mask) + dp->dom_rtoffset;
  ep = (uint8_t *)((uint8_t *)mask) + mlen;
- if (ap > ep)
- return (-1);
+ if (ap >= ep)
+ return (0);
 
  /* Trim trailing zeroes. */
  while (ap < ep && ep[-1] == 0)

Reply | Threaded
Open this post in threaded view
|

Re: route add ::/0 ...

Martin Pieuchot
On 28/06/20(Sun) 20:41, YASUOKA Masahiko wrote:

> Hi,
>
> When "::/0" is used as "default",
>
>   # route add ::/0 fe80::1%em0
>   add net ::/0: gateway fe80::1%em0: Invalid argument
>
> route command trims the sockaddr to { .len = 2, .family = AF_INET6 }
> for "::/0", but rtable_satoplen() refuses it.  I think it should be
> accepted.

rtable_satoplen() is used in many places, not just in the socket parsing
code used by route(8).  I don't know what side effects can be introduced
by this change.

Why is IPv6 different from IPv4 when it comes to the default route?
Shouldn't we change route(8) to have a `sa_len' of 0?  That would make
the following true:

        mlen = mask->sa_len;

        /* Default route */
        if (mlen == 0)
                return (0)

> Allow sockaddr for prefix length be trimmed before the key(address)
> field.  Actually "route" command trims at the address family field for
> "::/0"
>
> Index: sys/net/rtable.c
> ===================================================================
> RCS file: /cvs/src/sys/net/rtable.c,v
> retrieving revision 1.69
> diff -u -p -r1.69 rtable.c
> --- sys/net/rtable.c 21 Jun 2019 17:11:42 -0000 1.69
> +++ sys/net/rtable.c 28 Jun 2020 11:30:54 -0000
> @@ -887,8 +887,8 @@ rtable_satoplen(sa_family_t af, struct s
>  
>   ap = (uint8_t *)((uint8_t *)mask) + dp->dom_rtoffset;
>   ep = (uint8_t *)((uint8_t *)mask) + mlen;
> - if (ap > ep)
> - return (-1);
> + if (ap >= ep)
> + return (0);

That means the kernel now silently ignore sockaddr short `sa_len'. Are
they supposed to be supported or are they symptoms of bugs?

>   /* Trim trailing zeroes. */
>   while (ap < ep && ep[-1] == 0)

Reply | Threaded
Open this post in threaded view
|

Re: route add ::/0 ...

YASUOKA Masahiko-3
Hi,

On Mon, 29 Jun 2020 10:12:23 +0200
Martin Pieuchot <[hidden email]> wrote:

> On 28/06/20(Sun) 20:41, YASUOKA Masahiko wrote:
>> Hi,
>>
>> When "::/0" is used as "default",
>>
>>   # route add ::/0 fe80::1%em0
>>   add net ::/0: gateway fe80::1%em0: Invalid argument
>>
>> route command trims the sockaddr to { .len = 2, .family = AF_INET6 }
>> for "::/0", but rtable_satoplen() refuses it.  I think it should be
>> accepted.
>
> rtable_satoplen() is used in many places, not just in the socket parsing
> code used by route(8).  I don't know what side effects can be introduced
> by this change.
>
> Why is IPv6 different from IPv4 when it comes to the default route?

Diferent functions are used.  route(8) uses inet_makenetandmask() to
create a sockaddr for IPv4 prefix length and uses prefixlen() for IPv6
prefix length.  "/0" results:

IPv4
  { .len = 1, .family = 0, ... }
IPv6
  { .len = 2, .family = AF_INET6, ... }

Next, route(8) uses mask_addr() to trim the tailing zeros.

1129 void
1130 mask_addr(union sockunion *addr, union sockunion *mask, int which)
1131 {
1132         int olen = mask->sa.sa_len;
1133         char *cp1 = olen + (char *)mask, *cp2;
1134
1135         for (mask->sa.sa_len = 0; cp1 > (char *)mask; )
1136                 if (*--cp1 != '\0') {
1137                         mask->sa.sa_len = 1 + cp1 - (char *)mask;
1138                         break;
1139                 }

See #1135 carefully.  As the results, the sockaddrs become:

IPv4
  { .len = 0, .family = 0, ... }
IPv6
  { .len = 2, .family = AF_INET6, ... }

Yes, we can fix IPv6 case to have .len = 0 as well.

But I thought kernel should accept both cases, since the
representation for IPv6 didn't seem so bad for me.

> Shouldn't we change route(8) to have a `sa_len' of 0?
>
> That would make the following true:
>
>         mlen = mask->sa_len;
>
> /* Default route */
> if (mlen == 0)
> return (0)
>
>> Allow sockaddr for prefix length be trimmed before the key(address)
>> field.  Actually "route" command trims at the address family field for
>> "::/0"
>>
>> Index: sys/net/rtable.c
>> ===================================================================
>> RCS file: /cvs/src/sys/net/rtable.c,v
>> retrieving revision 1.69
>> diff -u -p -r1.69 rtable.c
>> --- sys/net/rtable.c 21 Jun 2019 17:11:42 -0000 1.69
>> +++ sys/net/rtable.c 28 Jun 2020 11:30:54 -0000
>> @@ -887,8 +887,8 @@ rtable_satoplen(sa_family_t af, struct s
>>  
>>   ap = (uint8_t *)((uint8_t *)mask) + dp->dom_rtoffset;
>>   ep = (uint8_t *)((uint8_t *)mask) + mlen;
>> - if (ap > ep)
>> - return (-1);
>> + if (ap >= ep)
>> + return (0);
>
> That means the kernel now silently ignore sockaddr short `sa_len'. Are
> they supposed to be supported or are they symptoms of bugs?

I have missed rtable_satoplen() is used by other functions.

>
>>   /* Trim trailing zeroes. */
>>   while (ap < ep && ep[-1] == 0)
>

Reply | Threaded
Open this post in threaded view
|

Re: route add ::/0 ...

YASUOKA Masahiko-3
On Mon, 29 Jun 2020 18:45:07 +0900 (JST)
YASUOKA Masahiko <[hidden email]> wrote:

> On Mon, 29 Jun 2020 10:12:23 +0200
> Martin Pieuchot <[hidden email]> wrote:
>> On 28/06/20(Sun) 20:41, YASUOKA Masahiko wrote:
>>> Hi,
>>>
>>> When "::/0" is used as "default",
>>>
>>>   # route add ::/0 fe80::1%em0
>>>   add net ::/0: gateway fe80::1%em0: Invalid argument
>>>
>>> route command trims the sockaddr to { .len = 2, .family = AF_INET6 }
>>> for "::/0", but rtable_satoplen() refuses it.  I think it should be
>>> accepted.
>>
>> rtable_satoplen() is used in many places, not just in the socket parsing
>> code used by route(8).  I don't know what side effects can be introduced
>> by this change.
>>
>> Why is IPv6 different from IPv4 when it comes to the default route?
>
> Diferent functions are used.  route(8) uses inet_makenetandmask() to
> create a sockaddr for IPv4 prefix length and uses prefixlen() for IPv6
> prefix length.  "/0" results:
>
> IPv4
>   { .len = 1, .family = 0, ... }
> IPv6
>   { .len = 2, .family = AF_INET6, ... }

I'm sorry this is not correct.  It is actually

IPv6
  { .len = 28, .family = AF_INET6, ... }

> Next, route(8) uses mask_addr() to trim the tailing zeros.
>
> 1129 void
> 1130 mask_addr(union sockunion *addr, union sockunion *mask, int which)
> 1131 {
> 1132         int olen = mask->sa.sa_len;
> 1133         char *cp1 = olen + (char *)mask, *cp2;
> 1134
> 1135         for (mask->sa.sa_len = 0; cp1 > (char *)mask; )
> 1136                 if (*--cp1 != '\0') {
> 1137                         mask->sa.sa_len = 1 + cp1 - (char *)mask;
> 1138                         break;
> 1139                 }
>
> See #1135 carefully.  As the results, the sockaddrs become:
>
> IPv4
>   { .len = 0, .family = 0, ... }
> IPv6
>   { .len = 2, .family = AF_INET6, ... }

I'm start wondering what the mask_addr() is for.

   1123 void
   1124 mask_addr(union sockunion *addr, union sockunion *mask, int which)
   1125 {
   1126         int olen = mask->sa.sa_len;
   1127         char *cp1 = olen + (char *)mask, *cp2;
   1128
   1129         for (mask->sa.sa_len = 0; cp1 > (char *)mask; )
   1130                 if (*--cp1 != '\0') {
   1131                         mask->sa.sa_len = 1 + cp1 - (char *)mask;
   1132                         break;
   1133                 }
   1134         if ((rtm_addrs & which) == 0)
   1135                 return;
   1136         switch (addr->sa.sa_family) {
   1137         case AF_INET:
   1138         case AF_INET6:
   1139         case AF_UNSPEC:
   1140                 return;
   1141         }
   1142         cp1 = mask->sa.sa_len + 1 + (char *)addr;
   1143         cp2 = addr->sa.sa_len + 1 + (char *)addr;
   1144         while (cp2 > cp1)
   1145                 *--cp2 = '\0';
   1146         cp2 = mask->sa.sa_len + 1 + (char *)mask;
   1147         while (cp1 > addr->sa.sa_data)
   1148                 *--cp1 &= *--cp2;
   1149 }

The function mask_addr() doesn't mask address for IPv4 and IPv6.  Does
any address family other than IPv4 or IPv6 require #1142:1148?  The
function seems to just trim the trailing zero.  Is it neccesaary?  And
it causes the confusion on the kernel.  How about deleting
mask_addr()?

The diff following also fixes the problem.

diff --git a/sbin/route/route.c b/sbin/route/route.c
index 9e43d8e89b6..87f26e5c1e7 100644
--- a/sbin/route/route.c
+++ b/sbin/route/route.c
@@ -107,7 +107,6 @@ void print_rtmsg(struct rt_msghdr *, int);
 void pmsg_common(struct rt_msghdr *);
 void pmsg_addrs(char *, int);
 void bprintf(FILE *, int, char *);
-void mask_addr(union sockunion *, union sockunion *, int);
 int getaddr(int, int, char *, struct hostent **);
 void getmplslabel(char *, int);
 int rtmsg(int, int, int, uint8_t);
@@ -1088,8 +1087,6 @@ rtmsg(int cmd, int flags, int fmask, uint8_t prio)
  rtm.rtm_mpls = mpls_flags;
  rtm.rtm_hdrlen = sizeof(rtm);
 
- if (rtm_addrs & RTA_NETMASK)
- mask_addr(&so_dst, &so_mask, RTA_DST);
  /* store addresses in ascending order of RTA values */
  NEXTADDR(RTA_DST, so_dst);
  NEXTADDR(RTA_GATEWAY, so_gate);
@@ -1120,34 +1117,6 @@ rtmsg(int cmd, int flags, int fmask, uint8_t prio)
  return (0);
 }
 
-void
-mask_addr(union sockunion *addr, union sockunion *mask, int which)
-{
- int olen = mask->sa.sa_len;
- char *cp1 = olen + (char *)mask, *cp2;
-
- for (mask->sa.sa_len = 0; cp1 > (char *)mask; )
- if (*--cp1 != '\0') {
- mask->sa.sa_len = 1 + cp1 - (char *)mask;
- break;
- }
- if ((rtm_addrs & which) == 0)
- return;
- switch (addr->sa.sa_family) {
- case AF_INET:
- case AF_INET6:
- case AF_UNSPEC:
- return;
- }
- cp1 = mask->sa.sa_len + 1 + (char *)addr;
- cp2 = addr->sa.sa_len + 1 + (char *)addr;
- while (cp2 > cp1)
- *--cp2 = '\0';
- cp2 = mask->sa.sa_len + 1 + (char *)mask;
- while (cp1 > addr->sa.sa_data)
- *--cp1 &= *--cp2;
-}
-
 char *msgtypes[] = {
  "",
  "RTM_ADD: Add Route",

> Yes, we can fix IPv6 case to have .len = 0 as well.
>
> But I thought kernel should accept both cases, since the
> representation for IPv6 didn't seem so bad for me.
>
>> Shouldn't we change route(8) to have a `sa_len' of 0?
>>
>> That would make the following true:
>>
>>         mlen = mask->sa_len;
>>
>> /* Default route */
>> if (mlen == 0)
>> return (0)
>>
>>> Allow sockaddr for prefix length be trimmed before the key(address)
>>> field.  Actually "route" command trims at the address family field for
>>> "::/0"
>>>
>>> Index: sys/net/rtable.c
>>> ===================================================================
>>> RCS file: /cvs/src/sys/net/rtable.c,v
>>> retrieving revision 1.69
>>> diff -u -p -r1.69 rtable.c
>>> --- sys/net/rtable.c 21 Jun 2019 17:11:42 -0000 1.69
>>> +++ sys/net/rtable.c 28 Jun 2020 11:30:54 -0000
>>> @@ -887,8 +887,8 @@ rtable_satoplen(sa_family_t af, struct s
>>>  
>>>   ap = (uint8_t *)((uint8_t *)mask) + dp->dom_rtoffset;
>>>   ep = (uint8_t *)((uint8_t *)mask) + mlen;
>>> - if (ap > ep)
>>> - return (-1);
>>> + if (ap >= ep)
>>> + return (0);
>>
>> That means the kernel now silently ignore sockaddr short `sa_len'. Are
>> they supposed to be supported or are they symptoms of bugs?
>
> I have missed rtable_satoplen() is used by other functions.
>
>>
>>>   /* Trim trailing zeroes. */
>>>   while (ap < ep && ep[-1] == 0)
>>

Reply | Threaded
Open this post in threaded view
|

Re: route add ::/0 ...

Klemens Nanni-2
On Mon, Jun 29, 2020 at 11:55:10PM +0900, YASUOKA Masahiko wrote:
> The function mask_addr() doesn't mask address for IPv4 and IPv6.  Does
> any address family other than IPv4 or IPv6 require #1142:1148?  The
> function seems to just trim the trailing zero.  Is it neccesaary?  And
> it causes the confusion on the kernel.  How about deleting
> mask_addr()?
>
> The diff following also fixes the problem.
Removing it breaks IPv4 default routes:

        # ifconfig lo1 rdomain 1 127.1.1.1
        # ./obj/route -nT1 add 0.0.0.0/0 127.1.1.1
        add net 0.0.0.0/0: gateway 127.1.1.1: Invalid argument
        # route -nT1 add 0.0.0.0/0 127.1.1.1      
        add net 0.0.0.0/0: gateway 127.1.1.1

Reply | Threaded
Open this post in threaded view
|

Re: route add ::/0 ...

YASUOKA Masahiko-3
On Mon, 29 Jun 2020 19:18:17 +0200
Klemens Nanni <[hidden email]> wrote:

> On Mon, Jun 29, 2020 at 11:55:10PM +0900, YASUOKA Masahiko wrote:
>> The function mask_addr() doesn't mask address for IPv4 and IPv6.  Does
>> any address family other than IPv4 or IPv6 require #1142:1148?  The
>> function seems to just trim the trailing zero.  Is it neccesaary?  And
>> it causes the confusion on the kernel.  How about deleting
>> mask_addr()?
>>
>> The diff following also fixes the problem.
> Removing it breaks IPv4 default routes:
>
> # ifconfig lo1 rdomain 1 127.1.1.1
> # ./obj/route -nT1 add 0.0.0.0/0 127.1.1.1
> add net 0.0.0.0/0: gateway 127.1.1.1: Invalid argument
> # route -nT1 add 0.0.0.0/0 127.1.1.1      
> add net 0.0.0.0/0: gateway 127.1.1.1

Thanks,

inet_makenetandmask() had required another treatment.

Also -prefixlen 0 for -inet has a bug

 % doas ./obj/route -T100 add -inet 0.0.0.0 -prefixlen 0 127.0.0.1
 add net 0.0.0.0: gateway 127.0.0.1
 % netstat -nrf inet -T 100
 Routing tables

 Internet:
 Destination        Gateway            Flags   Refs      Use   Mtu  Prio Iface
 0.0.0.0/32         127.0.0.1          UGS        0        0 32768     8 lo100

/0 becomes /32.  The diff following also fixes the problem.


diff --git a/sbin/route/route.c b/sbin/route/route.c
index 9e43d8e89b6..532a918148d 100644
--- a/sbin/route/route.c
+++ b/sbin/route/route.c
@@ -107,7 +107,6 @@ void print_rtmsg(struct rt_msghdr *, int);
 void pmsg_common(struct rt_msghdr *);
 void pmsg_addrs(char *, int);
 void bprintf(FILE *, int, char *);
-void mask_addr(union sockunion *, union sockunion *, int);
 int getaddr(int, int, char *, struct hostent **);
 void getmplslabel(char *, int);
 int rtmsg(int, int, int, uint8_t);
@@ -781,12 +780,9 @@ inet_makenetandmask(u_int32_t net, struct sockaddr_in *sin, int bits)
  sin->sin_addr.s_addr = htonl(net);
  sin = &so_mask.sin;
  sin->sin_addr.s_addr = htonl(mask);
- sin->sin_len = 0;
- sin->sin_family = 0;
+ sin->sin_family = AF_INET;
  cp = (char *)(&sin->sin_addr + 1);
- while (*--cp == '\0' && cp > (char *)sin)
- continue;
- sin->sin_len = 1 + cp - (char *)sin;
+ sin->sin_len = sizeof(struct sockaddr_in);
 }
 
 /*
@@ -1001,7 +997,8 @@ prefixlen(int af, char *s)
  memset(&so_mask, 0, sizeof(so_mask));
  so_mask.sin.sin_family = AF_INET;
  so_mask.sin.sin_len = sizeof(struct sockaddr_in);
- so_mask.sin.sin_addr.s_addr = htonl(0xffffffff << (32 - len));
+ if (len != 0)
+ so_mask.sin.sin_addr.s_addr = htonl(0xffffffff << (32 - len));
  break;
  case AF_INET6:
  so_mask.sin6.sin6_family = AF_INET6;
@@ -1088,8 +1085,6 @@ rtmsg(int cmd, int flags, int fmask, uint8_t prio)
  rtm.rtm_mpls = mpls_flags;
  rtm.rtm_hdrlen = sizeof(rtm);
 
- if (rtm_addrs & RTA_NETMASK)
- mask_addr(&so_dst, &so_mask, RTA_DST);
  /* store addresses in ascending order of RTA values */
  NEXTADDR(RTA_DST, so_dst);
  NEXTADDR(RTA_GATEWAY, so_gate);
@@ -1120,34 +1115,6 @@ rtmsg(int cmd, int flags, int fmask, uint8_t prio)
  return (0);
 }
 
-void
-mask_addr(union sockunion *addr, union sockunion *mask, int which)
-{
- int olen = mask->sa.sa_len;
- char *cp1 = olen + (char *)mask, *cp2;
-
- for (mask->sa.sa_len = 0; cp1 > (char *)mask; )
- if (*--cp1 != '\0') {
- mask->sa.sa_len = 1 + cp1 - (char *)mask;
- break;
- }
- if ((rtm_addrs & which) == 0)
- return;
- switch (addr->sa.sa_family) {
- case AF_INET:
- case AF_INET6:
- case AF_UNSPEC:
- return;
- }
- cp1 = mask->sa.sa_len + 1 + (char *)addr;
- cp2 = addr->sa.sa_len + 1 + (char *)addr;
- while (cp2 > cp1)
- *--cp2 = '\0';
- cp2 = mask->sa.sa_len + 1 + (char *)mask;
- while (cp1 > addr->sa.sa_data)
- *--cp1 &= *--cp2;
-}
-
 char *msgtypes[] = {
  "",
  "RTM_ADD: Add Route",

Reply | Threaded
Open this post in threaded view
|

Re: route add ::/0 ...

Klemens Nanni-2
On Tue, Jun 30, 2020 at 09:00:30AM +0900, YASUOKA Masahiko wrote:

> inet_makenetandmask() had required another treatment.
>
> Also -prefixlen 0 for -inet has a bug
>
>  % doas ./obj/route -T100 add -inet 0.0.0.0 -prefixlen 0 127.0.0.1
>  add net 0.0.0.0: gateway 127.0.0.1
>  % netstat -nrf inet -T 100
>  Routing tables
>
>  Internet:
>  Destination        Gateway            Flags   Refs      Use   Mtu  Prio Iface
>  0.0.0.0/32         127.0.0.1          UGS        0        0 32768     8 lo100
>
> /0 becomes /32.  The diff following also fixes the problem.
Yes, this looks correct to me;  regress is also happy (again).

OK kn