Date: Fri, 13 Oct 2023 18:37:47 +0200 From: Kristof Provost <kp@FreeBSD.org> To: "Alexander V. Chernikov" <melifaro@FreeBSD.org> Cc: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 5f19f790b392 - main - netlink: add snl(3) support for parsing unknown-size arrays Message-ID: <8CFADE61-3604-4915-9AE2-F52E6C17DB6E@FreeBSD.org> In-Reply-To: <202305271114.34RBED8O007212@gitrepo.freebsd.org> References: <202305271114.34RBED8O007212@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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=3D5f19f790b3923354871ce049= b84c7807ecb0cad5 > > 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 !=3D NULL) { > int hdr_off =3D (char *)(nw->hdr) - nw->base; > + > nw->hdr =3D (struct nlmsghdr *)(void *)((char *)new_base + hdr_off);= I=E2=80=99ve been working on converting a few more pf ioctls to using net= link, and ran into an odd failure once the request got big enough. My req= uests all resulted in =E2=80=9CNo buffer space available=E2=80=9D. That t= urned 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=E2=80=99re re-allocating the header here, = but I was still using the old struct nlmsghdr *hdr in the calling functio= n. That seems to be a pattern in other netlink code as well, e.g. https://cg= it.reebsd.org/src/tree/sbin/route/route_netlink.c#n269 where we get the s= truct nlmsghdr *hdr when we create the request. We seem to be getting away with that here, because most requests are smal= l. Still, I think we should at the very least fix that so it doesn=E2=80=99= t mislead others. Ideally I think we should avoid returning pointers that will unexpectedly= be invalidated. That is, I think we shouldn=E2=80=99t allow callers to a= ccess struct snl_writer.hdr in the first place. (Also, I don=E2=80=99t immediately see where we free the old memory. Are = we leaking that or am I missing something?) Best regards, Kristof
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8CFADE61-3604-4915-9AE2-F52E6C17DB6E>