[PATCH 0/3] pasta: Don't propagate host address lifetimes to the container

[PATCH 0/3] pasta: Don't propagate host address lifetimes to the container

[David Gibson]

We realized in yesterday's call that podman issue 19405 could be
explained by the fact that along with other attributes we're copying
the lifetime of host addresses to the container.

Here is a fix for that bug, along with a couple of other small fixes
to the netlink code I noticed as I was making it.

Link: https://github.com/containers/podman/issues/19405
Link: https://bugs.passt.top/show_bug.cgi?id=70

David Gibson (3):
  netlink: Remove redundant check on nlmsg_type
  netlink: Correctly calculate attribute length for address messages
  netlink: Don't propagate host address expiry to the container

 netlink.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

-- 
2.41.0

[PATCH 1/3] netlink: Remove redundant check on nlmsg_type

[David Gibson]

In the loop within nl_addr_dup() we check and skip for any messages that
aren't of type RTM_NEWADDR.  This is a leftover that was missed in the
recent big netlink cleanup.  In fact we already check for the message type
in the nl_foreach_oftype() macro, so the explicit test is redudant.
Remove it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 netlink.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/netlink.c b/netlink.c
index 1226379..ff44e13 100644
--- a/netlink.c
+++ b/netlink.c
@@ -669,9 +669,6 @@ int nl_addr_dup(int s_src, unsigned int ifi_src,
 		struct rtattr *rta;
 		size_t na;
 
-		if (nh->nlmsg_type != RTM_NEWADDR)
-			continue;
-
 		ifa = (struct ifaddrmsg *)NLMSG_DATA(nh);
 
 		if (rc < 0 || ifa->ifa_scope == RT_SCOPE_LINK ||
-- 
@@ -669,9 +669,6 @@ int nl_addr_dup(int s_src, unsigned int ifi_src,
 		struct rtattr *rta;
 		size_t na;
 
-		if (nh->nlmsg_type != RTM_NEWADDR)
-			continue;
-
 		ifa = (struct ifaddrmsg *)NLMSG_DATA(nh);
 
 		if (rc < 0 || ifa->ifa_scope == RT_SCOPE_LINK ||
-- 
2.41.0

[PATCH 2/3] netlink: Correctly calculate attribute length for address messages

[David Gibson]

In nl_addr_get() and nl_addr_dup() we step the attributes attached to each
RTM_NEWADDR message with a loop initialised with IFA_RTA() and
RTM_PAYLOAD() macros.  RTM_PAYLOAD(), however is for RTM_NEWROUTE messages
(struct rtmsg), not RTM_NEWADDR messages (struct ifaddrmsg).  Consequently
it miscalculates the size and means we can skip some attributes.  Switch
to IFA_PAYLOAD() which we should be using here.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/netlink.c b/netlink.c
index ff44e13..69a5304 100644
--- a/netlink.c
+++ b/netlink.c
@@ -548,7 +548,7 @@ int nl_addr_get(int s, unsigned int ifi, sa_family_t af,
 		if (ifa->ifa_index != ifi)
 			continue;
 
-		for (rta = IFA_RTA(ifa), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
+		for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh); RTA_OK(rta, na);
 		     rta = RTA_NEXT(rta, na)) {
 			if (rta->rta_type != IFA_ADDRESS)
 				continue;
@@ -677,7 +677,7 @@ int nl_addr_dup(int s_src, unsigned int ifi_src,
 
 		ifa->ifa_index = ifi_dst;
 
-		for (rta = IFA_RTA(ifa), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
+		for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh); RTA_OK(rta, na);
 		     rta = RTA_NEXT(rta, na)) {
 			if (rta->rta_type == IFA_LABEL)
 				rta->rta_type = IFA_UNSPEC;
-- 
@@ -548,7 +548,7 @@ int nl_addr_get(int s, unsigned int ifi, sa_family_t af,
 		if (ifa->ifa_index != ifi)
 			continue;
 
-		for (rta = IFA_RTA(ifa), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
+		for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh); RTA_OK(rta, na);
 		     rta = RTA_NEXT(rta, na)) {
 			if (rta->rta_type != IFA_ADDRESS)
 				continue;
@@ -677,7 +677,7 @@ int nl_addr_dup(int s_src, unsigned int ifi_src,
 
 		ifa->ifa_index = ifi_dst;
 
-		for (rta = IFA_RTA(ifa), na = RTM_PAYLOAD(nh); RTA_OK(rta, na);
+		for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh); RTA_OK(rta, na);
 		     rta = RTA_NEXT(rta, na)) {
 			if (rta->rta_type == IFA_LABEL)
 				rta->rta_type = IFA_UNSPEC;
-- 
2.41.0

[PATCH 3/3] netlink: Don't propagate host address expiry to the container

[David Gibson]

When we copy addresses from the host to the container in nl_addr_dup(), we
copy all the address's attributes, including IFA_CACHEINFO, which controls
the address's lifetime.  If the host address is managed by, for example,
DHCP, it will typically have a finite lifetime.

When we copy that lifetime to the pasta container, that lifetime will
remain, meaning the kernel will eventually remove the address, typically
some hours later.  The container, however, won't have the DHCP client or
whatever was managing and maintaining the address in the host, so it will
just lose connectivity.

Long term, we may want to monitor host address changes and reflect them to
the guest.  But for now, we just want to take a snapshot of the host's
address and set those in the container permanently.  We can accomplish that
by stripping off the IFA_CACHEINFO attribute as we copy addresses.

Link: https://github.com/containers/podman/issues/19405
Link: https://bugs.passt.top/show_bug.cgi?id=70

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 netlink.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/netlink.c b/netlink.c
index 69a5304..f55f2c3 100644
--- a/netlink.c
+++ b/netlink.c
@@ -679,7 +679,9 @@ int nl_addr_dup(int s_src, unsigned int ifi_src,
 
 		for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh); RTA_OK(rta, na);
 		     rta = RTA_NEXT(rta, na)) {
-			if (rta->rta_type == IFA_LABEL)
+			/* Strip label and expiry (cacheinfo) information */
+			if (rta->rta_type == IFA_LABEL ||
+			    rta->rta_type == IFA_CACHEINFO)
 				rta->rta_type = IFA_UNSPEC;
 		}
 
-- 
@@ -679,7 +679,9 @@ int nl_addr_dup(int s_src, unsigned int ifi_src,
 
 		for (rta = IFA_RTA(ifa), na = IFA_PAYLOAD(nh); RTA_OK(rta, na);
 		     rta = RTA_NEXT(rta, na)) {
-			if (rta->rta_type == IFA_LABEL)
+			/* Strip label and expiry (cacheinfo) information */
+			if (rta->rta_type == IFA_LABEL ||
+			    rta->rta_type == IFA_CACHEINFO)
 				rta->rta_type = IFA_UNSPEC;
 		}
 
-- 
2.41.0

Re: [PATCH 0/3] pasta: Don't propagate host address lifetimes to the container

[Stefano Brivio]

On Tue, 15 Aug 2023 13:51:26 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> We realized in yesterday's call that podman issue 19405 could be
> explained by the fact that along with other attributes we're copying
> the lifetime of host addresses to the container.
> 
> Here is a fix for that bug, along with a couple of other small fixes
> to the netlink code I noticed as I was making it.
> 
> Link: https://github.com/containers/podman/issues/19405
> Link: https://bugs.passt.top/show_bug.cgi?id=70
> 
> David Gibson (3):
>   netlink: Remove redundant check on nlmsg_type
>   netlink: Correctly calculate attribute length for address messages
>   netlink: Don't propagate host address expiry to the container

Applied.

-- 
Stefano