[PATCH] selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage

[PATCH] selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage

[Sean Young]

While porting, extend coverage of the BPF_LIRC_MODE2 attach/detach/
query API:

 - bpf_prog_attach() with invalid flags is rejected with -EINVAL
   and does not attach the program
 - bpf_prog_query() with invalid flags is rejected with -EINVAL
   without disturbing existing attachments
 - bpf_prog_query() reports the correct program id, not just count,
   at each step, via bpf_prog_get_info_by_fd()
 - a lirc chardev can hold more than one attached program: load a
   second, independent instance, attach it alongside the first,
   confirm both are reported by bpf_prog_query(), then detach it
   without disturbing the first program's attachment
 - detaching an already-detached program consistently fails with
   -ENOENT, for both the first and second program

Signed-off-by: Sean Young <sean@mess.org>
Assisted-by: Claude:claude-sonnet-5
---
 tools/testing/selftests/bpf/Makefile          |   3 -
 .../selftests/bpf/prog_tests/lirc_mode2.c     | 328 ++++++++++++++++++
 .../testing/selftests/bpf/progs/lirc_mode2.c  |  32 ++
 .../bpf/progs/test_lirc_mode2_kern.c          |  26 --
 .../testing/selftests/bpf/test_lirc_mode2.sh  |  41 ---
 5 files changed, 360 insertions(+), 70 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/lirc_mode2.c
 create mode 100644 tools/testing/selftests/bpf/progs/lirc_mode2.c
 delete mode 100644 tools/testing/selftests/bpf/progs/test_lirc_mode2_kern.c
 delete mode 100755 tools/testing/selftests/bpf/test_lirc_mode2.sh

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 2b2f93dec474..f8ec0f574559 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -123,7 +123,6 @@ TEST_FILES = xsk_prereqs.sh $(wildcard progs/btf_dump_test_case_*.c)
 
 # Order correspond to 'make run_tests' order
 TEST_PROGS := test_kmod.sh \
-	test_lirc_mode2.sh \
 	test_bpftool_build.sh \
 	test_doc_build.sh \
 	test_xsk.sh \
@@ -141,7 +140,6 @@ TEST_GEN_PROGS_EXTENDED = \
 	bench \
 	flow_dissector_load \
 	test_cpp \
-	test_lirc_mode2_user \
 	veristat \
 	xdp_features \
 	xdp_hw_metadata \
@@ -338,7 +336,6 @@ $(OUTPUT)/test_sockmap: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_tcpnotify_user: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(TRACE_HELPERS)
 $(OUTPUT)/test_sock_fields: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_tag: $(TESTING_HELPERS)
-$(OUTPUT)/test_lirc_mode2_user: $(TESTING_HELPERS)
 $(OUTPUT)/flow_dissector_load: $(TESTING_HELPERS)
 $(OUTPUT)/test_maps: $(TESTING_HELPERS)
 $(OUTPUT)/test_verifier: $(TESTING_HELPERS) $(CAP_HELPERS) $(UNPRIV_HELPERS)
diff --git a/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c b/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c
new file mode 100644
index 000000000000..17a38cd03845
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c
@@ -0,0 +1,328 @@
+// SPDX-License-Identifier: GPL-2.0
+// test ir decoder
+//
+// Copyright (C) 2018 Sean Young <sean@mess.org>
+
+// A lirc chardev is a device representing a consumer IR (cir) device which
+// can receive infrared signals from remote control and/or transmit IR.
+//
+// IR is sent as a series of pulses and space somewhat like morse code. The
+// BPF program can decode this into scancodes so that rc-core can translate
+// this into input key codes using the rc keymap.
+//
+// This test works by sending IR over rc-loopback, so the IR is processed by
+// BPF and then decoded into scancodes. The lirc chardev must be the one
+// associated with rc-loopback, see the output of ir-keytable(1).
+//
+// The following CONFIG options must be enabled for the test to succeed:
+// CONFIG_RC_CORE=y
+// CONFIG_BPF_RAWIR_EVENT=y
+// CONFIG_RC_LOOPBACK=y
+
+#include <linux/input.h>
+#include <linux/lirc.h>
+#include <glob.h>
+#include <limits.h>
+#include <poll.h>
+#include <test_progs.h>
+#include "lirc_mode2.skel.h"
+
+/* Read the DEVNAME= line out of the first uevent file that matches
+ * pattern, and turn it into a /dev/<name> path.
+ */
+static bool find_devname(const char *pattern, char *path, size_t path_sz)
+{
+	glob_t gl = {};
+	bool found = false;
+	FILE *f;
+
+	if (glob(pattern, 0, NULL, &gl) || gl.gl_pathc == 0)
+		goto out;
+
+	f = fopen(gl.gl_pathv[0], "r");
+	if (!f)
+		goto out;
+
+	char line[256];
+
+	while (fgets(line, sizeof(line), f)) {
+		char *val;
+
+		if (strncmp(line, "DEVNAME=", 8))
+			continue;
+
+		val = line + 8;
+		val[strcspn(val, "\n")] = '\0';
+		snprintf(path, path_sz, "/dev/%s", val);
+		found = true;
+		break;
+	}
+
+	fclose(f);
+out:
+	globfree(&gl);
+	return found;
+}
+
+/* Load rc-loopback and find the lirc and input chardevs it created. */
+static bool find_loopback_devices(char *lirc_path, char *input_path,
+				  size_t path_sz)
+{
+	glob_t gl = {};
+	bool found = false;
+
+	/* Ignore failure, we check for the resulting devices below. */
+	system("modprobe rc-loopback > /dev/null 2>&1");
+
+	if (glob("/sys/class/rc/rc*", 0, NULL, &gl)) {
+		fprintf(stderr, "No rc devices found, is CONFIG_RC_LOOPBACK enabled?\n");
+		return false;
+	}
+
+	for (size_t i = 0; i < gl.gl_pathc; i++) {
+		const char *rcdir = gl.gl_pathv[i];
+		char uevent_path[PATH_MAX];
+		char uevent[4096];
+		char pattern[PATH_MAX];
+		FILE *f;
+		size_t n;
+
+		snprintf(uevent_path, sizeof(uevent_path), "%s/uevent", rcdir);
+		f = fopen(uevent_path, "r");
+		if (!f)
+			continue;
+		n = fread(uevent, 1, sizeof(uevent) - 1, f);
+		fclose(f);
+		uevent[n] = '\0';
+
+		if (!strstr(uevent, "DRV_NAME=rc-loopback"))
+			continue;
+
+		snprintf(pattern, sizeof(pattern), "%s/lirc*/uevent", rcdir);
+		if (!find_devname(pattern, lirc_path, path_sz))
+			continue;
+
+		snprintf(pattern, sizeof(pattern), "%s/input*/event*/uevent", rcdir);
+		if (!find_devname(pattern, input_path, path_sz))
+			continue;
+
+		found = true;
+		break;
+	}
+
+	globfree(&gl);
+	return found;
+}
+
+void test_lirc_mode2(void)
+{
+	char lirc_path[PATH_MAX], input_path[PATH_MAX];
+	int lircfd = -1, inputfd = -1, progfd, progfd2 = -1;
+	struct lirc_mode2 *skel = NULL, *skel2 = NULL;
+	__u32 prog_ids[10], prog_flags[10], prog_cnt;
+	struct bpf_prog_info info;
+	__u32 info_len, prog_id, prog_id2;
+	int testir1 = 0x8ead;	/* keydown flag (0x8000) | scancode 0xead */
+	int testir2 = 0x4081;	/* pointer_rel flag (0x4000) | rel_x=1 | rel_y=1 */
+	struct input_event event;
+	struct pollfd pfd = {};
+	int ret;
+
+	if (getuid() != 0) {
+		test__skip();
+		return;
+	}
+
+	if (!find_loopback_devices(lirc_path, input_path, sizeof(lirc_path))) {
+		test__skip();
+		return;
+	}
+
+	skel = lirc_mode2__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "lirc_mode2__open_and_load"))
+		return;
+
+	progfd = bpf_program__fd(skel->progs.bpf_decoder);
+
+	memset(&info, 0, sizeof(info));
+	info_len = sizeof(info);
+	ret = bpf_prog_get_info_by_fd(progfd, &info, &info_len);
+	if (!ASSERT_OK(ret, "get first program's info"))
+		goto out;
+	prog_id = info.id;
+
+	lircfd = open(lirc_path, O_RDWR | O_NONBLOCK);
+	if (!ASSERT_GE(lircfd, 0, "open lirc device"))
+		goto out;
+
+	/* Try to detach it before it was ever attached, should fail. */
+	ret = bpf_prog_detach2(progfd, lircfd, BPF_LIRC_MODE2);
+	if (!ASSERT_EQ(ret, -ENOENT, "detach unattached program"))
+		goto out;
+
+	inputfd = open(input_path, O_RDONLY | O_NONBLOCK);
+	if (!ASSERT_GE(inputfd, 0, "open input device"))
+		goto out;
+
+	prog_cnt = ARRAY_SIZE(prog_ids);
+	ret = bpf_prog_query(lircfd, BPF_LIRC_MODE2, 0, prog_flags, prog_ids,
+			     &prog_cnt);
+	if (!ASSERT_OK(ret, "query programs before attach"))
+		goto out;
+	if (!ASSERT_EQ(prog_cnt, 0, "no programs should be attached yet"))
+		goto out;
+
+	/* Invalid attach flags must be rejected, and must not attach. */
+	ret = bpf_prog_attach(progfd, lircfd, BPF_LIRC_MODE2, 1);
+	if (!ASSERT_EQ(ret, -EINVAL, "attach with invalid flags"))
+		goto out;
+
+	prog_cnt = ARRAY_SIZE(prog_ids);
+	ret = bpf_prog_query(lircfd, BPF_LIRC_MODE2, 0, prog_flags, prog_ids,
+			     &prog_cnt);
+	if (!ASSERT_OK(ret, "query programs after rejected attach"))
+		goto out;
+	if (!ASSERT_EQ(prog_cnt, 0, "rejected attach should not attach"))
+		goto out;
+
+	ret = bpf_prog_attach(progfd, lircfd, BPF_LIRC_MODE2, 0);
+	if (!ASSERT_OK(ret, "attach program to lirc device"))
+		goto out;
+
+	/* Invalid query flags must be rejected too, without upsetting state. */
+	prog_cnt = ARRAY_SIZE(prog_ids);
+	ret = bpf_prog_query(lircfd, BPF_LIRC_MODE2, 1, prog_flags, prog_ids,
+			     &prog_cnt);
+	ASSERT_EQ(ret, -EINVAL, "query with invalid flags");
+
+	prog_cnt = ARRAY_SIZE(prog_ids);
+	ret = bpf_prog_query(lircfd, BPF_LIRC_MODE2, 0, prog_flags, prog_ids,
+			     &prog_cnt);
+	if (!ASSERT_OK(ret, "query programs after attach"))
+		goto out_detach;
+	if (!ASSERT_EQ(prog_cnt, 1, "one program should be attached"))
+		goto out_detach;
+	ASSERT_EQ(prog_ids[0], prog_id, "queried id should match attached program");
+
+	/* Write raw IR */
+	ret = write(lircfd, &testir1, sizeof(testir1));
+	if (!ASSERT_EQ(ret, sizeof(testir1), "send test IR message 1"))
+		goto out_detach;
+
+	pfd.fd = inputfd;
+	pfd.events = POLLIN;
+
+	for (;;) {
+		poll(&pfd, 1, 100);
+
+		/* Read decoded IR */
+		ret = read(inputfd, &event, sizeof(event));
+		if (!ASSERT_EQ(ret, sizeof(event), "read decoded IR 1"))
+			goto out_detach;
+
+		if (event.type == EV_MSC && event.code == MSC_SCAN &&
+		    event.value == 0xead)
+			break;
+	}
+
+	/* Write raw IR */
+	ret = write(lircfd, &testir2, sizeof(testir2));
+	if (!ASSERT_EQ(ret, sizeof(testir2), "send test IR message 2"))
+		goto out_detach;
+
+	for (;;) {
+		poll(&pfd, 1, 100);
+
+		/* Read decoded IR */
+		ret = read(inputfd, &event, sizeof(event));
+		if (!ASSERT_EQ(ret, sizeof(event), "read decoded IR 2"))
+			goto out_detach;
+
+		if (event.type == EV_REL && event.code == REL_Y &&
+		    event.value == 1)
+			break;
+	}
+
+	prog_cnt = ARRAY_SIZE(prog_ids);
+	ret = bpf_prog_query(lircfd, BPF_LIRC_MODE2, 0, prog_flags, prog_ids,
+			     &prog_cnt);
+	if (!ASSERT_OK(ret, "query programs after IR was decoded"))
+		goto out_detach;
+	if (!ASSERT_EQ(prog_cnt, 1, "one program should still be attached"))
+		goto out_detach;
+
+	/*
+	 * The lirc chardev can hold more than one attached program at once.
+	 * Load a second, independent instance and check it can be attached
+	 * alongside the first, queried, and then detached on its own
+	 * without disturbing the first program's attachment.
+	 */
+	skel2 = lirc_mode2__open_and_load();
+	if (!ASSERT_OK_PTR(skel2, "lirc_mode2__open_and_load (2nd)"))
+		goto out_detach;
+
+	progfd2 = bpf_program__fd(skel2->progs.bpf_decoder);
+
+	memset(&info, 0, sizeof(info));
+	info_len = sizeof(info);
+	ret = bpf_prog_get_info_by_fd(progfd2, &info, &info_len);
+	if (!ASSERT_OK(ret, "get second program's info"))
+		goto out_detach;
+	prog_id2 = info.id;
+
+	ret = bpf_prog_attach(progfd2, lircfd, BPF_LIRC_MODE2, 0);
+	if (!ASSERT_OK(ret, "attach second program to lirc device"))
+		goto out_detach;
+
+	prog_cnt = ARRAY_SIZE(prog_ids);
+	ret = bpf_prog_query(lircfd, BPF_LIRC_MODE2, 0, prog_flags, prog_ids,
+			     &prog_cnt);
+	if (!ASSERT_OK(ret, "query programs after second attach"))
+		goto out_detach2;
+	if (!ASSERT_EQ(prog_cnt, 2, "two programs should be attached"))
+		goto out_detach2;
+	ASSERT_TRUE((prog_ids[0] == prog_id && prog_ids[1] == prog_id2) ||
+		    (prog_ids[0] == prog_id2 && prog_ids[1] == prog_id),
+		    "queried ids should be the two attached programs");
+
+	/* Detach the second program; the first should remain attached. */
+	ret = bpf_prog_detach2(progfd2, lircfd, BPF_LIRC_MODE2);
+	if (!ASSERT_OK(ret, "detach second program"))
+		goto out_detach2;
+
+	/* Detaching an already-detached program should now fail. */
+	ret = bpf_prog_detach2(progfd2, lircfd, BPF_LIRC_MODE2);
+	ASSERT_EQ(ret, -ENOENT, "detach second program again");
+
+	prog_cnt = ARRAY_SIZE(prog_ids);
+	ret = bpf_prog_query(lircfd, BPF_LIRC_MODE2, 0, prog_flags, prog_ids,
+			     &prog_cnt);
+	if (!ASSERT_OK(ret, "query programs after second detach"))
+		goto out_detach;
+	if (!ASSERT_EQ(prog_cnt, 1, "one program should remain attached"))
+		goto out_detach;
+	ASSERT_EQ(prog_ids[0], prog_id, "remaining program should be the first one");
+
+out_detach:
+	/* Let's try detaching it now it is actually attached. */
+	ret = bpf_prog_detach2(progfd, lircfd, BPF_LIRC_MODE2);
+	ASSERT_OK(ret, "detach program from lirc device");
+
+	/* Detaching it again should now fail the same way. */
+	ret = bpf_prog_detach2(progfd, lircfd, BPF_LIRC_MODE2);
+	ASSERT_EQ(ret, -ENOENT, "detach program from lirc device again");
+	goto out;
+
+out_detach2:
+	/* Best-effort cleanup of the second program before bailing out. */
+	bpf_prog_detach2(progfd2, lircfd, BPF_LIRC_MODE2);
+	goto out_detach;
+
+out:
+	if (inputfd >= 0)
+		close(inputfd);
+	if (lircfd >= 0)
+		close(lircfd);
+	lirc_mode2__destroy(skel2);
+	lirc_mode2__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/lirc_mode2.c b/tools/testing/selftests/bpf/progs/lirc_mode2.c
new file mode 100644
index 000000000000..98137f3c5c03
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/lirc_mode2.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+// test ir decoder
+//
+// Copyright (C) 2018 Sean Young <sean@mess.org>
+
+#include <linux/bpf.h>
+#include <linux/lirc.h>
+#include <bpf/bpf_helpers.h>
+
+SEC("lirc_mode2")
+int bpf_decoder(unsigned int *sample)
+{
+	if (LIRC_IS_PULSE(*sample)) {
+		unsigned int duration = LIRC_VALUE(*sample);
+
+		/*
+		 * Flag bits picked deliberately low: rc-loopback simulates
+		 * a receiver overflow for any pulse over MS_TO_US(50) (see
+		 * loop_tx_ir() in rc-loopback.c), which would silently
+		 * swallow the sample before it ever reaches this decoder.
+		 */
+		if (duration & 0x8000)
+			bpf_rc_keydown(sample, 0x40, duration & 0x3fff, 0);
+		if (duration & 0x4000)
+			bpf_rc_pointer_rel(sample, (duration >> 7) & 0x7f,
+					   duration & 0x7f);
+	}
+
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/test_lirc_mode2_kern.c b/tools/testing/selftests/bpf/progs/test_lirc_mode2_kern.c
deleted file mode 100644
index cbe4284c032f..000000000000
--- a/tools/testing/selftests/bpf/progs/test_lirc_mode2_kern.c
+++ /dev/null
@@ -1,26 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-// test ir decoder
-//
-// Copyright (C) 2018 Sean Young <sean@mess.org>
-
-#include <linux/bpf.h>
-#include <linux/lirc.h>
-#include <bpf/bpf_helpers.h>
-
-SEC("lirc_mode2")
-int bpf_decoder(unsigned int *sample)
-{
-	if (LIRC_IS_PULSE(*sample)) {
-		unsigned int duration = LIRC_VALUE(*sample);
-
-		if (duration & 0x1000)
-			bpf_rc_keydown(sample, 0x40, duration & 0xffff, 0);
-		if (duration & 0x2000)
-			bpf_rc_pointer_rel(sample, (duration >> 8) & 0xff,
-					   duration & 0xff);
-	}
-
-	return 0;
-}
-
-char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_lirc_mode2.sh b/tools/testing/selftests/bpf/test_lirc_mode2.sh
deleted file mode 100755
index 5252b91f48a1..000000000000
--- a/tools/testing/selftests/bpf/test_lirc_mode2.sh
+++ /dev/null
@@ -1,41 +0,0 @@
-#!/bin/bash
-# SPDX-License-Identifier: GPL-2.0
-
-# Kselftest framework requirement - SKIP code is 4.
-ksft_skip=4
-ret=$ksft_skip
-
-msg="skip all tests:"
-if [ $UID != 0 ]; then
-	echo $msg please run this as root >&2
-	exit $ksft_skip
-fi
-
-GREEN='\033[0;92m'
-RED='\033[0;31m'
-NC='\033[0m' # No Color
-
-modprobe rc-loopback
-
-for i in /sys/class/rc/rc*
-do
-	if grep -q DRV_NAME=rc-loopback $i/uevent
-	then
-		LIRCDEV=$(grep DEVNAME= $i/lirc*/uevent | sed sQDEVNAME=Q/dev/Q)
-		INPUTDEV=$(grep DEVNAME= $i/input*/event*/uevent | sed sQDEVNAME=Q/dev/Q)
-	fi
-done
-
-if [ -n "$LIRCDEV" ];
-then
-	TYPE=lirc_mode2
-	./test_lirc_mode2_user $LIRCDEV $INPUTDEV
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		echo -e ${RED}"FAIL: $TYPE"${NC}
-	else
-		echo -e ${GREEN}"PASS: $TYPE"${NC}
-	fi
-fi
-
-exit $ret
-- 
2.55.0

Re: [PATCH] selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing `globfree()` when `glob()` returns an error in `find_loopback_devices`.
- [Low] Orphaned test file `test_lirc_mode2_user.c` left in the source tree.
- [Low] Multi-line comment formatting violates BPF subsystem guidelines.
--

--- Patch [1]: [PATCH] selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage ---
commit 932c1322ffc2d27251b53c4449db5c4ec36061e6
Author: Sean Young <sean@mess.org>

selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage

While porting, extend coverage of the BPF_LIRC_MODE2 attach/detach/
query API:

[ ... ]

> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 2b2f93dec474b..f8ec0f574559a 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
[ ... ]
> @@ -141,7 +140,6 @@ TEST_GEN_PROGS_EXTENDED = \
>  	bench \
>  	flow_dissector_load \
>  	test_cpp \
> -	test_lirc_mode2_user \
>  	veristat \

[Severity: Low]
This isn't a bug, but it looks like tools/testing/selftests/bpf/test_lirc_mode2_user.c
was removed from the Makefile here but not deleted from the source tree. Should
that file be deleted as part of this patch?

[ ... ]
> diff --git a/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c b/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c
> new file mode 100644
> index 0000000000000..17a38cd03845c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c
> @@ -0,0 +1,328 @@
[ ... ]
> +/* Read the DEVNAME= line out of the first uevent file that matches
> + * pattern, and turn it into a /dev/<name> path.
> + */
> +static bool find_devname(const char *pattern, char *path, size_t path_sz)

[Severity: Low]
This isn't a bug, but does this violate the BPF subsystem multi-line comment
style guidelines? The opening /* should probably be on its own line:

/*
 * Read the DEVNAME= line out of the first uevent file that matches
 * pattern, and turn it into a /dev/<name> path.
 */

[ ... ]
> +/* Load rc-loopback and find the lirc and input chardevs it created. */
> +static bool find_loopback_devices(char *lirc_path, char *input_path,
> +				  size_t path_sz)
> +{
> +	glob_t gl = {};
> +	bool found = false;
> +
> +	/* Ignore failure, we check for the resulting devices below. */
> +	system("modprobe rc-loopback > /dev/null 2>&1");
> +
> +	if (glob("/sys/class/rc/rc*", 0, NULL, &gl)) {
> +		fprintf(stderr, "No rc devices found, is CONFIG_RC_LOOPBACK enabled?\n");
> +		return false;
> +	}

[Severity: Medium]
Could this leak memory if glob() returns an error like GLOB_NOMATCH? Some
implementations of glob() may allocate memory for gl_pathv even on error paths.
Should there be a call to globfree(&gl) before returning false?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820175154.708374-1-sean@mess.org?part=1

Re: [PATCH] selftests/bpf: convert lirc_mode2 to prog_tests and extend coverage

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 6748 bytes --]

> diff --git a/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c b/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c
> new file mode 100644
> index 000000000000..17a38cd03845
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/lirc_mode2.c

[ ... ]

> +/* Load rc-loopback and find the lirc and input chardevs it created. */
> +static bool find_loopback_devices(char *lirc_path, char *input_path,
> +				  size_t path_sz)
> +{
> +	glob_t gl = {};
> +	bool found = false;
> +
> +	/* Ignore failure, we check for the resulting devices below. */
> +	system("modprobe rc-loopback > /dev/null 2>&1");

A subsystem pattern flags this as potentially concerning: this hand-rolls
a fire-and-forget shell invocation when test_progs.h already provides
SYS_NOFAIL(fmt, ...) at test_progs.h:444, which handles both the ignored
return value and the redirect to /dev/null. The line would become just
SYS_NOFAIL("modprobe rc-loopback"). SYS_NOFAIL() is used 119 times across
prog_tests/ for exactly this pattern.

The subsystem guide flags tests that reimplement existing helpers instead
of using them, though in this case raw system() is also widespread in
prog_tests/ so the behaviour is identical either way. Worth considering
for consistency?

> +
> +	if (glob("/sys/class/rc/rc*", 0, NULL, &gl)) {
> +		fprintf(stderr, "No rc devices found, is CONFIG_RC_LOOPBACK enabled?\n");
> +		return false;
> +	}
> +
> +	for (size_t i = 0; i < gl.gl_pathc; i++) {
[ ... ]
> +		if (!strstr(uevent, "DRV_NAME=rc-loopback"))
> +			continue;
> +
> +		snprintf(pattern, sizeof(pattern), "%s/lirc*/uevent", rcdir);
> +		if (!find_devname(pattern, lirc_path, path_sz))
> +			continue;
> +
> +		snprintf(pattern, sizeof(pattern), "%s/input*/event*/uevent", rcdir);
> +		if (!find_devname(pattern, input_path, path_sz))
> +			continue;
> +
> +		found = true;
> +		break;
> +	}
> +
> +	globfree(&gl);
> +	return found;
> +}

A subsystem pattern flags this as potentially concerning: the only
diagnostic is attached to the wrong condition, so the common unconfigured
case skips with no explanation.

glob("/sys/class/rc/rc*") fails only when there is no rc device at all
(CONFIG_RC_CORE=n or no IR hardware), yet that's where the message names
CONFIG_RC_LOOPBACK. The actual CONFIG_RC_LOOPBACK=n case (rc devices
exist, but none has DRV_NAME=rc-loopback) and the CONFIG_LIRC=n case (no
lirc*/uevent under the loopback rc device) both fall through the loop and
reach the silent 'return found' with found=false.

test_lirc_mode2() then calls test__skip() with no message (test__skip()
takes no argument), so the run reports lirc_mode2:SKIP with nothing
telling the reader which prerequisite was missing.

> +
> +void test_lirc_mode2(void)
> +{
> +	char lirc_path[PATH_MAX], input_path[PATH_MAX];
> +	int lircfd = -1, inputfd = -1, progfd, progfd2 = -1;
> +	struct lirc_mode2 *skel = NULL, *skel2 = NULL;
[ ... ]
> +
> +	if (getuid() != 0) {
> +		test__skip();
> +		return;
> +	}
> +
> +	if (!find_loopback_devices(lirc_path, input_path, sizeof(lirc_path))) {
> +		test__skip();
> +		return;
> +	}

A subsystem pattern flags this as potentially concerning: both skip paths
call bare test__skip() with no accompanying message, so the two very
different reasons for skipping are indistinguishable in the test output.

The first skips because the test is not running as root; the second skips
because find_loopback_devices() could not find an rc-loopback lirc/input
device pair (which can mean CONFIG_RC_LOOPBACK is not built, or modprobe
rc-loopback failed, or the sysfs walk did not match). A user seeing
lirc_mode2:SKIP has no way to tell which precondition was missing.

That said, bare test__skip() with no message is the overwhelmingly
dominant idiom in this directory (126 call sites use it with no message),
and test__skip() accepts no message parameter, so there is no in-framework
way to attach a reason. By the 'compare to the neighbours' rule this would
normally be consistent with established practice.

> diff --git a/tools/testing/selftests/bpf/progs/lirc_mode2.c b/tools/testing/selftests/bpf/progs/lirc_mode2.c
> new file mode 100644
> index 000000000000..98137f3c5c03
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/lirc_mode2.c

[ ... ]

> +SEC("lirc_mode2")
> +int bpf_decoder(unsigned int *sample)
> +{
> +	if (LIRC_IS_PULSE(*sample)) {
> +		unsigned int duration = LIRC_VALUE(*sample);
> +
> +		/*
> +		 * Flag bits picked deliberately low: rc-loopback simulates
> +		 * a receiver overflow for any pulse over MS_TO_US(50) (see
> +		 * loop_tx_ir() in rc-loopback.c), which would silently
> +		 * swallow the sample before it ever reaches this decoder.
> +		 */
> +		if (duration & 0x8000)
> +			bpf_rc_keydown(sample, 0x40, duration & 0x3fff, 0);
> +		if (duration & 0x4000)
> +			bpf_rc_pointer_rel(sample, (duration >> 7) & 0x7f,
> +					   duration & 0x7f);
> +	}
> +
> +	return 0;
> +}

Does the comment match what the code does? Looking at the deleted
progs/test_lirc_mode2_kern.c, the old decoder used flag bits 0x1000/0x2000
with masks 0xffff, >>8 & 0xff, and & 0xff. The new code uses 0x8000/0x4000
with masks 0x3fff, >>7 & 0x7f, and & 0x7f.

The comment claims the flag bits were "picked deliberately low" to stay
under rc-loopback's MS_TO_US(50) overflow threshold, but the bits were
moved UP: the largest encodable sample went from 0x3FFF (16383 us, ample
margin under the 50000 us threshold) to 0xBFFF (49151 us, 849 us of
margin), and with both flags set, 0xFFFF (65535 us) exceeds the threshold
and would be discarded by ir_raw_event_overflow() rather than decoded.

The two committed test vectors (0x8ead and 0x4081) are both safe, so the
test passes today. The concern is future-facing: the comment actively
misleads anyone adding a third vector into believing high-bit values are
the safe choice, when the new layout leaves only 849 us of headroom and
any value with both flag bits set is silently swallowed by the loopback
overflow simulation.

The new bit layout is genuinely cleaner than the old one (with flags at
0x1000/0x2000 and rel_x extracted as (duration >> 8) & 0xff, the 0x2000
flag bit landed inside rel_x, so flags and payload overlapped). Moving
flags to the top two bits and shrinking the payload to 14 bits removes
that overlap. Could the comment be updated to match what the code actually
does, and perhaps mention the 50 ms ceiling on encodable samples?

Also, the commit message enumerates five coverage additions but does not
mention the format change at all.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32401351183