Re: git: 5f19f790b392 - main - netlink: add snl(3) support for parsing unknown-size arrays

On 27 May 2023, at 13:14, Alexander V. Chernikov wrote:
> The branch main has been updated by melifaro:
>
> URL: https://cgit.FreeBSD.org/src/commit/?id=5f19f790b3923354871ce049b84c7807ecb0cad5
>
> commit 5f19f790b3923354871ce049b84c7807ecb0cad5
> Author:     Alexander V. Chernikov <melifaro@FreeBSD.org>
> AuthorDate: 2023-05-27 11:09:01 +0000
> Commit:     Alexander V. Chernikov <melifaro@FreeBSD.org>
> CommitDate: 2023-05-27 11:13:14 +0000
>
>     netlink: add snl(3) support for parsing unknown-size arrays
>
>     Reviewed by:    bapt
>     Differential Review: https://reviews.freebsd.org/D40282
>     MFC after:      2 weeks
> ---
<snip>
>  static inline bool
>  snl_attr_get_nla(struct snl_state *ss __unused, struct nlattr *nla,
>      const void *arg __unused, void *target)
> @@ -878,6 +959,7 @@ snl_realloc_msg_buffer(struct snl_writer *nw, size_t sz)
>  	memcpy(new_base, nw->base, nw->offset);
>  	if (nw->hdr != NULL) {
>  		int hdr_off = (char *)(nw->hdr) - nw->base;
> +
>  		nw->hdr = (struct nlmsghdr *)(void *)((char *)new_base + hdr_off);

I’ve been working on converting a few more pf ioctls to using netlink, and ran into an odd failure once the request got big enough. My requests all resulted in “No buffer space available”. That turned out to be because they were of size zero. Some more digging showed that the struct nlmsghdr *hdr pointer returned by snl_finalize_msg() was different from the one returned by snl_create_msg_request().

I believe the issue is that we’re re-allocating the header here, but I was still using the old struct nlmsghdr *hdr in the calling function.

That seems to be a pattern in other netlink code as well, e.g. https://cgit.reebsd.org/src/tree/sbin/route/route_netlink.c#n269 where we get the struct nlmsghdr *hdr when we create the request.
We seem to be getting away with that here, because most requests are small. Still, I think we should at the very least fix that so it doesn’t mislead others.

Ideally I think we should avoid returning pointers that will unexpectedly be invalidated. That is, I think we shouldn’t allow callers to access struct snl_writer.hdr in the first place.

(Also, I don’t immediately see where we free the old memory. Are we leaking that or am I missing something?)

Best regards,
Kristof