Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] lib: vsprintf: check for NULL device_node name in device_node_string()
@ 2021-02-17 12:15 Enrico Weigelt, metux IT consult
  2021-02-17 13:50 ` Andy Shevchenko
  2021-02-18  0:47 ` Sergey Senozhatsky
  0 siblings, 2 replies; 6+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-02-17 12:15 UTC (permalink / raw)
  To: linux-kernel
  Cc: pmladek, rostedt, sergey.senozhatsky, andriy.shevchenko, linux,
	kafai, songliubraving, yhs, john.fastabend, kpsingh, netdev, bpf

Under rare circumstances it may happen that a device node's name is NULL
(most likely kernel bug in some other place). In such situations anything
but helpful, if the debug printout crashes, and nobody knows what actually
happened here.

Therefore protect it by an explicit NULL check and print out an extra
warning.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
 lib/vsprintf.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3b53c73580c5..050a60b88073 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2013,6 +2013,10 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 			break;
 		case 'n':	/* name */
 			p = fwnode_get_name(of_fwnode_handle(dn));
+			if (!p) {
+				pr_warn("device_node without name. Kernel bug ?\n");
+				p = "<NULL>";
+			}
 			precision = str_spec.precision;
 			str_spec.precision = strchrnul(p, '@') - p;
 			buf = string(buf, end, p, str_spec);
-- 
2.11.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] lib: vsprintf: check for NULL device_node name in device_node_string()
  2021-02-17 12:15 [PATCH] lib: vsprintf: check for NULL device_node name in device_node_string() Enrico Weigelt, metux IT consult
@ 2021-02-17 13:50 ` Andy Shevchenko
  2021-02-18 12:53   ` Petr Mladek
  2021-02-23 19:54   ` Enrico Weigelt, metux IT consult
  2021-02-18  0:47 ` Sergey Senozhatsky
  1 sibling, 2 replies; 6+ messages in thread
From: Andy Shevchenko @ 2021-02-17 13:50 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: linux-kernel, pmladek, rostedt, sergey.senozhatsky, linux, kafai,
	songliubraving, yhs, john.fastabend, kpsingh, netdev, bpf

On Wed, Feb 17, 2021 at 01:15:43PM +0100, Enrico Weigelt, metux IT consult wrote:
> Under rare circumstances it may happen that a device node's name is NULL
> (most likely kernel bug in some other place).

What circumstances? How can I reproduce this? More information, please!

> In such situations anything
> but helpful, if the debug printout crashes, and nobody knows what actually
> happened here.
> 
> Therefore protect it by an explicit NULL check and print out an extra
> warning.

...

> +				pr_warn("device_node without name. Kernel bug ?\n");

If it's not once, then it's possible to have log spammed with this, right?

...

> +				p = "<NULL>";

We have different standard de facto for NULL pointers to be printed. Actually
if you wish, you may gather them under one definition (maybe somewhere under
printk) and export to everybody to use.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] lib: vsprintf: check for NULL device_node name in device_node_string()
  2021-02-17 12:15 [PATCH] lib: vsprintf: check for NULL device_node name in device_node_string() Enrico Weigelt, metux IT consult
  2021-02-17 13:50 ` Andy Shevchenko
@ 2021-02-18  0:47 ` Sergey Senozhatsky
  1 sibling, 0 replies; 6+ messages in thread
From: Sergey Senozhatsky @ 2021-02-18  0:47 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: linux-kernel, pmladek, rostedt, sergey.senozhatsky,
	andriy.shevchenko, linux, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, netdev, bpf

On (21/02/17 13:15), Enrico Weigelt, metux IT consult wrote:
> Under rare circumstances it may happen that a device node's name is NULL
> (most likely kernel bug in some other place). In such situations anything
> but helpful, if the debug printout crashes, and nobody knows what actually
> happened here.
> 
> Therefore protect it by an explicit NULL check and print out an extra
> warning.
> 
> Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
> ---
>  lib/vsprintf.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 3b53c73580c5..050a60b88073 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2013,6 +2013,10 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
>  			break;
>  		case 'n':	/* name */
>  			p = fwnode_get_name(of_fwnode_handle(dn));
> +			if (!p) {
> +				pr_warn("device_node without name. Kernel bug ?\n");
> +				p = "<NULL>";
> +			}
>  			precision = str_spec.precision;
>  			str_spec.precision = strchrnul(p, '@') - p;
>  			buf = string(buf, end, p, str_spec);

What about other fwnode_get_name() calls in vsprintf?

	-ss

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] lib: vsprintf: check for NULL device_node name in device_node_string()
  2021-02-17 13:50 ` Andy Shevchenko
@ 2021-02-18 12:53   ` Petr Mladek
  2021-02-23 19:54     ` Enrico Weigelt, metux IT consult
  2021-02-23 19:54   ` Enrico Weigelt, metux IT consult
  1 sibling, 1 reply; 6+ messages in thread
From: Petr Mladek @ 2021-02-18 12:53 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Enrico Weigelt, metux IT consult, linux-kernel, rostedt,
	sergey.senozhatsky, linux, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, netdev, bpf

On Wed 2021-02-17 15:50:00, Andy Shevchenko wrote:
> On Wed, Feb 17, 2021 at 01:15:43PM +0100, Enrico Weigelt, metux IT consult wrote:
> > Under rare circumstances it may happen that a device node's name is NULL
> > (most likely kernel bug in some other place).
> 
> What circumstances? How can I reproduce this? More information, please!
> 
> > In such situations anything
> > but helpful, if the debug printout crashes, and nobody knows what actually
> > happened here.
> > 
> > Therefore protect it by an explicit NULL check and print out an extra
> > warning.
> 
> ...
> 
> > +				pr_warn("device_node without name. Kernel bug ?\n");
> 
> If it's not once, then it's possible to have log spammed with this, right?
> 
> ...
> 
> > +				p = "<NULL>";
> 
> We have different standard de facto for NULL pointers to be printed. Actually
> if you wish, you may gather them under one definition (maybe somewhere under
> printk) and export to everybody to use.

Please, use

	if (check_pointer(&buf, end, p, spec))
		return buf;

It will print "(null)" instead of the name. It should be enough
to inform the user this way. The extra pr_warn() does not help
much to localize the problem anyway. And it is better to avoid
recursion in this path.

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] lib: vsprintf: check for NULL device_node name in device_node_string()
  2021-02-17 13:50 ` Andy Shevchenko
  2021-02-18 12:53   ` Petr Mladek
@ 2021-02-23 19:54   ` Enrico Weigelt, metux IT consult
  1 sibling, 0 replies; 6+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-02-23 19:54 UTC (permalink / raw)
  To: Andy Shevchenko, Enrico Weigelt, metux IT consult
  Cc: linux-kernel, pmladek, rostedt, sergey.senozhatsky, linux, kafai,
	songliubraving, yhs, john.fastabend, kpsingh, netdev, bpf

On 17.02.21 14:50, Andy Shevchenko wrote:
> On Wed, Feb 17, 2021 at 01:15:43PM +0100, Enrico Weigelt, metux IT consult wrote:
>> Under rare circumstances it may happen that a device node's name is NULL
>> (most likely kernel bug in some other place).
> 
> What circumstances? How can I reproduce this? More information, please!

Observed it when applying a broken overlay. (sorry, didn't keep that
broken code :o). In this case, the device_node was left without a name
(pointing to NULL).

>> +				pr_warn("device_node without name. Kernel bug ?\n");
> 
> If it's not once, then it's possible to have log spammed with this, right?

It only has occoured once for me. I don't think spamming could happen,
unless one's hacking deeply in the oftree code.

>> +				p = "<NULL>";
> 
> We have different standard de facto for NULL pointers to be printed. Actually
> if you wish, you may gather them under one definition (maybe somewhere under
> printk) and export to everybody to use.

Seen it in Petr's reply ... going to use that in v2.

--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] lib: vsprintf: check for NULL device_node name in device_node_string()
  2021-02-18 12:53   ` Petr Mladek
@ 2021-02-23 19:54     ` Enrico Weigelt, metux IT consult
  0 siblings, 0 replies; 6+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-02-23 19:54 UTC (permalink / raw)
  To: Petr Mladek, Andy Shevchenko
  Cc: Enrico Weigelt, metux IT consult, linux-kernel, rostedt,
	sergey.senozhatsky, linux, kafai, songliubraving, yhs,
	john.fastabend, kpsingh, netdev, bpf

On 18.02.21 13:53, Petr Mladek wrote:

> Please, use
> 
> 	if (check_pointer(&buf, end, p, spec))
> 		return buf;
> 
> It will print "(null)" instead of the name. It should be enough
> to inform the user this way. The extra pr_warn() does not help
> much to localize the problem anyway. And it is better to avoid
> recursion in this path.

thx, going to use it in v2.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-17 12:15 [PATCH] lib: vsprintf: check for NULL device_node name in device_node_string() Enrico Weigelt, metux IT consult
2021-02-17 13:50 ` Andy Shevchenko
2021-02-18 12:53   ` Petr Mladek
2021-02-23 19:54     ` Enrico Weigelt, metux IT consult
2021-02-23 19:54   ` Enrico Weigelt, metux IT consult
2021-02-18  0:47 ` Sergey Senozhatsky

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git