Bug 2162315

Hi Thomas,

Before I found the root issue for this bug(I have discussed with David Ahern. The fix looks not straight forward). Would you like to try the `ip nexthop` cmd? e.g.

# ip nexthop add id 1 via 192.168.1.1 dev eth1
# ip nexthop add id 2 via 192.168.1.1 dev eth1
# ip nexthop add id 101 group 1,1/2,2
# ip route add 192.168.2.0/24 nhid 101
# ip route show 192.168.2.0/24
192.168.2.0/24 nhid 101
        nexthop via 192.168.1.1 dev eth1 weight 1
        nexthop via 192.168.1.1 dev eth1 weight 2

# ip nexthop del id 1
# ip nexthop show
id 2 via 192.168.1.1 dev eth1 scope host
id 101 group 2,2
# ip route show 192.168.2.0/24
192.168.2.0/24 nhid 101 via 192.168.1.1 dev eth1

# ip nexthop add id 1 via 192.168.1.1 dev eth1
# ip nexthop replace id 101 group 1,1/2,2
# ip route show 192.168.2.0/24
192.168.2.0/24 nhid 101
        nexthop via 192.168.1.1 dev eth1 weight 1
        nexthop via 192.168.1.1 dev eth1 weight 2

`ip nexthop` has nh id, which could be used to select each nexthop.

Thanks
Hangbin

hi Hangbin.

I don't understand what is there to try. I am aware of nexthop objects, thank you.

Do you mean, NetworkManager should use nexthop objects? That may well be, however it would be a larger change and require some more analysis.

But this bug report isn't really about NetworkManager. Yes, NetworkManager is mentioned as an example for how it affects a user. But the problem is not NetworkManager specific.

If the issues (this and others) of the netlink API for route objects can be fixed, then there seems less reason to change NetworkManager to nexthop objects.
If it cannot (won't) be fixed, then would be another argument for using nexthop objects...

(In reply to Thomas Haller from comment #2)
> Do you mean, NetworkManager should use nexthop objects? 

Yes. This is the new nexthop api which should avoid most conflicts.

> That may well be, however it would be a larger change and require some more analysis.

Got it.

> 
> But this bug report isn't really about NetworkManager. Yes, NetworkManager
> is mentioned as an example for how it affects a user. But the problem is not
> NetworkManager specific.
> 
> If the issues (this and others) of the netlink API for route objects can be
> fixed, then there seems less reason to change NetworkManager to nexthop
> objects.
> If it cannot (won't) be fixed, then would be another argument for using
> nexthop objects...

OK. I will try to fix the nexthop bugs you opened. The IPv4 and IPv6 route codes
have a lot of differences. I'm not sure if upstream will accept all.

For this bug. The reason that IPv4 could add 2 same routes with only different weights is
IPv4 checks all the fields of 2 routes.

```
int fib_table_insert(struct net *net, struct fib_table *tb,
                     struct fib_config *cfg, struct netlink_ext_ack *extack)
{
                ...
                fa_match = NULL;
                fa_first = fa;
                hlist_for_each_entry_from(fa, fa_list) {
                        if ((fa->fa_slen != slen) ||
                            (fa->tb_id != tb->tb_id) ||
                            (fa->fa_dscp != dscp))
                                break;
                        if (fa->fa_info->fib_priority != fi->fib_priority)
                                break;
                        if (fa->fa_type == cfg->fc_type &&
                            fa->fa_info == fi) {                   <-- here it checks the total struct fib_info
                                fa_match = fa;
                                break;
                        }
               ...
```

While IPv6 only checks the dev and gw address.

```
static inline bool rt6_duplicate_nexthop(struct fib6_info *a, struct fib6_info *b)
{
        struct fib6_nh *nha, *nhb;

        if (a->nh || b->nh)
                return nexthop_cmp(a->nh, b->nh);

        nha = a->fib6_nh;
        nhb = b->fib6_nh;
        return nha->fib_nh_dev == nhb->fib_nh_dev &&
               ipv6_addr_equal(&nha->fib_nh_gw6, &nhb->fib_nh_gw6) &&
               !lwtunnel_cmp_encap(nha->fib_nh_lws, nhb->fib_nh_lws);
}
```

(In reply to Hangbin Liu from comment #3)
> For this bug. The reason that IPv4 could add 2 same routes with only
> different weights is IPv4 checks all the fields of 2 routes.

Update:

  # ip route append 1.2.3.4 nexthop via 192.168.5.1 dev net0 weight 1
  # ip route append 1.2.3.4 nexthop via 192.168.5.1 dev net0 weight 2
  # ip -4 -d route show
  ...
  unicast 1.2.3.4 via 192.168.5.1 dev net0 proto boot scope global 
  unicast 1.2.3.4 via 192.168.5.1 dev net0 proto boot scope global

These 2 routes are totally 2 different routes, not same route (to 1.2.3.4) with different nexthops.

Here is why they tread them as different routes.

- fib_create_info
  - fib_find_info()
    - nh_comp()
      - nh->fib_nh_weight != onh->fib_nh_weight

As these 2 routes are just regular routes instead of multipath routes. So kernel
only dump the normal route info instead of weight info. e.g.

  - fib_dump_info
    - if (nhs == 1): fib_nexthop_info
      - just add RTA_GATEWAY
    - else: fib_add_multipath
      - Start with RTA_MULTIPATH flag
      - fib_add_nexthop(with fib_nh_weight)
        - add struct rtnexthop rtnh to store the weight info
        - fib_nexthop_info

Confirmed with David, This will not be fixed as it will change the uAPI.

Reference link: https://lore.kernel.org/netdev/078061ce-1411-d150-893a-d0a950c8866f@kernel.org/