linux-kernel - Re: [PATCH v2 5/8] printk: move printk sysctl to printk/sysctl.c

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] [day] [month] [year] [list]

Message-ID: <YaU8oTPBiWTP/4Ll@bombadil.infradead.org>
Date:   Mon, 29 Nov 2021 12:48:33 -0800
From:   Luis Chamberlain <mcgrof@...nel.org>
To:     Petr Mladek <pmladek@...e.com>
Cc:     akpm@...ux-foundation.org, keescook@...omium.org,
        yzaikin@...gle.com, nixiaoming@...wei.com, ebiederm@...ssion.com,
        steve@....org, gregkh@...uxfoundation.org, rafael@...nel.org,
        tytso@....edu, viro@...iv.linux.org.uk, senozhatsky@...omium.org,
        rostedt@...dmis.org, john.ogness@...utronix.de,
        dgilbert@...erlog.com, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, linux-scsi@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/8] printk: move printk sysctl to printk/sysctl.c

On Fri, Nov 26, 2021 at 01:51:38PM +0100, Petr Mladek wrote:
> On Wed 2021-11-24 15:14:32, Luis Chamberlain wrote:
> > From: Xiaoming Ni <nixiaoming@...wei.com>
> > 
> > The kernel/sysctl.c is a kitchen sink where everyone leaves
> > their dirty dishes, this makes it very difficult to maintain.
> > 
> > To help with this maintenance let's start by moving sysctls to
> > places where they actually belong. The proc sysctl maintainers
> > do not want to know what sysctl knobs you wish to add for your own
> > piece of code, we just care about the core logic.
> > 
> > So move printk sysctl from kernel/sysctl.c to kernel/printk/sysctl.c.
> > Use register_sysctl() to register the sysctl interface.
> > 
> > diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
> > index d118739874c0..f5b388e810b9 100644
> > --- a/kernel/printk/Makefile
> > +++ b/kernel/printk/Makefile
> > @@ -2,5 +2,8 @@
> >  obj-y	= printk.o
> >  obj-$(CONFIG_PRINTK)	+= printk_safe.o
> >  obj-$(CONFIG_A11Y_BRAILLE_CONSOLE)	+= braille.o
> > -obj-$(CONFIG_PRINTK)	+= printk_ringbuffer.o
> >  obj-$(CONFIG_PRINTK_INDEX)	+= index.o
> > +
> > +obj-$(CONFIG_PRINTK)                 += printk_support.o
> > +printk_support-y	             := printk_ringbuffer.o
> > +printk_support-$(CONFIG_SYSCTL)	     += sysctl.o
> 
> I have never seen this trick. It looks like a dirty hack ;-)

It has been used in mac80211 for over a decade now :) See
net/mac80211/Makefile

> Anyway, I do not see it described in the documentation. I wonder
> if it works only by chance.
> 
> IMHO, a cleaner solution would be to add the following
> into init/Kconfig:
> 
> config BUILD_PRINTK_SYSCTL
> 	bool
> 	default (PRINTK && SYSCTL)
> 
> and then use:
> 
> obj-$(CONFIG_BUILD_PRINTK_SYSCTL)    += sysctl.o

I suppose it is a matter of taste, either way works with me,
but I think less kconfig logic is better here.

> > diff --git a/kernel/printk/sysctl.c b/kernel/printk/sysctl.c
> > new file mode 100644
> > index 000000000000..653ae04aab7f
> > --- /dev/null
> > +++ b/kernel/printk/sysctl.c
> > @@ -0,0 +1,85 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * sysctl.c: General linux system control interface
> > + */
> > +
> > +#include <linux/sysctl.h>
> > +#include <linux/printk.h>
> > +#include <linux/capability.h>
> > +#include <linux/ratelimit.h>
> > +#include "internal.h"
> > +
> > +static const int ten_thousand = 10000;
> 
> The patch should also remove the variable in kernel/sysctl.c.
> 
> Otherwise, it looks like a really nice clean up.

Ah yes that variable is now unused there. Thanks

  Luis