From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-dy1-f179.google.com (mail-dy1-f179.google.com [74.125.82.179]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8E3D418BBAE for ; Wed, 17 Jun 2026 00:44:17 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.82.179 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781657059; cv=none; b=San6yDOaTFAcjfimHrgGd2HljcSlZxbcozk6f/6snVGH8gtnM1ldb2Qevei4bpL30D1UF07OV84ge8F76TEuFf/O/ufCTKzKBhhuxUdND5aTGOYmeNDkq8UFxYxnvs7vOJ3+uvh1qKmD6RG9Dpt0JhpzTURtTRMjss+bVNKWRhw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781657059; c=relaxed/simple; bh=DEN/111C1FkE7tXQJWjifv5zKhxGnTDUz4W0b0nsrKo=; h=Mime-Version:Content-Type:Date:Message-Id:Cc:Subject:From:To: References:In-Reply-To; b=DQwF/uTEcj+sxeYqwrpTHe6luz6yIAeZFg/32viHiunLtB8mq51sUR3tEdm4DhBhl9183KrHMymPQ35dbt1c37Px44sVQZS6uPFr8M1n7MqyDuccuzjg56Yq3zXiwemgfBKKMMBOlyk9ko2kljQ7BqZdchSLiC5kPcDrUpI1bAc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=etsalapatis.com; spf=pass smtp.mailfrom=etsalapatis.com; dkim=pass (2048-bit key) header.d=etsalapatis-com.20251104.gappssmtp.com header.i=@etsalapatis-com.20251104.gappssmtp.com header.b=fVq2QUwj; arc=none smtp.client-ip=74.125.82.179 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=etsalapatis.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=etsalapatis.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=etsalapatis-com.20251104.gappssmtp.com header.i=@etsalapatis-com.20251104.gappssmtp.com header.b="fVq2QUwj" Received: by mail-dy1-f179.google.com with SMTP id 5a478bee46e88-304df7ff4c2so297040eec.0 for ; Tue, 16 Jun 2026 17:44:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=etsalapatis-com.20251104.gappssmtp.com; s=20251104; t=1781657057; x=1782261857; darn=vger.kernel.org; h=in-reply-to:references:to:from:subject:cc:message-id:date :content-transfer-encoding:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=LQq++K17JRNcTpXKTVWZr3Aw/CAliMfIgrA0KhotXMI=; b=fVq2QUwjxVMDOQ8RXtvHRIwNcEzrY6BrwTjVWlOyrm3dvTteIPj1K0nhQ75MkeU3jr 2HdfD2//1Rc4b9K8r20bswGCwagnjvYhuqTBoRMAjM4vCMMVcecC8khNCkpg6YYusO49 sI3ziNYHTHaelHrZJC6GpWC/IXr5+CMH3CRWZ8wsLphsAuP+F5FwmmqFZ7sgZsDcTvPW 07nBZgFlH2noudLqZq5FOGZeMsTupOC4VOdmW9cqnCjuwope1l55ELqFwrcJV5VQUHTc sU2u3eD/u67bVjUDa9h3m7m3uKwT1nNSQ704IRutbFK6Dm+gwpE3tMoKQaysbwfvl4dz 9z7w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781657057; x=1782261857; h=in-reply-to:references:to:from:subject:cc:message-id:date :content-transfer-encoding:mime-version:x-gm-gg:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=LQq++K17JRNcTpXKTVWZr3Aw/CAliMfIgrA0KhotXMI=; b=sePzn8pslGG7hFK21cGtmn1qukd8YHRU1/OsM433sYGlzQL2pJXw0cCk94iEZb4avA 4i0iwT/5bvUpGNjS6sSwkZsA68ATf7EoRuKbo0B8CFPZbia/cqdNZe+YscFhrfIrDz4N NmZAp8QblFz7wcYb/WriCY3YCa2uhzhCnBffo4C4uVOkICh3W8DHf4Efx6VtzQwIRjLX zDYaVUgJSi1QxxPstoO4bDmPLcjvNYGCMfzCASIy2x612b3BCUUo+gtViRkQCmAk2c/N L67MnPmUJvQj0gfUbaKdX0ub1tuS9BYAaN/Kf1YDIboeGVvU5sMuYuFBUi2VfVBMDDz2 5byQ== X-Forwarded-Encrypted: i=1; AFNElJ926rzOn0fQzHOPiurL9vjqcz5u1DSGHwqiNyL7RslMn5OVoDXqXtryq4AwoigrZv6ApG3AEPeuAn30SkQrp0E=@vger.kernel.org X-Gm-Message-State: AOJu0YzWzX8vnQD6ey5JD1dyJ8EYGz8dXTjvaiGHZgNBS0SuPaZwsX/j 95mC8IrljFBC+1YLnUMdG7DKM48RAEX+H9fNQf1bgpy1zqCrzTM+tie8gvM80ykehpM= X-Gm-Gg: AfdE7cmcmiGTGAa5asz2snfXzE2Xf6sNtRt504AbeT3Jb8dG1LfeiGyNLhUO9503Rvr 27pdclo9HA5MjWxSBv/qVLVejlwcNg3WIwC85zQk1gMRM+9lC9Cfd0KSiMBGj2zQJKNlRQo3GKf 7P2UQ40BJruQWLyQpsJygvb7f/FglW1j7+v708Vsm42riiK7GnPH80reyLj2S1/MBWPQ8X8PwXS F/CJitgpPa3IIaxc5cDKOpBS+4iNWd8BJ4EaAqyWbsBe7WHwiYyl7bW80/bClxTv2AVJhLEmOr3 WZDCdySaYWfDs+JnpFWUdIU/ipvKjBOki5zF60Fxw92iB/3xc0WD0kcAyaK7otKD6QXU/5BI5pe 945Kb2P8uHTYlGxbuEJa28k3K/qZF2GPrT2y+Oz60f8PI8crnZjo0DFZ53g8/rE7e/GTdR64r8O gZiO0D X-Received: by 2002:a05:7301:578a:b0:2ee:7b2e:8a3e with SMTP id 5a478bee46e88-30bcee0e1e2mr582639eec.1.1781657056551; Tue, 16 Jun 2026 17:44:16 -0700 (PDT) Received: from localhost ([2620:10d:c090:600::2568]) by smtp.gmail.com with ESMTPSA id 5a478bee46e88-30b6191bf9asm15241079eec.31.2026.06.16.17.44.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 16 Jun 2026 17:44:16 -0700 (PDT) Precedence: bulk X-Mailing-List: linux-kselftest@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 16 Jun 2026 20:44:13 -0400 Message-Id: Cc: , , , "Andrew Werner" , "Zvi Effron" , "Andrii Nakryiko" Subject: Re: [PATCH bpf 2/6] libbpf: ringbuf: Prevent NULL callback crash From: "Emil Tsalapatis" To: "Tamir Duberstein" , "Alexei Starovoitov" , "Daniel Borkmann" , "Andrii Nakryiko" , "Martin KaFai Lau" , "Eduard Zingerman" , "Kumar Kartikeya Dwivedi" , "Song Liu" , "Yonghong Song" , "Jiri Olsa" , "Shuah Khan" , "Andrea Righi" , "Xu Kuohai" , "Andrea Righi" X-Mailer: aerc 0.21.0-0-g5549850facc2 References: <20260613-bpf-ringbuf-fixes-v1-0-e623481cb724@kernel.org> <20260613-bpf-ringbuf-fixes-v1-2-e623481cb724@kernel.org> In-Reply-To: <20260613-bpf-ringbuf-fixes-v1-2-e623481cb724@kernel.org> On Sat Jun 13, 2026 at 9:48 PM EDT, Tamir Duberstein wrote: > ring_buffer__new() and ring_buffer__add() allow a NULL sample > callback. When callback-based consumption reaches such a ring, it calls > through the NULL function pointer and crashes. > > Validate every ring in a manager before polling or consuming. Return > -EINVAL without consuming records from an earlier valid ring or waiting > for an event. Perform the same check before honoring a zero record bound > so invalid callback consumption consistently reports the error. > Can we just prevent a ring from being added with a NULL sample_cb? What's the use for permitting it? Even if we don't, rechecking the callbacks every single time we consume the ringbuf seems overkill. > Fixes: bf99c936f947 ("libbpf: Add BPF ring buffer support") > Assisted-by: Codex:gpt-5.5 > Signed-off-by: Tamir Duberstein > --- > tools/lib/bpf/libbpf.h | 11 ++- > tools/lib/bpf/ringbuf.c | 41 +++++++++-- > tools/testing/selftests/bpf/prog_tests/ringbuf.c | 93 ++++++++++++++++++= ++++++ > 3 files changed, 134 insertions(+), 11 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index bba4e8464396..9ba6b9ad3498 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -1526,18 +1526,17 @@ LIBBPF_API int ring__map_fd(const struct ring *r)= ; > * > * @param r A ringbuffer object. > * @return The number of records consumed (or INT_MAX, whichever is less= ), or > - * a negative number if any of the callbacks return an error. > + * a negative error code on failure. > */ > LIBBPF_API int ring__consume(struct ring *r); > =20 > /** > - * @brief **ring__consume_n()** consumes up to a requested amount of ite= ms from > - * a ringbuffer without event polling. > + * @brief **ring__consume_n()** consumes up to a requested number of rec= ords > + * from a ringbuffer without event polling. > * > * @param r A ringbuffer object. > - * @param n Maximum amount of items to consume. > - * @return The number of items consumed, or a negative number if any of = the > - * callbacks return an error. > + * @param n Maximum number of records to consume. > + * @return The number of records consumed, or a negative error code on f= ailure. > */ > LIBBPF_API int ring__consume_n(struct ring *r, size_t n); > =20 > diff --git a/tools/lib/bpf/ringbuf.c b/tools/lib/bpf/ringbuf.c > index f2bb619d5a75..ae7fa79b6217 100644 > --- a/tools/lib/bpf/ringbuf.c > +++ b/tools/lib/bpf/ringbuf.c > @@ -231,6 +231,24 @@ static inline int roundup_len(__u32 len) > return (len + 7) / 8 * 8; > } > =20 > +static int ringbuf_validate(const struct ring *r) > +{ > + return r->sample_cb ? 0 : -EINVAL; > +} > + > +static int ringbuf_validate_callbacks(const struct ring_buffer *rb) > +{ > + int i, err; > + > + for (i =3D 0; i < rb->ring_cnt; i++) { > + err =3D ringbuf_validate(rb->rings[i]); > + if (err) > + return err; > + } > + > + return 0; > +} > + > static int64_t ringbuf_process_ring(struct ring *r, size_t n) > { > int *len_ptr, len, err; > @@ -240,6 +258,9 @@ static int64_t ringbuf_process_ring(struct ring *r, s= ize_t n) > bool got_new_data; > void *sample; > =20 > + err =3D ringbuf_validate(r); > + if (err) > + return err; > if (n =3D=3D 0) > return 0; > =20 > @@ -284,14 +305,17 @@ static int64_t ringbuf_process_ring(struct ring *r,= size_t n) > * records. > * > * Returns number of records consumed across all registered ring buffers= (or > - * n, whichever is less), or negative number if any of the callbacks ret= urn > - * error. > + * n, whichever is less), or a negative error code on failure. > */ > int ring_buffer__consume_n(struct ring_buffer *rb, size_t n) > { > int64_t err, res =3D 0; > int i; > =20 > + err =3D ringbuf_validate_callbacks(rb); > + if (err) > + return libbpf_err(err); > + > for (i =3D 0; i < rb->ring_cnt; i++) { > struct ring *ring =3D rb->rings[i]; > =20 > @@ -309,14 +333,17 @@ int ring_buffer__consume_n(struct ring_buffer *rb, = size_t n) > =20 > /* Consume available ring buffer(s) data without event polling. > * Returns number of records consumed across all registered ring buffers= (or > - * INT_MAX, whichever is less), or negative number if any of the callbac= ks > - * return error. > + * INT_MAX, whichever is less), or a negative error code on failure. > */ > int ring_buffer__consume(struct ring_buffer *rb) > { > int64_t err, res =3D 0; > int i; > =20 > + err =3D ringbuf_validate_callbacks(rb); > + if (err) > + return libbpf_err(err); > + > for (i =3D 0; i < rb->ring_cnt; i++) { > struct ring *ring =3D rb->rings[i]; > =20 > @@ -334,13 +361,17 @@ int ring_buffer__consume(struct ring_buffer *rb) > =20 > /* Poll for available data and consume records, if any are available. > * Returns number of records consumed (or INT_MAX, whichever is less), o= r > - * negative number, if any of the registered callbacks returned error. > + * a negative error code on failure. > */ > int ring_buffer__poll(struct ring_buffer *rb, int timeout_ms) > { > int i, cnt; > int64_t err, res =3D 0; > =20 > + err =3D ringbuf_validate_callbacks(rb); > + if (err) > + return libbpf_err(err); > + > cnt =3D epoll_wait(rb->epoll_fd, rb->events, rb->ring_cnt, timeout_ms); > if (cnt < 0) > return libbpf_err(-errno); > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/tes= ting/selftests/bpf/prog_tests/ringbuf.c > index 4f0558f14847..9ce996bcea8c 100644 > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c > @@ -401,6 +401,97 @@ static int process_n_sample(void *ctx, void *data, s= ize_t len) > return 0; > } > =20 > +static int process_noop_sample(void *ctx, void *data, size_t len) > +{ > + return 0; > +} > + > +static void ringbuf_null_cb_subtest(void) > +{ > + struct test_ringbuf_n_lskel *skel_n; > + struct ring_buffer *ringbuf =3D NULL; > + struct ring *ring; > + unsigned long consumer_pos; > + int no_cb_map_fd =3D -1; > + int err; > + > + skel_n =3D test_ringbuf_n_lskel__open(); > + if (!ASSERT_OK_PTR(skel_n, "test_ringbuf_n_lskel__open")) > + return; > + > + skel_n->maps.ringbuf.max_entries =3D getpagesize(); > + skel_n->bss->pid =3D getpid(); > + skel_n->bss->value =3D SAMPLE_VALUE; > + > + err =3D test_ringbuf_n_lskel__load(skel_n); > + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__load")) > + goto cleanup; > + > + err =3D test_ringbuf_n_lskel__attach(skel_n); > + if (!ASSERT_OK(err, "test_ringbuf_n_lskel__attach")) > + goto cleanup; > + > + syscall(__NR_getpgid); > + > + no_cb_map_fd =3D bpf_map_create(BPF_MAP_TYPE_RINGBUF, NULL, 0, 0, > + getpagesize(), NULL); > + if (!ASSERT_OK_FD(no_cb_map_fd, "bpf_map_create")) > + goto cleanup; > + > + /* Manager APIs must validate all rings before consuming any of them. *= / > + ringbuf =3D ring_buffer__new(skel_n->maps.ringbuf.map_fd, > + process_noop_sample, NULL, NULL); > + if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new")) > + goto cleanup_fd; > + > + ring =3D ring_buffer__ring(ringbuf, 0); > + if (!ASSERT_OK_PTR(ring, "ring_buffer__ring")) > + goto cleanup_ringbuf; > + > + err =3D ring_buffer__add(ringbuf, no_cb_map_fd, NULL, NULL); > + if (!ASSERT_OK(err, "ring_buffer__add_no_cb")) > + goto cleanup_ringbuf; > + > + consumer_pos =3D ring__consumer_pos(ring); > + ASSERT_GT(ring__producer_pos(ring), consumer_pos, > + "producer_pos_mixed_cb"); > + > + err =3D ring_buffer__consume_n(ringbuf, 0); > + ASSERT_EQ(err, -EINVAL, "ringbuf_consume_zero_mixed_cb"); > + err =3D ring_buffer__consume(ringbuf); > + ASSERT_EQ(err, -EINVAL, "ringbuf_consume_mixed_cb"); > + err =3D ring_buffer__poll(ringbuf, 0); > + ASSERT_EQ(err, -EINVAL, "ringbuf_poll_mixed_cb"); > + ASSERT_EQ(ring__consumer_pos(ring), consumer_pos, > + "consumer_pos_mixed_cb"); > + > + ring_buffer__free(ringbuf); > + ringbuf =3D > + ring_buffer__new(skel_n->maps.ringbuf.map_fd, NULL, NULL, NULL); > + if (!ASSERT_OK_PTR(ringbuf, "ring_buffer__new_no_cb")) > + goto cleanup_fd; > + > + ring =3D ring_buffer__ring(ringbuf, 0); > + if (!ASSERT_OK_PTR(ring, "ring_buffer__ring_no_cb")) > + goto cleanup_ringbuf; > + consumer_pos =3D ring__consumer_pos(ring); > + > + err =3D ring_buffer__consume_n(ringbuf, 0); > + ASSERT_EQ(err, -EINVAL, "ringbuf_consume_zero_no_cb"); > + err =3D ring__consume_n(ring, 0); > + ASSERT_EQ(err, -EINVAL, "ring_consume_zero_no_cb"); > + err =3D ring__consume(ring); > + ASSERT_EQ(err, -EINVAL, "ring_consume_no_cb"); > + ASSERT_EQ(ring__consumer_pos(ring), consumer_pos, "consumer_pos_no_cb")= ; > + > +cleanup_ringbuf: > + ring_buffer__free(ringbuf); > +cleanup_fd: > + close(no_cb_map_fd); > +cleanup: > + test_ringbuf_n_lskel__destroy(skel_n); > +} > + > static void ringbuf_n_subtest(void) > { > struct test_ringbuf_n_lskel *skel_n; > @@ -579,6 +670,8 @@ void test_ringbuf(void) > ringbuf_subtest(); > if (test__start_subtest("ringbuf_n")) > ringbuf_n_subtest(); > + if (test__start_subtest("ringbuf_null_cb")) > + ringbuf_null_cb_subtest(); > if (test__start_subtest("ringbuf_map_key")) > ringbuf_map_key_subtest(); > if (test__start_subtest("ringbuf_write"))