of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS

PATCH Applied Marc Zyngier 3 participants 8 messages Aug 20, 2021 – Aug 23, 2021 Device tree

lore

Share & tools

lore

Marc Zyngier

author

2021-08-20 14:47 UTC #0 lore raw

PATCH 1/1 of: Don't allow __of_attached_node_sysfs() without CONFIG_SYSFS

Trying to boot without SYSFS, but with OF_DYNAMIC quickly results in a crash:

[    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
[...]
[    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
[    0.105810] Hardware name: linux,dummy-virt (DT)
[    0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
[    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
[...]
[    0.134087] Call trace:
[    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
[    0.136054]  safe_name+0x4c/0xd0
[    0.136994]  __of_attach_node_sysfs+0xf8/0x124
[    0.138287]  of_core_init+0x90/0xfc
[    0.139296]  driver_init+0x30/0x4c
[    0.140283]  kernel_init_freeable+0x160/0x1b8
[    0.141543]  kernel_init+0x30/0x140
[    0.142561]  ret_from_fork+0x10/0x18

While not having sysfs isn't a very common option these days, it is still expected that such configuration would work.

Paper over it by bailing out from __of_attach_node_sysfs() if CONFIG_SYSFS isn't enabled.

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/of/kobj.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/of/kobj.c +1 -1

index a32e60b024b8..6675b5e56960 100644
@@ -119,7 +119,7 @@ int __of_attach_node_sysfs(struct device_node *np)
 	struct property *pp;
 	int rc;
 
-	if (!of_kset)
+	if (!IS_ENABLED(CONFIG_SYSFS) || !of_kset)
 		return 0;
 
 	np->kobj.kset = of_kset;

Signature · 1 line

2.30.2

Frank Rowand

2021-08-23 04:01 UTC #1 lore raw

Hi Marc,

Quoted 21 lines · Marc Zyngier

Trying to boot without SYSFS, but with OF_DYNAMIC quickly results in a crash:

[    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
[...]
[    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
[    0.105810] Hardware name: linux,dummy-virt (DT)
[    0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
[    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
[...]
[    0.134087] Call trace:
[    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
[    0.136054]  safe_name+0x4c/0xd0
[    0.136994]  __of_attach_node_sysfs+0xf8/0x124
[    0.138287]  of_core_init+0x90/0xfc
[    0.139296]  driver_init+0x30/0x4c
[    0.140283]  kernel_init_freeable+0x160/0x1b8
[    0.141543]  kernel_init+0x30/0x140
[    0.142561]  ret_from_fork+0x10/0x18

Quoted 5 lines

While not having sysfs isn't a very common option these days, it is still expected that such configuration would work.
Paper over it by bailing out from __of_attach_node_sysfs() if CONFIG_SYSFS isn't enabled.

CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC is enabled, and it should not be possible to disable CONFIG_SYSFS in this case.

Can you send your .config?
What ARCH did you build the kernel with?

Thanks,

Frank

Quoted 20 lines

Signed-off-by Marc Zyngier <[email protected]>

---
 drivers/of/kobj.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/of/kobj.c +1 -1

index a32e60b024b8..6675b5e56960 100644
@@ -119,7 +119,7 @@ int __of_attach_node_sysfs(struct device_node *np)
 	struct property *pp;
 	int rc;
 
-	if (!of_kset)
+	if (!IS_ENABLED(CONFIG_SYSFS) || !of_kset)
 		return 0;
 
 	np->kobj.kset = of_kset;

Marc Zyngier

author

2021-08-23 08:04 UTC #2 lore raw

Hi Frank,

Quoted 28 lines · Frank Rowand

Hi Marc,

Quoted 24 lines · Marc Zyngier

Trying to boot without SYSFS, but with OF_DYNAMIC quickly results in a crash:

[    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
[...]
[    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
[    0.105810] Hardware name: linux,dummy-virt (DT)
[    0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
[    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
[...]
[    0.134087] Call trace:
[    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
[    0.136054]  safe_name+0x4c/0xd0
[    0.136994]  __of_attach_node_sysfs+0xf8/0x124
[    0.138287]  of_core_init+0x90/0xfc
[    0.139296]  driver_init+0x30/0x4c
[    0.140283]  kernel_init_freeable+0x160/0x1b8
[    0.141543]  kernel_init+0x30/0x140
[    0.142561]  ret_from_fork+0x10/0x18

While not having sysfs isn't a very common option these days, it is still expected that such configuration would work.

Quoted 6 lines

Quoted 2 lines
Paper over it by bailing out from __of_attach_node_sysfs() if CONFIG_SYSFS isn't enabled.
CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC is enabled, and it should not be possible to disable CONFIG_SYSFS in this case.

OF_DYNAMIC selects OF_KOBJ, which itself doesn't actually depends on SYSFS. It merely has a "def_bool SYSFS". If a stronger dependency is expected, that's probably where it should be expressed.

However, kobj.c already has a couple of

	if (!IS_ENABLED(CONFIG_SYSFS))
		return 0;

sprinkled over it, so there is already an assumption that it can be build without SYSFS.

Quoted 2 lines

Can you send your .config?

https://pastebin.com/MmRWuxLr

Quoted 1 line

What ARCH did you build the kernel with?

That's an arm64 build.

Thanks,

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Rob Herring

maintainer

2021-08-23 18:47 UTC #3 lore raw

In reply to Frank Rowand

Quoted 27 lines · Frank Rowand

Hi Marc,

Quoted 24 lines · Marc Zyngier

Trying to boot without SYSFS, but with OF_DYNAMIC quickly results in a crash:

[    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
[...]
[    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
[    0.105810] Hardware name: linux,dummy-virt (DT)
[    0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
[    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
[...]
[    0.134087] Call trace:
[    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
[    0.136054]  safe_name+0x4c/0xd0
[    0.136994]  __of_attach_node_sysfs+0xf8/0x124
[    0.138287]  of_core_init+0x90/0xfc
[    0.139296]  driver_init+0x30/0x4c
[    0.140283]  kernel_init_freeable+0x160/0x1b8
[    0.141543]  kernel_init+0x30/0x140
[    0.142561]  ret_from_fork+0x10/0x18

While not having sysfs isn't a very common option these days, it is still expected that such configuration would work.

Quoted 6 lines

Quoted 2 lines
Paper over it by bailing out from __of_attach_node_sysfs() if CONFIG_SYSFS isn't enabled.
CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC is enabled, and it should not be possible to disable CONFIG_SYSFS in this case.

That used to be true, but isn't now.

Rob

Frank Rowand

2021-08-23 21:27 UTC #4 lore raw

Quoted 28 lines · Rob Herring

Quoted 27 lines · Frank Rowand

Hi Marc,

Quoted 24 lines · Marc Zyngier

Trying to boot without SYSFS, but with OF_DYNAMIC quickly results in a crash:

[    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
[...]
[    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
[    0.105810] Hardware name: linux,dummy-virt (DT)
[  0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
[    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
[...]
[    0.134087] Call trace:
[    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
[    0.136054]  safe_name+0x4c/0xd0
[    0.136994]  __of_attach_node_sysfs+0xf8/0x124
[    0.138287]  of_core_init+0x90/0xfc
[    0.139296]  driver_init+0x30/0x4c
[    0.140283]  kernel_init_freeable+0x160/0x1b8
[    0.141543]  kernel_init+0x30/0x140
[    0.142561]  ret_from_fork+0x10/0x18

While not having sysfs isn't a very common option these days, it is still expected that such configuration would work.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Paper over it by bailing out from __of_attach_node_sysfs() if CONFIG_SYSFS isn't enabled.
CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC is enabled, and it should not be possible to disable CONFIG_SYSFS in this case.
That used to be true, but isn't now.

OK. I agree with you, but when I investigated the original patch email I came to a different conclusion because of the way that I used make menuconfig to debug the situation.

It is true when I start with a .config created from 'make qcom_defconfig', then select OF_UNITTEST, which is the only way I can see OF_DYNAMIC. It is the "if OF_UNITTEST" that means SYSFS can not be disabled.

If I start with the .config that Marc supplied, then make menuconfig still does not show the OC_DYNAMIC option, but leaves it set since it was already set. In this case SYSFS remains disabled because OF_UNITTEST is also disabled.

Using '/OF_DYNAMIC' from within make menuconfig, to get more info about OF_DYNAMIC tells me that the prompt for OF_DYNAMIC is visible if OF && OF_UNITTEST. This is due to the "if OF_UNITTEST" in line 58 of drivers/of/Kconfig in the OF_DYNAMIC specification.

Thus I can't figure out how to use make menuconfig to set OF_DYNAMIC without setting OF_UNITTEST. I tried setting OF_UNITTEST, then setting OF_DYNAMIC, saving the changes, then another make menuconfig to disable OF_UNITTEST, which then has the side effect of unsetting OF_DYNAMIC.

Would you accept a patch that removes the "if OF_UNITTEST" from the Kconfig entry for OF_DYNAMIC?

-Frank

Quoted 3 lines

Rob

Rob Herring

maintainer

2021-08-23 22:14 UTC #5 lore raw

Quoted 39 lines · Frank Rowand

Quoted 28 lines · Rob Herring
Quoted 27 lines · Frank Rowand
Hi Marc,
Quoted 24 lines · Marc Zyngier
Trying to boot without SYSFS, but with OF_DYNAMIC quickly results in a crash:
[    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
[...]
[    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
[    0.105810] Hardware name: linux,dummy-virt (DT)
[  0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
[    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
[...]
[    0.134087] Call trace:
[    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
[    0.136054]  safe_name+0x4c/0xd0
[    0.136994]  __of_attach_node_sysfs+0xf8/0x124
[    0.138287]  of_core_init+0x90/0xfc
[    0.139296]  driver_init+0x30/0x4c
[    0.140283]  kernel_init_freeable+0x160/0x1b8
[    0.141543]  kernel_init+0x30/0x140
[    0.142561]  ret_from_fork+0x10/0x18
While not having sysfs isn't a very common option these days, it is still expected that such configuration would work.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Paper over it by bailing out from __of_attach_node_sysfs() if CONFIG_SYSFS isn't enabled.
CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC is enabled, and it should not be possible to disable CONFIG_SYSFS in this case.
That used to be true, but isn't now.

Quoted 7 lines

OK. I agree with you, but when I investigated the original patch email I came to a different conclusion because of the way that I used make menuconfig to debug the situation.
It is true when I start with a .config created from 'make qcom_defconfig', then select OF_UNITTEST, which is the only way I can see OF_DYNAMIC. It is the "if OF_UNITTEST" that means SYSFS can not be disabled.

Not really. Disabling SYSFS has nothing to do with the DT code. It's not super easy though. It required setting EXPERT and disabling CONFIGFS_FS and things selecting it (PCIE endpoint and USB gadget).

Quoted 4 lines

If I start with the .config that Marc supplied, then make menuconfig still does not show the OC_DYNAMIC option, but leaves it set since it was already set. In this case SYSFS remains disabled because OF_UNITTEST is also disabled.

I don't see the relationship between SYSFS and OF_UNITTEST.

Quoted 10 lines

Using '/OF_DYNAMIC' from within make menuconfig, to get more info about OF_DYNAMIC tells me that the prompt for OF_DYNAMIC is visible if OF && OF_UNITTEST. This is due to the "if OF_UNITTEST" in line 58 of drivers/of/Kconfig in the OF_DYNAMIC specification.
Thus I can't figure out how to use make menuconfig to set OF_DYNAMIC without setting OF_UNITTEST. I tried setting OF_UNITTEST, then setting OF_DYNAMIC, saving the changes, then another make menuconfig to disable OF_UNITTEST, which then has the side effect of unsetting OF_DYNAMIC.

Selecting OF_OVERLAY also.

Quoted 2 lines

Would you accept a patch that removes the "if OF_UNITTEST" from the Kconfig entry for OF_DYNAMIC?

I guess. The purpose of making it visible was for compile testing, so maybe make it 'if COMPILE_TEST' instead?

Looking at occurrences of CONFIG_OF_DYNAMIC, there's more than I'd like. This for example is a common pattern:

drivers/spi/spi.c: if (IS_ENABLED(CONFIG_OF_DYNAMIC))
drivers/spi/spi.c- WARN_ON(of_reconfig_notifier_register(&spi_of_notifier));

Really, of_reconfig_notifier_register() should just return 0 if !IS_ENABLED(CONFIG_OF_DYNAMIC).

Rob

Frank Rowand

2021-08-23 22:21 UTC #6 lore raw

Quoted 52 lines · Rob Herring

Quoted 39 lines · Frank Rowand
Quoted 28 lines · Rob Herring
Quoted 27 lines · Frank Rowand
Hi Marc,
Quoted 24 lines · Marc Zyngier
Trying to boot without SYSFS, but with OF_DYNAMIC quickly results in a crash:
[    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
[...]
[    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
[    0.105810] Hardware name: linux,dummy-virt (DT)
[  0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
[    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
[...]
[    0.134087] Call trace:
[    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
[    0.136054]  safe_name+0x4c/0xd0
[    0.136994]  __of_attach_node_sysfs+0xf8/0x124
[    0.138287]  of_core_init+0x90/0xfc
[    0.139296]  driver_init+0x30/0x4c
[    0.140283]  kernel_init_freeable+0x160/0x1b8
[    0.141543]  kernel_init+0x30/0x140
[    0.142561]  ret_from_fork+0x10/0x18
While not having sysfs isn't a very common option these days, it is still expected that such configuration would work.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
Paper over it by bailing out from __of_attach_node_sysfs() if CONFIG_SYSFS isn't enabled.
CONFIG_SYSFS should be automatically selected when CONFIG_OF_DYNAMIC is enabled, and it should not be possible to disable CONFIG_SYSFS in this case.
That used to be true, but isn't now.
Quoted 7 lines
OK. I agree with you, but when I investigated the original patch email I came to a different conclusion because of the way that I used make menuconfig to debug the situation.
It is true when I start with a .config created from 'make qcom_defconfig', then select OF_UNITTEST, which is the only way I can see OF_DYNAMIC. It is the "if OF_UNITTEST" that means SYSFS can not be disabled.
Not really. Disabling SYSFS has nothing to do with the DT code. It's not super easy though. It required setting EXPERT and disabling CONFIGFS_FS and things selecting it (PCIE endpoint and USB gadget).

Quoted 6 lines

Quoted 4 lines
If I start with the .config that Marc supplied, then make menuconfig still does not show the OC_DYNAMIC option, but leaves it set since it was already set. In this case SYSFS remains disabled because OF_UNITTEST is also disabled.
I don't see the relationship between SYSFS and OF_UNITTEST.

I don't either. Other than the results of experimenting showing that if I enable OF_UNITTEST then I can no long disable SYSFS.

Quoted 12 lines

Quoted 11 lines
Using '/OF_DYNAMIC' from within make menuconfig, to get more info about OF_DYNAMIC tells me that the prompt for OF_DYNAMIC is visible if OF && OF_UNITTEST. This is due to the "if OF_UNITTEST" in line 58 of drivers/of/Kconfig in the OF_DYNAMIC specification.
Thus I can't figure out how to use make menuconfig to set OF_DYNAMIC without setting OF_UNITTEST. I tried setting OF_UNITTEST, then setting OF_DYNAMIC, saving the changes, then another make menuconfig to disable OF_UNITTEST, which then has the side effect of unsetting OF_DYNAMIC.

Quoted 7 lines

Selecting OF_OVERLAY also.
Quoted 2 lines
Would you accept a patch that removes the "if OF_UNITTEST" from the Kconfig entry for OF_DYNAMIC?
I guess. The purpose of making it visible was for compile testing, so maybe make it 'if COMPILE_TEST' instead?

After thinking a bit, I didn't like my original suggestion. I'll think some more, but I think that I will not follow up on my original suggestion.

Quoted 11 lines

Looking at occurrences of CONFIG_OF_DYNAMIC, there's more than I'd like. This for example is a common pattern:
drivers/spi/spi.c: if (IS_ENABLED(CONFIG_OF_DYNAMIC))
drivers/spi/spi.c- WARN_ON(of_reconfig_notifier_register(&spi_of_notifier));
Really, of_reconfig_notifier_register() should just return 0 if !IS_ENABLED(CONFIG_OF_DYNAMIC).

I'll add researching use of CONFIG_OF_DYNAMIC to my todo list.

-Frank

Quoted 3 lines

Rob

Rob Herring

maintainer Applied

2021-08-23 18:49 UTC #7 lore raw

In reply to Marc Zyngier

Quoted 24 lines · Marc Zyngier

Trying to boot without SYSFS, but with OF_DYNAMIC quickly results in a crash:

[    0.088460] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
[...]
[    0.103927] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.14.0-rc3 #4179
[    0.105810] Hardware name: linux,dummy-virt (DT)
[    0.107147] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[    0.108876] pc : kernfs_find_and_get_ns+0x3c/0x7c
[    0.110244] lr : kernfs_find_and_get_ns+0x3c/0x7c
[...]
[    0.134087] Call trace:
[    0.134800]  kernfs_find_and_get_ns+0x3c/0x7c
[    0.136054]  safe_name+0x4c/0xd0
[    0.136994]  __of_attach_node_sysfs+0xf8/0x124
[    0.138287]  of_core_init+0x90/0xfc
[    0.139296]  driver_init+0x30/0x4c
[    0.140283]  kernel_init_freeable+0x160/0x1b8
[    0.141543]  kernel_init+0x30/0x140
[    0.142561]  ret_from_fork+0x10/0x18

While not having sysfs isn't a very common option these days, it is still expected that such configuration would work.

Quoted 8 lines

Paper over it by bailing out from __of_attach_node_sysfs() if CONFIG_SYSFS isn't enabled.
Signed-off-by Marc Zyngier <[email protected]>
---
 drivers/of/kobj.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks!