Re: [PATCH bpf-next] libbpf: Use dynamically allocated buffer when receiving netlink messages

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Fri, Feb 11, 2022 at 3:37 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:
>
> > On Fri, Feb 11, 2022 at 11:51 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> When receiving netlink messages, libbpf was using a statically allocated
> >> stack buffer of 4k bytes. This happened to work fine on systems with a 4k
> >> page size, but on systems with larger page sizes it can lead to truncated
> >> messages. The user-visible impact of this was that libbpf would insist no
> >> XDP program was attached to some interfaces because that bit of the netlink
> >> message got chopped off.
> >>
> >> Fix this by switching to a dynamically allocated buffer; we borrow the
> >> approach from iproute2 of using recvmsg() with MSG_PEEK|MSG_TRUNC to get
> >> the actual size of the pending message before receiving it, adjusting the
> >> buffer as necessary. While we're at it, also add retries on interrupted
> >> system calls around the recvmsg() call.
> >>
> >> Reported-by: Zhiqian Guan <zhguan@redhat.com>
> >> Fixes: 8bbb77b7c7a2 ("libbpf: Add various netlink helpers")
> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >> ---
> >>  tools/lib/bpf/netlink.c | 55 ++++++++++++++++++++++++++++++++++++++---
> >>  1 file changed, 52 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/tools/lib/bpf/netlink.c b/tools/lib/bpf/netlink.c
> >> index c39c37f99d5c..9a6e95206bf0 100644
> >> --- a/tools/lib/bpf/netlink.c
> >> +++ b/tools/lib/bpf/netlink.c
> >> @@ -87,22 +87,70 @@ enum {
> >>         NL_DONE,
> >>  };
> >>
> >> +static int __libbpf_netlink_recvmsg(int sock, struct msghdr *mhdr, int flags)
> >
> > let's not use names starting with underscored. Just call it
> > "netlink_recvmsg" or something like that.
>
> Alright, will fix.
>
> >> +{
> >> +       int len;
> >> +
> >> +       do {
> >> +               len = recvmsg(sock, mhdr, flags);
> >
> > recvmsg returns ssize_t, is it ok to truncate to int?
>
> In practice, yeah; the kernel is not going to return a single message
> that overflows an int, even on 32bit. And with an int return type it's
> more natural to return -errno instead of having the caller deal with
> that. So unless you have strong objections I'd prefer to keep it this
> way...

yep, int is fine

>
> >> +       } while (len < 0 && (errno == EINTR || errno == EAGAIN));
> >> +
> >> +       if (len < 0)
> >> +               return -errno;
> >> +       return len;
> >> +}
> >> +
> >> +static int libbpf_netlink_recvmsg(int sock, struct msghdr *mhdr, char **buf)
> >> +{
> >> +       struct iovec *iov = mhdr->msg_iov;
> >> +       void *nbuf;
> >> +       int len;
> >> +
> >> +       len = __libbpf_netlink_recvmsg(sock, mhdr, MSG_PEEK | MSG_TRUNC);
> >> +       if (len < 0)
> >> +               return len;
> >> +
> >> +       if (len < 4096)
> >> +               len = 4096;
> >> +
> >> +       if (len > iov->iov_len) {
> >> +               nbuf = realloc(iov->iov_base, len);
> >> +               if (!nbuf) {
> >> +                       free(iov->iov_base);
> >> +                       return -ENOMEM;
> >> +               }
> >> +               iov->iov_base = nbuf;
> >
> > this function both sets iov->iov_base *and* returns buf. It's quite a
> > convoluted contract. Seems like buf is not necessary (and also NULL
> > out iov->iov_base in case of error above?). But it might be cleaner to
> > do this MSG_PEEK  + realloc + recvmsg  in libbpf_netlink_recv()
> > explicitly. It's only one place.
>
> Hmm, yeah, if I wrap the realloc code in a small helper that works; will
> fix.
>
> -Toke
>