[PATCH] netlink: For IPv4, IFA_LOCAL is the interface address, not IFA_ADDRESS

[PATCH] netlink: For IPv4, IFA_LOCAL is the interface address, not IFA_ADDRESS

[Stefano Brivio]

See the comment to the unnamed enum in linux/if_addr.h, which
currently states:

  /*
   * Important comment:
   * IFA_ADDRESS is prefix address, rather than local interface address.
   * It makes no difference for normally configured broadcast interfaces,
   * but for point-to-point IFA_ADDRESS is DESTINATION address,
   * local address is supplied in IFA_LOCAL attribute.
   *
   * [...]
   */

if we fetch IFA_ADDRESS, and we have a point-to-point link with a peer
address configured, we'll source the peer address as "our" address,
and refuse to resolve it in arp().

This was reported with pasta and a tun upstream interface configured
by OpenVPN in "p2p" topology: the target namespace will have similar
addresses and routes as the host, which is fine, and will try to
resolve the point-to-point peer address (because it's the default
gateway).

Given that we configure it as our address (only internally, not for
visibly in the namespace), we'll fail to resolve that and traffic
doesn't go anywhere.

Note that this is not the case for IPv6: there, IFA_ADDRESS is the
actual, local address of the interface, and IFA_LOCAL is not
necessarily present, so the comment in linux/if_addr.h doesn't apply
either.

Link: https://github.com/containers/podman/issues/22320
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 netlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/netlink.c b/netlink.c
index 89c0641..447fea2 100644
--- a/netlink.c
+++ b/netlink.c
@@ -668,7 +668,8 @@ int nl_addr_get(int s, unsigned int ifi, sa_family_t af,
 
 		for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh); RTA_OK(rta, na);
 		     rta = RTA_NEXT(rta, na)) {
-			if (rta->rta_type != IFA_ADDRESS)
+			if ((af == AF_INET  && rta->rta_type != IFA_LOCAL) ||
+			    (af == AF_INET6 && rta->rta_type != IFA_ADDRESS))
 				continue;
 
 			if (af == AF_INET && ifa->ifa_prefixlen > prefix_max) {
-- 
@@ -668,7 +668,8 @@ int nl_addr_get(int s, unsigned int ifi, sa_family_t af,
 
 		for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh); RTA_OK(rta, na);
 		     rta = RTA_NEXT(rta, na)) {
-			if (rta->rta_type != IFA_ADDRESS)
+			if ((af == AF_INET  && rta->rta_type != IFA_LOCAL) ||
+			    (af == AF_INET6 && rta->rta_type != IFA_ADDRESS))
 				continue;
 
 			if (af == AF_INET && ifa->ifa_prefixlen > prefix_max) {
-- 
2.43.0

Re: [PATCH] netlink: For IPv4, IFA_LOCAL is the interface address, not IFA_ADDRESS

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2396 bytes --]

On Thu, Apr 25, 2024 at 07:29:43AM +0200, Stefano Brivio wrote:
> See the comment to the unnamed enum in linux/if_addr.h, which
> currently states:
> 
>   /*
>    * Important comment:
>    * IFA_ADDRESS is prefix address, rather than local interface address.
>    * It makes no difference for normally configured broadcast interfaces,
>    * but for point-to-point IFA_ADDRESS is DESTINATION address,
>    * local address is supplied in IFA_LOCAL attribute.
>    *
>    * [...]
>    */
> 
> if we fetch IFA_ADDRESS, and we have a point-to-point link with a peer
> address configured, we'll source the peer address as "our" address,
> and refuse to resolve it in arp().
> 
> This was reported with pasta and a tun upstream interface configured
> by OpenVPN in "p2p" topology: the target namespace will have similar
> addresses and routes as the host, which is fine, and will try to
> resolve the point-to-point peer address (because it's the default
> gateway).
> 
> Given that we configure it as our address (only internally, not for
> visibly in the namespace), we'll fail to resolve that and traffic
> doesn't go anywhere.
> 
> Note that this is not the case for IPv6: there, IFA_ADDRESS is the
> actual, local address of the interface, and IFA_LOCAL is not
> necessarily present, so the comment in linux/if_addr.h doesn't apply
> either.

Huh.  Weird.

> Link: https://github.com/containers/podman/issues/22320
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  netlink.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/netlink.c b/netlink.c
> index 89c0641..447fea2 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -668,7 +668,8 @@ int nl_addr_get(int s, unsigned int ifi, sa_family_t af,
>  
>  		for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh); RTA_OK(rta, na);
>  		     rta = RTA_NEXT(rta, na)) {
> -			if (rta->rta_type != IFA_ADDRESS)
> +			if ((af == AF_INET  && rta->rta_type != IFA_LOCAL) ||
> +			    (af == AF_INET6 && rta->rta_type != IFA_ADDRESS))
>  				continue;
>  
>  			if (af == AF_INET && ifa->ifa_prefixlen > prefix_max) {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]