[PATCH v2] netlink: Strip nexthop identifiers when duplicating routes

[PATCH v2] netlink: Strip nexthop identifiers when duplicating routes

[Stefano Brivio]

If routing daemons set up host routes, for example FRR via OSPF as in
the reported issue, they might add nexthop identifiers (not objects)
that are generally not valid in the target namespace. Strip them off
as well, otherwise we'll get EINVAL from the kernel.

Link: https://github.com/containers/podman/issues/22960
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
v2: oops, it looks like I didn't run this through clang-tidy :( and it
    reported a bugprone-branch-clone if I have two branches both doing
    the same thing (rta->rta_type = RTA_UNSPEC). I condensed comments
    under the same branch, probably more elegant than carrying around
    yet another suppression.

 netlink.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/netlink.c b/netlink.c
index 2c9e71f..c082991 100644
--- a/netlink.c
+++ b/netlink.c
@@ -600,13 +600,22 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
 
 				if (discard)
 					break;
-			} else if (rta->rta_type == RTA_PREFSRC) {
-				/* Host routes might include a preferred source
-				 * address, which must be one of the host's
-				 * addresses.  However, with -a pasta will use a
-				 * different namespace address, making such a
-				 * route invalid in the namespace.  Strip off
-				 * RTA_PREFSRC attributes to avoid that. */
+			} else if (rta->rta_type == RTA_PREFSRC ||
+				   rta->rta_type == RTA_NH_ID) {
+				/* Strip RTA_PREFSRC attributes: host routes
+				 * might include a preferred source address,
+				 * which must be one of the host's addresses.
+				 * However, with -a, pasta will use a different
+				 * namespace address, making such a route
+				 * invalid in the namespace.
+				 *
+				 * Strip RTA_NH_ID attributes: host routes set
+				 * up via routing protocols (e.g. OSPF) might
+				 * contain a nexthop ID (and not nexthop
+				 * objects, which are taken care of in the
+				 * RTA_MULTIPATH case above) that's not valid
+				 * in the target namespace.
+				 */
 				rta->rta_type = RTA_UNSPEC;
 			}
 		}
-- 
@@ -600,13 +600,22 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
 
 				if (discard)
 					break;
-			} else if (rta->rta_type == RTA_PREFSRC) {
-				/* Host routes might include a preferred source
-				 * address, which must be one of the host's
-				 * addresses.  However, with -a pasta will use a
-				 * different namespace address, making such a
-				 * route invalid in the namespace.  Strip off
-				 * RTA_PREFSRC attributes to avoid that. */
+			} else if (rta->rta_type == RTA_PREFSRC ||
+				   rta->rta_type == RTA_NH_ID) {
+				/* Strip RTA_PREFSRC attributes: host routes
+				 * might include a preferred source address,
+				 * which must be one of the host's addresses.
+				 * However, with -a, pasta will use a different
+				 * namespace address, making such a route
+				 * invalid in the namespace.
+				 *
+				 * Strip RTA_NH_ID attributes: host routes set
+				 * up via routing protocols (e.g. OSPF) might
+				 * contain a nexthop ID (and not nexthop
+				 * objects, which are taken care of in the
+				 * RTA_MULTIPATH case above) that's not valid
+				 * in the target namespace.
+				 */
 				rta->rta_type = RTA_UNSPEC;
 			}
 		}
-- 
2.43.0

Re: [PATCH v2] netlink: Strip nexthop identifiers when duplicating routes

[David Gibson]

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

On Wed, Jun 19, 2024 at 06:21:47PM +0200, Stefano Brivio wrote:
> If routing daemons set up host routes, for example FRR via OSPF as in
> the reported issue, they might add nexthop identifiers (not objects)
> that are generally not valid in the target namespace. Strip them off
> as well, otherwise we'll get EINVAL from the kernel.
> 
> Link: https://github.com/containers/podman/issues/22960
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

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

> ---
> v2: oops, it looks like I didn't run this through clang-tidy :( and it
>     reported a bugprone-branch-clone if I have two branches both doing
>     the same thing (rta->rta_type = RTA_UNSPEC). I condensed comments
>     under the same branch, probably more elegant than carrying around
>     yet another suppression.
> 
>  netlink.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/netlink.c b/netlink.c
> index 2c9e71f..c082991 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -600,13 +600,22 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
>  
>  				if (discard)
>  					break;
> -			} else if (rta->rta_type == RTA_PREFSRC) {
> -				/* Host routes might include a preferred source
> -				 * address, which must be one of the host's
> -				 * addresses.  However, with -a pasta will use a
> -				 * different namespace address, making such a
> -				 * route invalid in the namespace.  Strip off
> -				 * RTA_PREFSRC attributes to avoid that. */
> +			} else if (rta->rta_type == RTA_PREFSRC ||
> +				   rta->rta_type == RTA_NH_ID) {
> +				/* Strip RTA_PREFSRC attributes: host routes
> +				 * might include a preferred source address,
> +				 * which must be one of the host's addresses.
> +				 * However, with -a, pasta will use a different
> +				 * namespace address, making such a route
> +				 * invalid in the namespace.
> +				 *
> +				 * Strip RTA_NH_ID attributes: host routes set
> +				 * up via routing protocols (e.g. OSPF) might
> +				 * contain a nexthop ID (and not nexthop
> +				 * objects, which are taken care of in the
> +				 * RTA_MULTIPATH case above) that's not valid
> +				 * in the target namespace.
> +				 */
>  				rta->rta_type = RTA_UNSPEC;
>  			}
>  		}

-- 
David Gibson (he or they)	| 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 --]