From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 99CB55A0271 for ; Mon, 18 Mar 2024 04:18:33 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1710731909; bh=jgjQ1PIMrFduAuWbdXo8bQrMlIMny25uYPueoJ8Ciy4=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CWGW4mClxlfqel3u1lHYPviuFokJY5XNDmec2MRVKaxio6pE6ZupZ12W4FtxrBdMg Au64pnemL21ue+XOmJ/DfCwKC5DSL0DYqiAeSCWvmp37VeQcPvpXsWiKVy5VSHbMXd kXXB5m4X+p3IAD9H+B3H14ZknrPS8Ddv7cGvz5wnR1Z9mqfMnQ4WYXhvFNrspE8bOG 4toZAQlLaSIqMjcVDb7hMa6Yq1MBiiYyvA35WKbqiRZ8lTAozsjPPhiYUBhOtlUxeo ulWIQhWRzyb6i0ohp6dsC23wZjVlJWHk5Vzzba481AbyBDslXO6koJYK+HO/hT5X9F y4Fs9yTLWOyGg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Tyg6Y486cz4wcK; Mon, 18 Mar 2024 14:18:29 +1100 (AEDT) Date: Mon, 18 Mar 2024 14:16:48 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH] netlink: Don't try to get further datagrams in nl_route_dup() on NLMSG_DONE Message-ID: References: <20240315112432.382212-1-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="4/tv7os8ezJOBaPJ" Content-Disposition: inline In-Reply-To: <20240315112432.382212-1-sbrivio@redhat.com> Message-ID-Hash: DCDFW6QTQTYNC5NL7NSQ4E3H355VCFSO X-Message-ID-Hash: DCDFW6QTQTYNC5NL7NSQ4E3H355VCFSO X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Martin Pitt , Paul Holzinger X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --4/tv7os8ezJOBaPJ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > 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 On the argument above. I do think we need to check over the netlink code to handle this more carefully, though. >=20 > Reported-by: Martin Pitt > Link: https://github.com/containers/podman/issues/22052 > Fixes: 4d6e9d0816e2 ("netlink: Always process all responses to a netlink = request") > Signed-off-by: Stefano Brivio > --- > netlink.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) >=20 > 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, > } > } > =20 > - if (!NLMSG_OK(nh, status) || status > 0) { > + if (nh->nlmsg_type !=3D NLMSG_DONE && > + (!NLMSG_OK(nh, status) || status > 0)) { > /* Process any remaining datagrams in a different > * buffer so we don't overwrite the first one. > */ --=20 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 --4/tv7os8ezJOBaPJ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmX3sh8ACgkQzQJF27ox 2Gck1g//a+55sSxzeTUznjsSirB5SS74TscfVvvDhhJRDqBeA6TxCiW9EM5Xtmac 529Dtp+RsK5/MD/pDmTnHnxvT4L9zWFXbRvqoqKGhYu0OmWxsQP9WocyJqhobpnt 5+Tk4mCayHMo9Y+awpnlpnlFkk/l3TSYYkFYnE7CZB8YhQcuPx3MkkdpZn/bQLLr g+EQIAtpVp5pAFuQ/fiO6Rw8+WiTFGxJdyjxyMfiEm98ik3/NE5iw/MX8RLectAp YuG+eBFsJTvgI2Olr2U8I/F0P0BTNoUKK5eYwD0yhHHVOpoEwaXUP5vNfsPVI4GU Do17v086P7Y4luRk407/qpMbaCK0hw7Iy1PnsoGhEQgpbVR7oYxZpovvXFr84ubh DH5GDbgM0imZnWuChvDXbeNJXYNhXg1Gt9lhBuF72lZFYf+z1doHKS6+t0wubd4y /ADOzQOAfJTTE2nE/0irJaR1CS76Ktz20oO04nWy486K+4L7AtdZ69IG6MBkN7k4 FGMyY9FkSBoxgou1BMy5ol5HOO9uJ79Cd0WkGXEYrbD40vXpe+EXruHcmuc2fgJD zjBaOskLLmG3Qk2U+3Gj6KWHL61+Fn96ejR6fcz9eB5cSuxdaENFB56uTuXbDL/6 Fny/CJBiO7VITOt7vNxA7ihKNbtmw8vpmTU1u2mgQQAqsVLDxOI= =Aa9C -----END PGP SIGNATURE----- --4/tv7os8ezJOBaPJ--