linux-kernel - Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for per-console loglevels

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: <Zz1tOxW6PO_2OeSA@chrisdown.name>
Date: Wed, 20 Nov 2024 05:01:47 +0000
From: Chris Down <chris@...isdown.name>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Petr Mladek <pmladek@...e.com>, linux-kernel@...r.kernel.org,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Tony Lindgren <tony.lindgren@...ux.intel.com>, kernel-team@...com
Subject: Re: [PATCH v6 06/11] printk: console: Introduce sysfs interface for
 per-console loglevels

Thanks for looking this over :-) All not mentioned points in this reply are 
acked.

Greg Kroah-Hartman writes:
>> diff --git a/Documentation/ABI/testing/sysfs-class-console b/Documentation/ABI/testing/sysfs-class-console
>> new file mode 100644
>> index 000000000000..40b90b190af3
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-console
>> @@ -0,0 +1,47 @@
>> +What:		/sys/class/console/
>> +Date:		October 2024
>
>It's no longer October 2024 :(

What would you recommend? When I sent them it was, and it doesn't seem 
realistic to think that it's going to be less than one month from me sending 
the patches to when it gets merged, no?

>> +What:		/sys/class/console/<C>/loglevel
>> +Date:		October 2024
>> +Contact:	Chris Down <chris@...isdown.name>
>> +Description:	Read write. The current per-console loglevel, which will take
>> +		effect if not overridden by other non-sysfs controls (see
>> +		Documentation/admin-guide/per-console-loglevel.rst). Bounds are
>> +		0 (LOGLEVEL_EMERG) to 8 (LOGLEVEL_DEBUG + 1) inclusive. Also
>> +		takes the special value "-1" to indicate that no per-console
>> +		loglevel is set, and we should defer to the global controls.
>
>-1 is odd, why?  That's going to confuse everyone :(

I originally had it that you had to send "unset" instead of -1, but in 
discussion with Petr it was suggested to change it to -1.

Petr, what do you think?

>> +	if (console->classdev)
>> +		device_unregister(console->classdev);
>
>How could this be NULL?

I think it's from an earlier version of the patch where we would still continue 
setup even if we couldn't allocate it. I'm okay removing it.

>> +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
>> +			     char *buf)
>> +{
>> +	struct console *con = dev_get_drvdata(dev);
>> +
>> +	return sysfs_emit(buf, "%d\n", READ_ONCE(con->level));
>
>While I admire the use of READ_ONCE() properly, it really doesn't matter
>for sysfs as it could change right afterwards and no one cares.  So no
>need for that here, right?

I'm not sure I understand, could you clarify? From my reading of the code it 
looks like we need this to avoid tearing.