On Mon, 29 Jun 2020 18:45:07 +0900 (JST)
YASUOKA Masahiko <[email protected]> wrote:
> On Mon, 29 Jun 2020 10:12:23 +0200
> Martin Pieuchot <[email protected]> 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)
>>
