Re: [PATCH] netlink: Fix handling of NLMSG_DONE in nl_route_dup()

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 19 Mar 2024 15:53:41 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> A recent kernel change 87d381973e49 ("genetlink: fit NLMSG_DONE into
> same read() as families") changed netlink behaviour so that the
> NLMSG_DONE terminating a bunch of responses can go in the same
> datagram as those responses, rather than in a separate one.
> 
> Our netlink code is supposed to handle that behaviour, and indeed does
> so for most cases, using the nl_foreach() macro.  However, there was a
> subtle error in nl_route_dup() which doesn't work with this change.
> f00b1534 ("netlink: Don't try to get further datagrams in
> nl_route_dup() on NLMSG_DONE") attempted to fix this, but has its own
> subtle error.
> 
> The problem arises because nl_route_dup(), unlike other cases doesn't
> just make a single pass through all the responses to a netlink
> request.  It needs to get all the routes, then make multiple passes
> through them.  We don't really have anywhere to buffer multiple
> datagrams, so we only support the case where all the routes fit in a
> single datagram - but we need to fail gracefully when that's not the
> case.
> 
> After receiving the first datagram of responses (with nl_next()) we
> have a first loop scanning them.  It needs to exit when either we run
> out of messages in the datagram (!NLMSG_OK()) or when we get a message
> indicating the last response (nl_status() <= 0).
> 
> What we do after the loop depends on which exit case we had.  If we
> saw the last response, we're done, but otherwise we need to receive
> more datagrams to discard the rest of the responses.
> 
> We attempt to check for that second case by re-checking NLMSG_OK(nh,
> status).  However in the got-last-response case, we've altered status
> from the number of remaining bytes to the error code (usually 0). That
> means NLMSG_OK() now returns false even if it didn't during the loop
> check.  To fix this we need separate variables for the number of bytes
> left and the final status code.
> 
> We also checked status after the loop, but this was redundant: we can
> only exit the loop with NLMSG_OK() == true if status <= 0.
> 
> Reported-by: Martin Pitt <mpitt@redhat.com>
> Fixes: f00b153414b1 ("netlink: Don't try to get further datagrams in nl_route_dup() on NLMSG_DONE")
> Fixes: 4d6e9d0816e2 ("netlink: Always process all responses to a netlink request")
> Link: https://github.com/containers/podman/issues/22052
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Applied.

-- 
Stefano