On Mon, 29 Jun 2020 18:45:07 +0900 (JST)
YASUOKA Masahiko <yasu...@openbsd.org> wrote:
> On Mon, 29 Jun 2020 10:12:23 +0200
> Martin Pieuchot <m...@openbsd.org> 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 via email to