Re: [PATCH] netlink: Don't try to get further datagrams in nl_route_dup() on NLMSG_DONE

[David Gibson <david@gibson.dropbear.id.au>]

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

On Fri, Mar 15, 2024 at 12:24:32PM +0100, Stefano Brivio wrote:
> Martin reports that, with Fedora Linux kernel version
> kernel-core-6.9.0-0.rc0.20240313gitb0546776ad3f.4.fc41.x86_64,
> including commit 87d381973e49 ("genetlink: fit NLMSG_DONE into same
> read() as families"), pasta doesn't exit once the network namespace
> is gone.
> 
> Actually, pasta is completely non-functional, at least with default
> options, because nl_route_dup(), which duplicates routes from the
> parent namespace into the target namespace at start-up, is stuck on
> a second receive operation for RTM_GETROUTE.
> 
> However, with that commit, the kernel is now able to fit the whole
> response, including the NLMSG_DONE message, into a single datagram,
> so no further messages will be received.
> 
> It turns out that commit 4d6e9d0816e2 ("netlink: Always process all
> responses to a netlink request") accidentally relied on the fact that
> we would always get at least two datagrams as a response to
> RTM_GETROUTE.

Huh, oops.  I feel like the expectation that NLMSG_DONE wold appear in
its own datagram was pre-existing, althogh the conseqences of it not
being so might have been different and less serious.  I recall
noticing that the code expected that, and assuming that was part of
the ABI.

> That is, the test to check if we expect another datagram, is based
> on the 'status' variable, which is 0 if we just parsed NLMSG_DONE,
> but we'll also expect another datagram if NLMSG_OK on the last
> message is false. But NLMSG_OK with a zero length is always false.
> 
> The problem is that we don't distinguish if status is zero because
> we got a NLMSG_DONE message, or because we processed all the
> available datagram bytes.
> 
> Introduce an explicit check on NLMSG_DONE. We should probably
> refactor this slightly, for example by introducing a special return
> code from nl_status(), but this is probably the least invasive fix
> for the issue at hand.

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

On the argument above.  I do think we need to check over the netlink
code to handle this more carefully, though.

> 
> Reported-by: Martin Pitt <mpitt@redhat.com>
> Link: https://github.com/containers/podman/issues/22052
> Fixes: 4d6e9d0816e2 ("netlink: Always process all responses to a netlink request")
> 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 9e7cccb..20de9b3 100644
> --- a/netlink.c
> +++ b/netlink.c
> @@ -525,7 +525,8 @@ int nl_route_dup(int s_src, unsigned int ifi_src,
>  		}
>  	}
>  
> -	if (!NLMSG_OK(nh, status) || status > 0) {
> +	if (nh->nlmsg_type != NLMSG_DONE &&
> +	    (!NLMSG_OK(nh, status) || status > 0)) {
>  		/* Process any remaining datagrams in a different
>  		 * buffer so we don't overwrite the first one.
>  		 */

-- 
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 --]