Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAEf4Bzb2_THiWkqNRnbN5LsOif6+9=GY7LrtEbQf6o24cihhMQ@mail.gmail.com>
 <20230609093625.727490-1-zhangmingyi5@huawei.com> <CAKH8qBu0AiB_0SZgU5N8EOmm4=hp5BRe=Yp5PHbyT1ZbRjdeOw@mail.gmail.com>
In-Reply-To: <CAKH8qBu0AiB_0SZgU5N8EOmm4=hp5BRe=Yp5PHbyT1ZbRjdeOw@mail.gmail.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Fri, 9 Jun 2023 11:23:50 -0700
Message-ID: <CAEf4BzZdpL3k5G4kY2t0xPs2YALz45O+MEmY672b409CvfL4NQ@mail.gmail.com>
Subject: Re: [PATCH] libbpf:fix use empty function pointers in ringbuf_poll
To:     Stanislav Fomichev <sdf@google.com>
Cc:     zhangmingyi <zhangmingyi5@huawei.com>, andrii@kernel.org,
        ast@kernel.org, bpf@vger.kernel.org, daniel@iogearbox.net,
        davem@davemloft.net, edumazet@google.com, hsinweih@uci.edu,
        jakub@cloudflare.com, john.fastabend@gmail.com,
        kongweibin2@huawei.com, kuba@kernel.org,
        linux-kernel@vger.kernel.org, liuxin350@huawei.com,
        netdev@vger.kernel.org, pabeni@redhat.com,
        syzbot+49f6cef45247ff249498@syzkaller.appspotmail.com,
        syzkaller-bugs@googlegroups.com, wuchangye@huawei.com,
        xiesongyang@huawei.com, yanan@huawei.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 9, 2023 at 9:55=E2=80=AFAM Stanislav Fomichev <sdf@google.com> =
wrote:
>
> On Fri, Jun 9, 2023 at 2:38=E2=80=AFAM zhangmingyi <zhangmingyi5@huawei.c=
om> wrote:
> >
> > On Fri, Jun 9, 2023 at 1:39 AM Andrii Nakryiko <andrii.nakryiko@gmail.c=
om> wrote:
> >
> > > On Thu, Jun 8, 2023 at 9:27=E2=80=AFAM Stanislav Fomichev <sdf@google=
.com> wrote:
> > > >
> > > > On Thu, Jun 8, 2023 at 6:00=E2=80=AFAM zhangmingyi <zhangmingyi5@hu=
awei.com> wrote:
> > > > >
> > > > > On 06/06,Stanislav Fomichev wrote:
> > > > >
> > > > > > On 06/05, Xin Liu wrote:
> > > > > > > From: zhangmingyi <zhangmingyi5@huawei.com>
> > > > > >
> > > > > > > The sample_cb of the ring_buffer__new interface can transfer =
NULL. However,
> > > > > > > the system does not check whether sample_cb is NULL during
> > > > > > > ring_buffer__poll, null pointer is used.
> > > > >
> > > > > > What is the point of calling ring_buffer__new with sample_cb =
=3D=3D NULL?
> > > > >
> > > > > Yes, as you said, passing sample_cb in ring_buffer__new to NULL d=
oesn't
> > > > > make sense, and few people use it that way, but that doesn't prev=
ent this
> > > > > from being a allowed and supported scenario. And when ring_buffer=
__poll is
> > > > > called, it leads to a segmentation fault (core dump), which I thi=
nk needs
> > > > > to be fixed to ensure the security quality of libbpf.
> > > >
> > > > I dunno. I'd argue that passing a NULL to ring_buffer__new is an AP=
I
> > > > misuse. Maybe ring_buffer__new should return -EINVAL instead when
> > > > passed NULL sample_cb? Although, we don't usually have those checks
> > > > for the majority of the arguments in libbpf...
> > >
> > > Right. I'd say we should add a proper doc comment specifying all
> > > arguments and which ones are optional or not. And make it explicit
> > > that callback is not optional. If we start checking every possible
> > > pointer for NULL, libbpf will be littered with NULL checks, I'm not
> > > sure that's good.
> >
> > I agree, we should add a proper doc comment specifying all
> > arguments and which ones are optional or not.
> > However, why does the external interface API in libbpf not verify input
> > parameters or add verification where risky operations may exist?
> > What's more, i think sample_cb=3DNULL is not strictly a mistake or
> > prohibited use, and is meaningless.
>
> It's not really customary in C to do it? So maybe you can follow up
> with the update to the doc?

Yep, we do not check every `struct bpf_object *` pointer to be non-NULL.

Having said that, I don't think it's such a big deal to make this
callback optional by assigning a no-op callback.

So let's definitely update doc comments to be explicit about one way
or the other. For the callback, let's just not do it on every record.
Just once during initialization would be better.

>
> The kindergarten is over, you pass NULL you get SIGSEGV :-D