Re: [PATCH] netlink: Fix selection of template interface

[Paul Holzinger <pholzing@redhat.com>]

On 20/03/2024 12:02, Stefano Brivio wrote:
> On Wed, 20 Mar 2024 11:51:59 +0100
> Paul Holzinger <pholzing@redhat.com> wrote:
>
>> On 20/03/2024 06:33, David Gibson wrote:
>>> Since f919dc7a4b1c ("conf, netlink: Don't require a default route to
>>> start"), if there is only one host interface with routes, we will pick that
>>> as the template interface, even if there are no default routes for an IP
>>> version.  Unfortunately this selection had a serious flaw: in some cases
>>> it would 'return' in the middle of an nl_foreach() loop, meaning we
>>> wouldn't consume all the netlink responses for our query.  This could cause
>>> later netlink operations to fail as we read leftover responses from the
>>> aborted query.
>>>
>>> Rewrite the interface detection to avoid this problem.  While we're there:
>>>     * Perform detection of both default and non-default routes in a single
>>>       pass, avoiding an ugly goto
>>>     * Give more detail on error and working but unusual paths about the
>>>       situation (no suitable interface, multiple possible candidates, etc.).
>>>
>>> Fixes: f919dc7a4b1c ("conf, netlink: Don't require a default route to start")
>>> Link: https://bugs.passt.top/show_bug.cgi?id=83
>>> Link: https://github.com/containers/podman/issues/22052
>>> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2270257
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>>    conf.c    |  4 ++--
>>>    netlink.c | 62 ++++++++++++++++++++++++++++++++++---------------------
>>>    2 files changed, 40 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/conf.c b/conf.c
>>> index 644752cc..9e0318a5 100644
>>> --- a/conf.c
>>> +++ b/conf.c
>>> @@ -584,7 +584,7 @@ static unsigned int conf_ip4(unsigned int ifi,
>>>    		ifi = nl_get_ext_if(nl_sock, AF_INET);
>>>    
>>>    	if (!ifi) {
>>> -		info("No interface with a route for IPv4: disabling IPv4");
>>> +		info("Couldn't pick external interface: disabling IPv4");
>>>    		return 0;
>>>    	}
>>>    
>>> @@ -656,7 +656,7 @@ static unsigned int conf_ip6(unsigned int ifi,
>>>    		ifi = nl_get_ext_if(nl_sock, AF_INET6);
>>>    
>>>    	if (!ifi) {
>>> -		info("No interface with a route for IPv6: disabling IPv6");
>>> +		info("Couldn't pick external interface: disabling IPv6");
>>>    		return 0;
>>>    	}
>>>    
>>> diff --git a/netlink.c b/netlink.c
>>> index 632304c1..c0a5f158 100644
>>> --- a/netlink.c
>>> +++ b/netlink.c
>>> @@ -254,8 +254,8 @@ unsigned int nl_get_ext_if(int s, sa_family_t af)
>>>    		.rtm.rtm_type	 = RTN_UNICAST,
>>>    		.rtm.rtm_family	 = af,
>>>    	};
>>> -	bool default_only = true;
>>> -	unsigned int ifi = 0;
>>> +	unsigned defifi = 0, anyifi = 0;
>>> +	unsigned ndef = 0, nany = 0;
>>>    	struct nlmsghdr *nh;
>>>    	struct rtattr *rta;
>>>    	char buf[NLBUFSIZ];
>>> @@ -263,7 +263,6 @@ unsigned int nl_get_ext_if(int s, sa_family_t af)
>>>    	uint32_t seq;
>>>    	size_t na;
>>>    
>>> -again:
>>>    	/* Look for an interface with a default route first, failing that, look
>>>    	 * for any interface with a route, and pick it only if it's the only
>>>    	 * interface with a route.
>>> @@ -271,46 +270,61 @@ again:
>>>    	seq = nl_send(s, &req, RTM_GETROUTE, NLM_F_DUMP, sizeof(req));
>>>    	nl_foreach_oftype(nh, status, s, buf, seq, RTM_NEWROUTE) {
>>>    		struct rtmsg *rtm = (struct rtmsg *)NLMSG_DATA(nh);
>>> +		unsigned thisifi = 0;
>>>    
>>> -		if (default_only) {
>>> -			if (ifi || rtm->rtm_dst_len || rtm->rtm_family != af)
>>> -				continue;
>>> -		} else {
>>> -			if (rtm->rtm_family != af)
>>> -				continue;
>>> -		}
>>> +		if (rtm->rtm_family != af)
>>> +			continue;
>>>    
>>>    		for (rta = RTM_RTA(rtm), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
>>>    		     rta = RTA_NEXT(rta, na)) {
>>>    			if (rta->rta_type == RTA_OIF) {
>>> -				if (!default_only && ifi &&
>>> -				    ifi != *(unsigned int *)RTA_DATA(rta))
>>> -					return 0;
>>> -
>>> -				ifi = *(unsigned int *)RTA_DATA(rta);
>>> +				thisifi = *(unsigned int *)RTA_DATA(rta);
>>>    			} else if (rta->rta_type == RTA_MULTIPATH) {
>>>    				const struct rtnexthop *rtnh;
>>>    
>>>    				rtnh = (struct rtnexthop *)RTA_DATA(rta);
>>> +				thisifi = rtnh->rtnh_ifindex;
>>> +			}
>>> +		}
>>>    
>>> -				if (!default_only && ifi &&
>>> -				    (int)ifi != rtnh->rtnh_ifindex)
>>> -					return 0;
>>> +		if (!thisifi)
>>> +			continue; /* No interface for this route */
>>>    
>>> -				ifi = rtnh->rtnh_ifindex;
>>> -			}
>>> +		if (rtm->rtm_dst_len == 0) {
>>> +			/* Default route */
>>> +			ndef++;
>>> +			if (!defifi)
>>> +				defifi = thisifi;
>>> +		} else {
>>> +			/* Non-default route */
>>> +			nany++;
>>> +			if (!anyifi)
>>> +				anyifi = thisifi;
>>>    		}
>>>    	}
>>>    
>>>    	if (status < 0)
>>>    		warn("netlink: RTM_GETROUTE failed: %s", strerror(-status));
>>>    
>>> -	if (!ifi && default_only) {
>>> -		default_only = false;
>>> -		goto again;
>>> +	if (defifi) {
>>> +		if (ndef > 1)
>>> +			info("Multiple default %s routes, picked first",
>>> +			     af == AF_INET ? "IPv4" : "IPv6");
>>> +		return defifi;
>>>    	}
>>>    
>>> -	return ifi;
>>> +	if (anyifi) {
>>> +		if (nany == 1)
>>> +			return anyifi;
>>> +
>>> +		warn("Multiple interfaces with %s routes, use -i to select one",
>>> +		     af == AF_INET ? "IPv4" : "IPv6");
>> This should not be a warning, for me this always triggers because I have
>> two interfaces with link local addresses and no global ipv6 route as I
>> do not have any ipv6 connection.
> I was about to reply as I just applied this with s/warn/info/ here :)
The more I think about this I think this suggestion is just wrong. I 
have a valid ipv4 connection and no ipv6, offering me to use -i is just 
wrong as it will not help me, especially if one uses a different 
interface for ipv4/ipv6 connections.
>
>> Or maybe the correct fix is to never consider ipv6 link local routes for
>> this logic? At least I cannot see the purpose of using a interface with
>> only a link local route.
>>
>>> +	}
>>> +
>>> +	if (!nany)
>>> +		warn("No interfaces with %s routes", af == AF_INET ? "IPv4" : "IPv6");
> ...and here, because if one has no IPv6 routes we would reintroduce the
> issue we just fixed in 338b6321ac0d ("conf: No routable interface for
> IPv4 or IPv6 is informational, not a warning").
>
> I think the purpose of picking interfaces based on routes for
> link-local destinations is for practical test setups like the one
> described in https://github.com/containers/podman/issues/21896.
>
> Functionally it doesn't make sense, but it shouldn't harm either
> (right?).

I am not sure how the issue is related to link local addresses, for ipv6 
a link local route must explicitly never be routed anywhere else besides 
the current interface so picking this seems incorrect as it will be 
unusable not matter what.

>
>>> +
>>> +	return 0;
>>>    }
>>>    
>>>    /**
>> I reproduced by having a second interface and confirm this patch fixes it.
> Thanks for checking! Let's hope that was the case in the failing test.
>