linux-kernel - Re: [PATCH net 1/1] net/ipv6: Netlink flag for new IPv6 Default Routes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZytjEINNRmtpadr_@shredder>
Date: Wed, 6 Nov 2024 14:37:36 +0200
From: Ido Schimmel <idosch@...sch.org>
To: Matt Muggeridge <Matt.Muggeridge@....com>
Cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com,
	horms@...nel.org, kuba@...nel.org, linux-api@...r.kernel.org,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	pabeni@...hat.com, stable@...r.kernel.org
Subject: Re: [PATCH net 1/1] net/ipv6: Netlink flag for new IPv6 Default
 Routes

On Tue, Nov 05, 2024 at 09:50:56PM -0500, Matt Muggeridge wrote:
> Thank you for your review and feedback, Ido.
> 
> >> Without this flag, when there are mutliple default routers, the kernel
> >> coalesces multiple default routes into an ECMP route. The ECMP route
> >> ignores per-route REACHABILITY information. If one of the default
> >> routers is unresponsive, with a Neighbor Cache entry of INCOMPLETE, then
> >> it can still be selected as the nexthop for outgoing packets. This
> >> results in an inability to communicate with remote hosts, even though
> >> one of the default routers remains REACHABLE. This violates RFC4861
> >> section 6.3.6, bullet 1.
> >
> >Do you have forwarding disabled (it causes RT6_LOOKUP_F_REACHABLE to be
> >set)?
> 
> Yes, forwarding is disabled on our embedded system. Though, this needs to
> work on systems regardless of the state of forwarding.
> 
> >  Is the problem that fib6_table_lookup() chooses a reachable
> >nexthop and then fib6_select_path() overrides it with an unreachable
> >one?
> 
> I'm afraid I don't know.

We need to understand the current behavior before adding a new interface
that we will never be able to remove. It is possible we can improve /
fix the current code. I won't have time to look into it myself until
next week.

> 
> The objective is to allow IPv6 Netlink clients to be able to create default
> routes from RAs in the same way the kernel creates default routes from RAs.
> Essentially, I'm trying to have Netlink and Kernel behaviors match.

I understand, but it's essentially an extension for the legacy IPv6
multipath API which we are trying to move away from towards the nexthop
API (see more below).

> 
> My analysis led me to the need for Netlink clients to set the kernel's
> fib6_config flags RTF_RA_ROUTER, where:
> 
>     #define RTF_RA_ROUTER		(RTF_ADDRCONF | RTF_DEFAULT)
> 
> >> +	if (rtm->rtm_flags & RTM_F_RA_ROUTER)
> >> +		cfg->fc_flags |= RTF_RA_ROUTER;
> >> +
> > 
> > It is possible there are user space programs out there that set this bit
> > (knowingly or not) when sending requests to the kernel and this change
> > will result in a behavior change for them. So, if we were to continue in
> > this path, this would need to be converted to a new netlink attribute to
> > avoid such potential problems.
> > 
> 
> Is this a mandated approach to implementing unspecified bits in a flag?
> 
> I'm a little surprised by this consideration. If we account for poorly
> written buggy user-programs, doesn't this open any API to an explosion
> of new attributes or other odd extensions? I'd imagine the same argument
> would be applicable to ioctl flags, socket flags, and so on. Why would we
> treat implementing unspecified Netlink bits differently to implementing
> unspecified ioctl bits, etc.
> 
> Naturally, if this is the mandated approach, then I'll reimplement it with
> a new Netlink attribute. I'm just trying to understand what is the
> Linux-lore, here?

Using this bit could have been valid if previously the kernel rejected
requests with this bit set, but as evident by your patch the kernel does
not do it. It is therefore possible that there are user space programs
out there that are working perfectly fine right now and they will break
/ misbehave after this change.

> 
> > BTW, you can avoid the coalescing problem by using the nexthop API (man
> > ip-nexthop).
> 
> I'm not sure how that would help in this case. We need the nexthop to be
> determined according to its REACHABILITY and other considerations described
> in RFC4861.

Using your example:

# ip nexthop add id 1 via fe80::200:10ff:fe10:1060 dev enp0s9
# ip -6 route add default nhid 1 expires 600 proto ra
# ip nexthop add id 2 via fe80::200:10ff:fe10:1061 dev enp0s9
# ip -6 route append default nhid 2 expires 600 proto ra
# ip -6 route
fe80::/64 dev enp0s9 proto kernel metric 256 pref medium
default nhid 1 via fe80::200:10ff:fe10:1060 dev enp0s9 proto ra metric 1024 expires 563sec pref medium
default nhid 2 via fe80::200:10ff:fe10:1061 dev enp0s9 proto ra metric 1024 expires 594sec pref medium