linux-kernel - Re: [PATCH v4 00/10] clk: Add kunit tests for fixed rate and parent data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b822c6a5488c4098059b6d3c35eecbbd.sboyd@kernel.org>
Date: Thu, 02 May 2024 18:27:42 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: David Gow <davidgow@...gle.com>
Cc: Michael Turquette <mturquette@...libre.com>, linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, patches@...ts.linux.dev, kunit-dev@...glegroups.com, linux-kselftest@...r.kernel.org, devicetree@...r.kernel.org, Brendan Higgins <brendan.higgins@...ux.dev>, Rae Moar <rmoar@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Rafael J . Wysocki <rafael@...nel.org>, Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, Daniel Latypov <dlatypov@...gle.com>, Christian Marangi <ansuelsmth@...il.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, Maxime Ripard <maxime@...no.tech>
Subject: Re: [PATCH v4 00/10] clk: Add kunit tests for fixed rate and parent data

Quoting David Gow (2024-05-01 01:08:11)
> 
> Thanks very much. I'm about halfway through reviewing these, and I
> like them a lot so far.
> 
> Most of my thoughts are just naming ideas. I fear some of them may be
> the reverse of previous suggestions, as we've since landed the KUnit
> device wrappers in include/kunit/device.h, which we decided would live
> as part of KUnit, not as part of the device infrastructure. I don't
> enormously mind if we make the opposite decision for these, though it
> does seem a bit inconsistent if we do 'devices' differently from
> 'platform_devices'. Thoughts?

Let's discuss on one of the patches.

> 
> The other thing I've noted so far is that the
> of_apply_kunit_platform_device and of_overlay_apply_kunit_cleanup
> tests fail (and BUG() with a NULL pointer) on powerpc:
> > [15:18:51]     # of_overlay_apply_kunit_platform_device: EXPECTATION FAILED at drivers/of/overlay_test.c:47
> > [15:18:51]     Expected pdev is not null, but is
> > [15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c

This seems to be because pdev is NULL and we call put_device(&pdev->dev)
on it. We could be nicer and have an 'if (pdev)' check there. I wonder
if that fixes the other two below?

---8<---
diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c
index 223e5a5c23c5..85cfbe6bb132 100644
--- a/drivers/of/overlay_test.c
+++ b/drivers/of/overlay_test.c
@@ -45,7 +45,8 @@ static void of_overlay_apply_kunit_platform_device(struct kunit *test)
 
 	pdev = of_find_device_by_node(np);
 	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, pdev);
-	put_device(&pdev->dev);
+	if (pdev)
+		put_device(&pdev->dev);
 }
 
 static int of_overlay_bus_match_compatible(struct device *dev, const void *data)
@@ -77,8 +78,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, np);
 
 	pdev = of_find_device_by_node(np);
-	put_device(&pdev->dev); /* Not derefing 'pdev' after this */
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, pdev);
+	put_device(&pdev->dev); /* Not derefing 'pdev' after this */
 
 	/* Remove overlay */
 	kunit_cleanup(&fake);
@@ -91,7 +92,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test)
 	dev = bus_find_device(&platform_bus_type, NULL, kunit_compatible,
 			      of_overlay_bus_match_compatible);
 	KUNIT_EXPECT_PTR_EQ(test, NULL, dev);
-	put_device(dev);
+	if (dev)
+		put_device(dev);
 }
 
 static struct kunit_case of_overlay_apply_kunit_test_cases[] = {

> > [15:18:51]     # of_overlay_apply_kunit_platform_device: try faulted: last line seen lib/kunit/resource.c:99
> > [15:18:51]     # of_overlay_apply_kunit_platform_device: internal error occurred preventing test case from running: -4
> > [15:18:51] [FAILED] of_overlay_apply_kunit_platform_device
> 
> > [15:18:51] BUG: Kernel NULL pointer dereference at 0x0000004c
> > [15:18:51] note: kunit_try_catch[698] exited with irqs disabled
> > [15:18:51]     # of_overlay_apply_kunit_cleanup: try faulted: last line seen drivers/of/overlay_test.c:77
> > [15:18:51]     # of_overlay_apply_kunit_cleanup: internal error occurred preventing test case from running: -4
> > [15:18:51] [FAILED] of_overlay_apply_kunit_cleanup
> 
> I've not had a chance to dig into it any further, yet, but it appears
> to work on all of the other architectures I tried.

Cool. I don't know why powerpc doesn't make devices. Maybe it has a
similar design to sparc to create resources. I'll check it out.