lib: handle default route destinations in nla_put_addr()

[KanjiMonster]

Default routes (0.0.0.0/0 and ::/0) won't have any data allocated, so their data length is zero. This causes nla_put_addr() to generate a zero-length address attribute, which is rightfully rejected by the kernel.

So in case we we get an address with a zero-length prefix, just allocate an appropriate sized attribute for the address family. Since nla_reserve initializes the attribute, we don't need to do anything with it.

Signed-off-by: Jonas Gorski jonas.gorski@bisdn.de

[thom311]

this looks right, but how was this working before? Did it not work? That would seem unexpected for behavior that exists for so long already.

[thom311]

Default routes (0.0.0.0/0 and ::/0) won't have any data allocated, so their data length is zero.

Why is that? This assertions seems wrong to me. If you create a struct nl_addr, I think you are supposed to provide the correct size (4 or 16 bytes, for IPv4/IPv6) with an all-zero buffer.

[KanjiMonster]

Default routes (0.0.0.0/0 and ::/0) won't have any data allocated, so their data length is zero.

Why is that? This assertions seems wrong to me. If you create a struct nl_addr, I think you are supposed to provide the correct size (4 or 16 bytes, for IPv4/IPv6) with an all-zero buffer.

rtnl_route_parse allocates a zero-length address if there is no RTA_DST (which is true for default routes), then sets family and prefixlen on that address (which was where the address triggering the issue came from for us, since we later constructed a netlink message based on the address returned by rtnl_route_get_dst()).

Similarily, nl_addr_parse will leave allocate a 0-length address for "none", "any", "all" and "default" (doesn't change len from 0 or sets it explicitly to 0, then goes to prefix where the address is allocated with len set to 0).

So to me this seemed to be intentional (haven't checked all places, just noticed these two).

[thom311]

I see. But shouldn't it be:

    len = nl_addr_get_len(addr);
    if (len > 0 || nl_addr_get_prefixlen(addr) > 0)
        return nla_put(msg, attrtype, len, nl_addr_get_binary_addr(addr));

    switch (nl_addr_get_family(addr)) {
        ...

[KanjiMonster]

I see. But shouldn't it be:

    len = nl_addr_get_len(addr);
    if (len > 0 || nl_addr_get_prefixlen(addr) > 0)
        return nla_put(msg, attrtype, len, nl_addr_get_binary_addr(addr));

    switch (nl_addr_get_family(addr)) {
        ...

True, that might be a better solution.

While investiating a bit further, I found an additional issue caused by the 0-length addresses: using nl_addr_parse, the address "0.0.0.0/0" does create an address with len > 0, which will then cause it to not be considered equal with nl_addr_cmp(), although nl_addr_cmp_prefix() will work.

This can be observed using src/nl-route-get:

$ ./src/nl-route-list --dst default --family=inet
inet default table main type unicast via 172.16.103.1 dev enp0s31f6
$ ./src/nl-route-list --dst 0.0.0.0/0 --family=inet
$ 

While technically true that these addresses are not identical, they probably should still compare as equal.

So the proper fix might be to always allocate the address with the appropriate length, and then there is no special casing needed, and it fixes the comparison.

[KanjiMonster]

This can be observed using src/nl-route-get:

$ ./src/nl-route-list --dst default --family=inet
inet default table main type unicast via 172.16.103.1 dev enp0s31f6
$ ./src/nl-route-list --dst 0.0.0.0/0 --family=inet
$ 

As comparison with ip, where both works:

$ ip route show default
default via 172.16.103.1 dev enp0s31f6 proto dhcp src 172.16.103.187 metric 20100 
$ ip route show 0.0.0.0/0
default via 172.16.103.1 dev enp0s31f6 proto dhcp src 172.16.103.187 metric 20100 

[KanjiMonster]

How about we fix it the other way round:

diff --git a/lib/addr.c b/lib/addr.c
index fae129343ec7..ab4bf2210cf1 100644
--- a/lib/addr.c
+++ b/lib/addr.c
@@ -325,14 +325,17 @@ int nl_addr_parse(const char *addrstr, int hint, struct nl_addr **result)
                 * no hint given the user wants to have a IPv4
                 * address given back. */
                family = AF_INET;
+               len = 4;
                goto prefix;

            case AF_INET6:
                family = AF_INET6;
+               len = 16;
                goto prefix;

            case AF_LLC:
                family = AF_LLC;
+               len = 6;
                goto prefix;

            default:
@@ -449,6 +452,8 @@ prefix:

    if (copy)
        nl_addr_set_binary_addr(addr, buf, len);
+   else
+       addr->a_len = len;

    if (prefix) {
        char *p;
@@ -460,7 +465,7 @@ prefix:
        }
        nl_addr_set_prefixlen(addr, pl);
    } else {
-       if (!plen)
+       if (copy && !plen)
            plen = len * 8;
        nl_addr_set_prefixlen(addr, plen);
    }
diff --git a/lib/route/route_obj.c b/lib/route/route_obj.c
index 9441b77a76e5..123101fb9282 100644
--- a/lib/route/route_obj.c
+++ b/lib/route/route_obj.c
@@ -145,7 +145,7 @@ static void route_dump_line(struct nl_object *a, struct nl_dump_params *p)
        nl_dump(p, "cache ");

    if (!(r->ce_mask & ROUTE_ATTR_DST) ||
-       nl_addr_get_len(r->rt_dst) == 0)
+       nl_addr_get_prefixlen(r->rt_dst) == 0)
        nl_dump(p, "default ");
    else
        nl_dump(p, "%s ", nl_addr2str(r->rt_dst, buf, sizeof(buf)));
@@ -1155,13 +1155,12 @@ int rtnl_route_parse(struct nlmsghdr *nlh, struct rtnl_route **result)
    if (tb[RTA_DST]) {
        if (!(dst = nl_addr_alloc_attr(tb[RTA_DST], family)))
            return -NLE_NOMEM;
+       nl_addr_set_prefixlen(dst, rtm->rtm_dst_len);
    } else {
-       if (!(dst = nl_addr_alloc(0)))
-           return -NLE_NOMEM;
-       nl_addr_set_family(dst, rtm->rtm_family);
+       if ((err = nl_addr_parse("default", family, &dst)))
+           return err;
    }

-   nl_addr_set_prefixlen(dst, rtm->rtm_dst_len);
    err = rtnl_route_set_dst(route, dst);
    if (err < 0)
        return err;

So let nl_addr_parse() generate zero'd addresses for "default"/"any"/"all" (but not for "none"), and let rtnl_route_parse() create a all-zero address as well (shorthanded here with reusing nl_addr_parse()).

For route_dump_line() we then need to check the prefixlen since len cannot be 0 anymore, but prefixlen should always be 0 for the default route.

This should mostly avoid generating addresses that generate invalid netlink messages (maybe nla_putl_addr() should just reject addresses with 0 length as well, in case somebody tries to use one).

[thom311]

How about we fix it the other way round:

I think this is better.

nla_put_addr() should not try to be too smart and workaround nonsensical nl-addrs. It's better that code that creates nl_addr, takes care to create them correctly.

[KanjiMonster]

How about we fix it the other way round:

I think this is better.

nla_put_addr() should not try to be too smart and workaround nonsensical nl-addrs. It's better that code that creates nl_addr, takes care to create them correctly.

Done. Now creates appropriately length address objects, except for the "none" address, for which it seemed wrong to set an address.

[KanjiMonster]

How about we fix it the other way round:

I think this is better. nla_put_addr() should not try to be too smart and workaround nonsensical nl-addrs. It's better that code that creates nl_addr, takes care to create them correctly.

Done. Now creates appropriately length address objects, except for the "none" address, for which it seemed wrong to set an address.

This should AFAICT also ensure that "none" won't compare equal to "any" or "default" or any other address (unless you use nl_addr_cmp_prefix).

[thom311]

merged. Thank you for your efforts!!

Unfortunately, i didn't wait with 3.7.0 for this, but the next release will also happen (eventually).