On Thu, Mar 27, 2025 at 09:38:05PM +0000, Tingmao Wang wrote:
Hi Mickaël,
Hi, thanks for the report.
On 3/20/25 19:06, Mickaël Salaün wrote: [...]
+static struct landlock_hierarchy * +get_hierarchy(const struct landlock_ruleset *const domain, const size_t layer) +{
- struct landlock_hierarchy *hierarchy = domain->hierarchy;
- ssize_t i;
- if (WARN_ON_ONCE(layer >= domain->num_layers))
return hierarchy;- for (i = domain->num_layers - 1; i > layer; i--) {
if (WARN_ON_ONCE(!hierarchy->parent))break;hierarchy = hierarchy->parent;- }
- return hierarchy;
+}
+#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
+static void test_get_hierarchy(struct kunit *const test) +{
- struct landlock_hierarchy dom0_hierarchy = {
.id = 10,- };
- struct landlock_hierarchy dom1_hierarchy = {
.parent = &dom0_hierarchy,.id = 20,- };
- struct landlock_hierarchy dom2_hierarchy = {
.parent = &dom1_hierarchy,.id = 30,- };
- struct landlock_ruleset dom2 = {
.hierarchy = &dom2_hierarchy,.num_layers = 3,- };
- KUNIT_EXPECT_EQ(test, 10, get_hierarchy(&dom2, 0)->id);
- KUNIT_EXPECT_EQ(test, 20, get_hierarchy(&dom2, 1)->id);
- KUNIT_EXPECT_EQ(test, 30, get_hierarchy(&dom2, 2)->id);
- KUNIT_EXPECT_EQ(test, 30, get_hierarchy(&dom2, -1)->id);
This causes a warning from WARN_ON_ONCE(layer >= domain->num_layers) when running this test, I guess because layer is unsigned.
Interestingly this doesn't make the test to fail (because the result is still correct), nor to show up when using tools/testing/kunit/kunit.py, which is why I didn't see that.
Should it be ssize_t, if this is an expected usage?
The get_hierarchy() code is correct, and the KUnit test is correct too. Using a ssize_t would introduce a bug.
The issue is that I wanted to test a case that should never happen, hence the WARN_ON_ONCE().
I guess the best "fix" for now would be to remove the KUnit test with -1, but there is a new KUnit feature to hide this kind of warning: https://lore.kernel.org/linux-kselftest/20250313114329.284104-1-acarmina@red... It is currently in linux-next, but I'm not sure it will be merged in Linux 6.15 .
For now I'll keep this commit but I'll send a fix/update to either remove the test or use the new DEFINE_SUPPRESSED_WARNING macros depending on its merge status.
------------[ cut here ]------------ WARNING: CPU: 7 PID: 145 at security/landlock/audit.c:142 get_hierarchy (security/landlock/audit.c:142) Modules linked in: CPU: 7 UID: 0 PID: 145 Comm: kunit_try_catch Tainted: G N 6.14.0-next-20250326-dev-00004-g4e57edc3e062-dirty #5 PREEMPT(undef) Tainted: [N]=TEST Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014 RIP: 0010:get_hierarchy (security/landlock/audit.c:142) Code: 83 e8 02 e8 18 00 84 c0 75 02 0f 0b 48 83 c4 08 48 89 d8 5b 41 5c 41 5e 5d c3 48 c7 c7 00 f3 21 83 e8 e2 e7 18 00 84 c0 75 e2 <0f> 0b eb de 48 89 75 e0 e8 a1 a9 a7 ff 48 8b 75 e0 e9 76 ff ff ff // snip // Call Trace:
<TASK> test_get_hierarchy (security/landlock/audit.c:178 (discriminator 5)) ? test_get_denied_layer (security/landlock/audit.c:158) ? lock_repin_lock (kernel/locking/lockdep.c:5649 kernel/locking/lockdep.c:5978) ? __lock_acquire (kernel/locking/lockdep.c:4675 kernel/locking/lockdep.c:5189) ? _raw_spin_unlock_irqrestore (./include/linux/spinlock_api_smp.h:151 kernel/locking/spinlock.c:194) ? find_held_lock (kernel/locking/lockdep.c:5348) ? trace_irq_enable (./include/trace/events/preemptirq.h:40 (discriminator 17)) ? trace_hardirqs_on (kernel/trace/trace_preemptirq.c:80) ? kvm_clock_get_cycles (./arch/x86/include/asm/preempt.h:95 arch/x86/kernel/kvmclock.c:80 arch/x86/kernel/kvmclock.c:86) ? ktime_get_ts64 (kernel/time/timekeeping.c:318 (discriminator 4) kernel/time/timekeeping.c:335 (discriminator 4) kernel/time/timekeeping.c:907 (discriminator 4)) kunit_try_run_case (lib/kunit/test.c:400 lib/kunit/test.c:443) ? kunit_try_run_case_cleanup (lib/kunit/test.c:430)