[PATCH 0/4] Move the sysctl interface to the corresponding feature code file

[PATCH 0/4] Move the sysctl interface to the corresponding feature code file

[Xiaoming Ni]

Use register_sysctl() to register the sysctl interface to avoid
merge conflicts when different features modify sysctl.c at the same time.

Here, the sysctl interfaces of hung task and watchdog are moved to the
corresponding feature code files

https://lkml.org/lkml/2020/5/11/1419

Xiaoming Ni (4):
  hung_task: Move hung_task syscl interface to hung_task_sysctl.c
  proc/sysctl: add shared variables -1
  watchdog: move watchdog sysctl to watchdog.c
  sysctl: Add register_sysctl_init() interface

 fs/proc/proc_sysctl.c        |   2 +-
 include/linux/sched/sysctl.h |   8 +--
 include/linux/sysctl.h       |   3 +
 kernel/Makefile              |   4 +-
 kernel/hung_task.c           |   6 +-
 kernel/hung_task.h           |  21 ++++++
 kernel/hung_task_sysctl.c    |  66 +++++++++++++++++
 kernel/sysctl.c              | 168 ++++++-------------------------------------
 kernel/watchdog.c            | 101 ++++++++++++++++++++++++++
 9 files changed, 219 insertions(+), 160 deletions(-)
 create mode 100644 kernel/hung_task.h
 create mode 100644 kernel/hung_task_sysctl.c

-- 
1.8.5.6

[PATCH 1/4] hung_task: Move hung_task sysctl interface to hung_task_sysctl.c

[Xiaoming Ni]

Move hung_task sysctl interface to hung_task_sysctl.c.
Use register_sysctl() to register the sysctl interface to avoid
merge conflicts when different features modify sysctl.c at the same time.

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
 include/linux/sched/sysctl.h |  8 +----
 kernel/Makefile              |  4 ++-
 kernel/hung_task.c           |  6 ++--
 kernel/hung_task.h           | 21 ++++++++++++
 kernel/hung_task_sysctl.c    | 80 ++++++++++++++++++++++++++++++++++++++++++++
 kernel/sysctl.c              | 50 ---------------------------
 6 files changed, 108 insertions(+), 61 deletions(-)
 create mode 100644 kernel/hung_task.h
 create mode 100644 kernel/hung_task_sysctl.c

diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
index d4f6215..c075e09 100644
--- a/include/linux/sched/sysctl.h
+++ b/include/linux/sched/sysctl.h
@@ -7,14 +7,8 @@
 struct ctl_table;
 
 #ifdef CONFIG_DETECT_HUNG_TASK
-extern int	     sysctl_hung_task_check_count;
-extern unsigned int  sysctl_hung_task_panic;
+/* used for hung_task and block/ */
 extern unsigned long sysctl_hung_task_timeout_secs;
-extern unsigned long sysctl_hung_task_check_interval_secs;
-extern int sysctl_hung_task_warnings;
-extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int write,
-					 void __user *buffer,
-					 size_t *lenp, loff_t *ppos);
 #else
 /* Avoid need for ifdefs elsewhere in the code */
 enum { sysctl_hung_task_timeout_secs = 0 };
diff --git a/kernel/Makefile b/kernel/Makefile
index 4cb4130..c16934bf 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -84,7 +84,9 @@ obj-$(CONFIG_KCOV) += kcov.o
 obj-$(CONFIG_KPROBES) += kprobes.o
 obj-$(CONFIG_FAIL_FUNCTION) += fail_function.o
 obj-$(CONFIG_KGDB) += debug/
-obj-$(CONFIG_DETECT_HUNG_TASK) += hung_task.o
+obj-$(CONFIG_DETECT_HUNG_TASK) += hung_tasks.o
+hung_tasks-y := hung_task.o
+hung_tasks-$(CONFIG_SYSCTL) += hung_task_sysctl.o
 obj-$(CONFIG_LOCKUP_DETECTOR) += watchdog.o
 obj-$(CONFIG_HARDLOCKUP_DETECTOR_PERF) += watchdog_hld.o
 obj-$(CONFIG_SECCOMP) += seccomp.o
diff --git a/kernel/hung_task.c b/kernel/hung_task.c
index 14a625c..bfe6e25 100644
--- a/kernel/hung_task.c
+++ b/kernel/hung_task.c
@@ -15,15 +15,13 @@
 #include <linux/kthread.h>
 #include <linux/lockdep.h>
 #include <linux/export.h>
-#include <linux/sysctl.h>
 #include <linux/suspend.h>
 #include <linux/utsname.h>
 #include <linux/sched/signal.h>
 #include <linux/sched/debug.h>
-#include <linux/sched/sysctl.h>
 
 #include <trace/events/sched.h>
-
+#include "hung_task.h"
 /*
  * The number of tasks checked:
  */
@@ -298,6 +296,8 @@ static int watchdog(void *dummy)
 
 static int __init hung_task_init(void)
 {
+	hung_task_sysctl_init();
+
 	atomic_notifier_chain_register(&panic_notifier_list, &panic_block);
 
 	/* Disable hung task detector on suspend */
diff --git a/kernel/hung_task.h b/kernel/hung_task.h
new file mode 100644
index 00000000..2fa6a19
--- /dev/null
+++ b/kernel/hung_task.h
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * hung_task sysctl data and function
+ */
+#ifndef KERNEL_HUNG_TASK_H_
+#define KERNEL_HUNG_TASK_H_
+#include <linux/sched/sysctl.h>
+extern int      sysctl_hung_task_check_count;
+extern unsigned int  sysctl_hung_task_panic;
+extern unsigned long sysctl_hung_task_check_interval_secs;
+extern int sysctl_hung_task_warnings;
+extern int proc_dohung_task_timeout_secs(struct ctl_table *table, int write,
+					 void __user *buffer,
+					 size_t *lenp, loff_t *ppos);
+#ifdef CONFIG_SYSCTL
+extern void __init hung_task_sysctl_init(void);
+#else
+#define hung_task_sysctl_init() do { } while (0)
+#endif
+
+#endif
diff --git a/kernel/hung_task_sysctl.c b/kernel/hung_task_sysctl.c
new file mode 100644
index 00000000..5b10d4e
--- /dev/null
+++ b/kernel/hung_task_sysctl.c
@@ -0,0 +1,80 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * register sysctl for hung_task
+ *
+ */
+
+#include <linux/sysctl.h>
+#include <linux/sched/sysctl.h>
+#include <linux/kmemleak.h>
+#include "hung_task.h"
+
+/*
+ * This is needed for proc_doulongvec_minmax of sysctl_hung_task_timeout_secs
+ * and hung_task_check_interval_secs
+ */
+static unsigned long hung_task_timeout_max = (LONG_MAX / HZ);
+static int neg_one = -1;
+static struct ctl_table hung_task_sysctls[] = {
+	{
+		.procname	= "hung_task_panic",
+		.data		= &sysctl_hung_task_panic,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+	{
+		.procname	= "hung_task_check_count",
+		.data		= &sysctl_hung_task_check_count,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+	},
+	{
+		.procname	= "hung_task_timeout_secs",
+		.data		= &sysctl_hung_task_timeout_secs,
+		.maxlen		= sizeof(unsigned long),
+		.mode		= 0644,
+		.proc_handler	= proc_dohung_task_timeout_secs,
+		.extra2		= &hung_task_timeout_max,
+	},
+	{
+		.procname	= "hung_task_check_interval_secs",
+		.data		= &sysctl_hung_task_check_interval_secs,
+		.maxlen		= sizeof(unsigned long),
+		.mode		= 0644,
+		.proc_handler	= proc_dohung_task_timeout_secs,
+		.extra2		= &hung_task_timeout_max,
+	},
+	{
+		.procname	= "hung_task_warnings",
+		.data		= &sysctl_hung_task_warnings,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &neg_one,
+	},
+	{}
+};
+
+/*
+ * The hung task sysctl has a default value.
+ * Even if register_sysctl() fails, it does not affect the main function of
+ * the hung task. At the same time, during the system initialization phase,
+ * malloc small memory will almost never fail.
+ * So the return value is ignored here
+ */
+void __init hung_task_sysctl_init(void)
+{
+	struct ctl_table_header *srt = register_sysctl("kernel", hung_task_sysctls);
+
+	if (unlikely(!srt)) {
+		pr_err("%s fail\n", __func__);
+		return;
+	}
+	kmemleak_not_leak(srt);
+}
+
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b9ff323..20adae0 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -149,13 +149,6 @@
 static int ngroups_max = NGROUPS_MAX;
 static const int cap_last_cap = CAP_LAST_CAP;
 
-/*
- * This is needed for proc_doulongvec_minmax of sysctl_hung_task_timeout_secs
- * and hung_task_check_interval_secs
- */
-#ifdef CONFIG_DETECT_HUNG_TASK
-static unsigned long hung_task_timeout_max = (LONG_MAX/HZ);
-#endif
 
 #ifdef CONFIG_INOTIFY_USER
 #include <linux/inotify.h>
@@ -1087,49 +1080,6 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
 		.proc_handler	= proc_dointvec,
 	},
 #endif
-#ifdef CONFIG_DETECT_HUNG_TASK
-	{
-		.procname	= "hung_task_panic",
-		.data		= &sysctl_hung_task_panic,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_ONE,
-	},
-	{
-		.procname	= "hung_task_check_count",
-		.data		= &sysctl_hung_task_check_count,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= SYSCTL_ZERO,
-	},
-	{
-		.procname	= "hung_task_timeout_secs",
-		.data		= &sysctl_hung_task_timeout_secs,
-		.maxlen		= sizeof(unsigned long),
-		.mode		= 0644,
-		.proc_handler	= proc_dohung_task_timeout_secs,
-		.extra2		= &hung_task_timeout_max,
-	},
-	{
-		.procname	= "hung_task_check_interval_secs",
-		.data		= &sysctl_hung_task_check_interval_secs,
-		.maxlen		= sizeof(unsigned long),
-		.mode		= 0644,
-		.proc_handler	= proc_dohung_task_timeout_secs,
-		.extra2		= &hung_task_timeout_max,
-	},
-	{
-		.procname	= "hung_task_warnings",
-		.data		= &sysctl_hung_task_warnings,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &neg_one,
-	},
-#endif
 #ifdef CONFIG_RT_MUTEXES
 	{
 		.procname	= "max_lock_depth",
-- 
1.8.5.6

Re: [PATCH 1/4] hung_task: Move hung_task sysctl interface to hung_task_sysctl.c

[Kees Cook]

On Fri, May 15, 2020 at 12:33:41PM +0800, Xiaoming Ni wrote:
> Move hung_task sysctl interface to hung_task_sysctl.c.
> Use register_sysctl() to register the sysctl interface to avoid
> merge conflicts when different features modify sysctl.c at the same time.
> 
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> ---
>  include/linux/sched/sysctl.h |  8 +----
>  kernel/Makefile              |  4 ++-
>  kernel/hung_task.c           |  6 ++--
>  kernel/hung_task.h           | 21 ++++++++++++
>  kernel/hung_task_sysctl.c    | 80 ++++++++++++++++++++++++++++++++++++++++++++

Why a separate file? That ends up needing changes to Makefile, the
creation of a new header file, etc. Why not just put it all into
hung_task.c directly?

-Kees

-- 
Kees Cook

Re: [PATCH 1/4] hung_task: Move hung_task sysctl interface to hung_task_sysctl.c

[Xiaoming Ni]

On 2020/5/15 16:04, Kees Cook wrote:
> On Fri, May 15, 2020 at 12:33:41PM +0800, Xiaoming Ni wrote:
>> Move hung_task sysctl interface to hung_task_sysctl.c.
>> Use register_sysctl() to register the sysctl interface to avoid
>> merge conflicts when different features modify sysctl.c at the same time.
>>
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>> ---
>>   include/linux/sched/sysctl.h |  8 +----
>>   kernel/Makefile              |  4 ++-
>>   kernel/hung_task.c           |  6 ++--
>>   kernel/hung_task.h           | 21 ++++++++++++
>>   kernel/hung_task_sysctl.c    | 80 ++++++++++++++++++++++++++++++++++++++++++++
> 
> Why a separate file? That ends up needing changes to Makefile, the
> creation of a new header file, etc. Why not just put it all into
> hung_task.c directly?
> 
> -Kees
> 
But Luis Chamberlain's suggestion is to put the hung_task sysctl code in 
a separate file. Details are in https://lkml.org/lkml/2020/5/13/762.
I am a little confused, not sure which way is better.

Thanks
Xiaoming Ni

Re: [PATCH 1/4] hung_task: Move hung_task sysctl interface to hung_task_sysctl.c

[Kees Cook]

On Fri, May 15, 2020 at 04:56:34PM +0800, Xiaoming Ni wrote:
> On 2020/5/15 16:04, Kees Cook wrote:
> > On Fri, May 15, 2020 at 12:33:41PM +0800, Xiaoming Ni wrote:
> > > Move hung_task sysctl interface to hung_task_sysctl.c.
> > > Use register_sysctl() to register the sysctl interface to avoid
> > > merge conflicts when different features modify sysctl.c at the same time.
> > > 
> > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> > > ---
> > >   include/linux/sched/sysctl.h |  8 +----
> > >   kernel/Makefile              |  4 ++-
> > >   kernel/hung_task.c           |  6 ++--
> > >   kernel/hung_task.h           | 21 ++++++++++++
> > >   kernel/hung_task_sysctl.c    | 80 ++++++++++++++++++++++++++++++++++++++++++++
> > 
> > Why a separate file? That ends up needing changes to Makefile, the
> > creation of a new header file, etc. Why not just put it all into
> > hung_task.c directly?
> > 
> > -Kees
> > 
> But Luis Chamberlain's suggestion is to put the hung_task sysctl code in a
> separate file. Details are in https://lkml.org/lkml/2020/5/13/762.
> I am a little confused, not sure which way is better.

Ah, yes, I see now. Luis, I disagree with your recommendation here -- I
think complicating the Makefile is much less intuitive than just
wrapping this code in an #ifdef block in the existing .c file.

-- 
Kees Cook

Re: [PATCH 1/4] hung_task: Move hung_task sysctl interface to hung_task_sysctl.c

[Luis Chamberlain]

On Fri, May 15, 2020 at 09:03:54AM -0700, Kees Cook wrote:
> On Fri, May 15, 2020 at 04:56:34PM +0800, Xiaoming Ni wrote:
> > On 2020/5/15 16:04, Kees Cook wrote:
> > > On Fri, May 15, 2020 at 12:33:41PM +0800, Xiaoming Ni wrote:
> > > > Move hung_task sysctl interface to hung_task_sysctl.c.
> > > > Use register_sysctl() to register the sysctl interface to avoid
> > > > merge conflicts when different features modify sysctl.c at the same time.
> > > > 
> > > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> > > > ---
> > > >   include/linux/sched/sysctl.h |  8 +----
> > > >   kernel/Makefile              |  4 ++-
> > > >   kernel/hung_task.c           |  6 ++--
> > > >   kernel/hung_task.h           | 21 ++++++++++++
> > > >   kernel/hung_task_sysctl.c    | 80 ++++++++++++++++++++++++++++++++++++++++++++
> > > 
> > > Why a separate file? That ends up needing changes to Makefile, the
> > > creation of a new header file, etc. Why not just put it all into
> > > hung_task.c directly?
> > > 
> > > -Kees
> > > 
> > But Luis Chamberlain's suggestion is to put the hung_task sysctl code in a
> > separate file. Details are in https://lkml.org/lkml/2020/5/13/762.
> > I am a little confused, not sure which way is better.
> 
> Ah, yes, I see now. Luis, I disagree with your recommendation here -- I
> think complicating the Makefile is much less intuitive than just
> wrapping this code in an #ifdef block in the existing .c file.

That's fine, sorry for the trouble Xiaoming.

  Luis

[PATCH 2/4] proc/sysctl: add shared variables -1

[Xiaoming Ni]

Add the shared variable SYSCTL_NEG_ONE to replace the variable neg_one
used in both sysctl_writes_strict and hung_task_warnings.

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
 fs/proc/proc_sysctl.c     | 2 +-
 include/linux/sysctl.h    | 1 +
 kernel/hung_task_sysctl.c | 3 +--
 kernel/sysctl.c           | 3 +--
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index b6f5d45..acae1fa 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -23,7 +23,7 @@
 static const struct inode_operations proc_sys_dir_operations;
 
 /* shared constants to be used in various sysctls */
-const int sysctl_vals[] = { 0, 1, INT_MAX };
+const int sysctl_vals[] = { 0, 1, INT_MAX, -1 };
 EXPORT_SYMBOL(sysctl_vals);
 
 /* Support for permanently empty directories */
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 02fa844..6d741d6 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -41,6 +41,7 @@
 #define SYSCTL_ZERO	((void *)&sysctl_vals[0])
 #define SYSCTL_ONE	((void *)&sysctl_vals[1])
 #define SYSCTL_INT_MAX	((void *)&sysctl_vals[2])
+#define SYSCTL_NEG_ONE	((void *)&sysctl_vals[3])
 
 extern const int sysctl_vals[];
 
diff --git a/kernel/hung_task_sysctl.c b/kernel/hung_task_sysctl.c
index 5b10d4e..62a51f5 100644
--- a/kernel/hung_task_sysctl.c
+++ b/kernel/hung_task_sysctl.c
@@ -14,7 +14,6 @@
  * and hung_task_check_interval_secs
  */
 static unsigned long hung_task_timeout_max = (LONG_MAX / HZ);
-static int neg_one = -1;
 static struct ctl_table hung_task_sysctls[] = {
 	{
 		.procname	= "hung_task_panic",
@@ -55,7 +54,7 @@
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &neg_one,
+		.extra1		= SYSCTL_NEG_ONE,
 	},
 	{}
 };
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 20adae0..01fc559 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -124,7 +124,6 @@
 static int sixty = 60;
 #endif
 
-static int __maybe_unused neg_one = -1;
 static int __maybe_unused two = 2;
 static int __maybe_unused four = 4;
 static unsigned long zero_ul;
@@ -540,7 +539,7 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &neg_one,
+		.extra1		= SYSCTL_NEG_ONE,
 		.extra2		= SYSCTL_ONE,
 	},
 #endif
-- 
1.8.5.6

Re: [PATCH 2/4] proc/sysctl: add shared variables -1

[Kees Cook]

On Fri, May 15, 2020 at 12:33:42PM +0800, Xiaoming Ni wrote:
> Add the shared variable SYSCTL_NEG_ONE to replace the variable neg_one
> used in both sysctl_writes_strict and hung_task_warnings.
> 
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> ---
>  fs/proc/proc_sysctl.c     | 2 +-
>  include/linux/sysctl.h    | 1 +
>  kernel/hung_task_sysctl.c | 3 +--
>  kernel/sysctl.c           | 3 +--

How about doing this refactoring in advance of the extraction patch?

>  4 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index b6f5d45..acae1fa 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -23,7 +23,7 @@
>  static const struct inode_operations proc_sys_dir_operations;
>  
>  /* shared constants to be used in various sysctls */
> -const int sysctl_vals[] = { 0, 1, INT_MAX };
> +const int sysctl_vals[] = { 0, 1, INT_MAX, -1 };
>  EXPORT_SYMBOL(sysctl_vals);
>  
>  /* Support for permanently empty directories */
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index 02fa844..6d741d6 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -41,6 +41,7 @@
>  #define SYSCTL_ZERO	((void *)&sysctl_vals[0])
>  #define SYSCTL_ONE	((void *)&sysctl_vals[1])
>  #define SYSCTL_INT_MAX	((void *)&sysctl_vals[2])
> +#define SYSCTL_NEG_ONE	((void *)&sysctl_vals[3])

Nit: let's keep these value-ordered? i.e. -1, 0, 1, INT_MAX.

-- 
Kees Cook

Re: [PATCH 2/4] proc/sysctl: add shared variables -1

[Xiaoming Ni]

On 2020/5/15 16:06, Kees Cook wrote:
> On Fri, May 15, 2020 at 12:33:42PM +0800, Xiaoming Ni wrote:
>> Add the shared variable SYSCTL_NEG_ONE to replace the variable neg_one
>> used in both sysctl_writes_strict and hung_task_warnings.
>>
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>> ---
>>   fs/proc/proc_sysctl.c     | 2 +-
>>   include/linux/sysctl.h    | 1 +
>>   kernel/hung_task_sysctl.c | 3 +--
>>   kernel/sysctl.c           | 3 +--
> 
> How about doing this refactoring in advance of the extraction patch?
Before  advance of the extraction patch, neg_one is only used in one 
file, does it seem to have no value for refactoring?


> 
>>   4 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
>> index b6f5d45..acae1fa 100644
>> --- a/fs/proc/proc_sysctl.c
>> +++ b/fs/proc/proc_sysctl.c
>> @@ -23,7 +23,7 @@
>>   static const struct inode_operations proc_sys_dir_operations;
>>   
>>   /* shared constants to be used in various sysctls */
>> -const int sysctl_vals[] = { 0, 1, INT_MAX };
>> +const int sysctl_vals[] = { 0, 1, INT_MAX, -1 };
>>   EXPORT_SYMBOL(sysctl_vals);
>>   
>>   /* Support for permanently empty directories */
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index 02fa844..6d741d6 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -41,6 +41,7 @@
>>   #define SYSCTL_ZERO	((void *)&sysctl_vals[0])
>>   #define SYSCTL_ONE	((void *)&sysctl_vals[1])
>>   #define SYSCTL_INT_MAX	((void *)&sysctl_vals[2])
>> +#define SYSCTL_NEG_ONE	((void *)&sysctl_vals[3])
> 
> Nit: let's keep these value-ordered? i.e. -1, 0, 1, INT_MAX.
Thanks for guidance, your method is better

Thanks.
Xiaoming Ni

Re: [PATCH 2/4] proc/sysctl: add shared variables -1

[Kees Cook]

On Fri, May 15, 2020 at 05:06:28PM +0800, Xiaoming Ni wrote:
> On 2020/5/15 16:06, Kees Cook wrote:
> > On Fri, May 15, 2020 at 12:33:42PM +0800, Xiaoming Ni wrote:
> > > Add the shared variable SYSCTL_NEG_ONE to replace the variable neg_one
> > > used in both sysctl_writes_strict and hung_task_warnings.
> > > 
> > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> > > ---
> > >   fs/proc/proc_sysctl.c     | 2 +-
> > >   include/linux/sysctl.h    | 1 +
> > >   kernel/hung_task_sysctl.c | 3 +--
> > >   kernel/sysctl.c           | 3 +--
> > 
> > How about doing this refactoring in advance of the extraction patch?
> Before  advance of the extraction patch, neg_one is only used in one file,
> does it seem to have no value for refactoring?

I guess it doesn't matter much, but I think it's easier to review in the
sense that neg_one is first extracted and then later everything else is
moved.

-- 
Kees Cook

Re: [PATCH 2/4] proc/sysctl: add shared variables -1

[Xiaoming Ni]

On 2020/5/16 0:05, Kees Cook wrote:
> On Fri, May 15, 2020 at 05:06:28PM +0800, Xiaoming Ni wrote:
>> On 2020/5/15 16:06, Kees Cook wrote:
>>> On Fri, May 15, 2020 at 12:33:42PM +0800, Xiaoming Ni wrote:
>>>> Add the shared variable SYSCTL_NEG_ONE to replace the variable neg_one
>>>> used in both sysctl_writes_strict and hung_task_warnings.
>>>>
>>>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>>>> ---
>>>>    fs/proc/proc_sysctl.c     | 2 +-
>>>>    include/linux/sysctl.h    | 1 +
>>>>    kernel/hung_task_sysctl.c | 3 +--
>>>>    kernel/sysctl.c           | 3 +--
>>>
>>> How about doing this refactoring in advance of the extraction patch?
>> Before  advance of the extraction patch, neg_one is only used in one file,
>> does it seem to have no value for refactoring?
> 
> I guess it doesn't matter much, but I think it's easier to review in the
> sense that neg_one is first extracted and then later everything else is
> moved.
> 
Later, when more features sysctl interface is moved to the code file, 
there will be more variables that need to be extracted.
So should I only extract the neg_one variable here, or should I extract 
all the variables used by multiple features?

  fs/proc/proc_sysctl.c  |  2 +-
  include/linux/sysctl.h | 11 ++++++++---
  kernel/sysctl.c        | 37 ++++++++++++++++---------------------
  3 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index b6f5d45..3f77e64 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -23,7 +23,7 @@
  static const struct inode_operations proc_sys_dir_operations;

  /* shared constants to be used in various sysctls */
-const int sysctl_vals[] = { 0, 1, INT_MAX };
+const int sysctl_vals[] = { -1, 0, 1, 2, 4, 100, 1000, INT_MAX };
  EXPORT_SYMBOL(sysctl_vals);

  /* Support for permanently empty directories */
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 43f8ef9..bf97c30 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -38,9 +38,14 @@
  struct ctl_dir;

  /* Keep the same order as in fs/proc/proc_sysctl.c */
-#define SYSCTL_ZERO	((void *)&sysctl_vals[0])
-#define SYSCTL_ONE	((void *)&sysctl_vals[1])
-#define SYSCTL_INT_MAX	((void *)&sysctl_vals[2])
+#define SYSCTL_NEG_ONE	((void *)&sysctl_vals[0])
+#define SYSCTL_ZERO	((void *)&sysctl_vals[1])
+#define SYSCTL_ONE	((void *)&sysctl_vals[2])
+#define SYSCTL_TWO	((void *)&sysctl_vals[3])
+#define SYSCTL_FOUR	((void *)&sysctl_vals[4])
+#define SYSCTL_ONE_HUNDRED	((void *)&sysctl_vals[5])
+#define SYSCTL_ONE_THOUSAND	((void *)&sysctl_vals[6])
+#define SYSCTL_INT_MAX	((void *)&sysctl_vals[7])

  extern const int sysctl_vals[];

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 5dd6d01..efe6172 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -118,14 +118,9 @@

  /* Constants used for minimum and  maximum */

-static int __maybe_unused neg_one = -1;
-static int __maybe_unused two = 2;
-static int __maybe_unused four = 4;
  static unsigned long zero_ul;
  static unsigned long one_ul = 1;
  static unsigned long long_max = LONG_MAX;
-static int one_hundred = 100;
-static int one_thousand = 1000;
  #ifdef CONFIG_PRINTK
  static int ten_thousand = 10000;
  #endif
@@ -534,7 +529,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.maxlen		= sizeof(int),
  		.mode		= 0644,
  		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= &neg_one,
+		.extra1		= SYSCTL_NEG_ONE,
  		.extra2		= SYSCTL_ONE,
  	},
  #endif
@@ -865,7 +860,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= proc_dointvec_minmax_sysadmin,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &two,
+		.extra2		= SYSCTL_TWO,
  	},
  #endif
  	{
@@ -1043,7 +1038,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= perf_cpu_time_max_percent_handler,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_hundred,
+		.extra2		= SYSCTL_ONE_HUNDRED,
  	},
  	{
  		.procname	= "perf_event_max_stack",
@@ -1061,7 +1056,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= perf_event_max_stack_handler,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_thousand,
+		.extra2		= SYSCTL_ONE_THOUSAND,
  	},
  #endif
  	{
@@ -1136,7 +1131,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= proc_dointvec_minmax,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &two,
+		.extra2		= SYSCTL_TWO,
  	},
  	{
  		.procname	= "panic_on_oom",
@@ -1145,7 +1140,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= proc_dointvec_minmax,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &two,
+		.extra2		= SYSCTL_TWO,
  	},
  	{
  		.procname	= "oom_kill_allocating_task",
@@ -1190,7 +1185,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= dirty_background_ratio_handler,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_hundred,
+		.extra2		= SYSCTL_ONE_HUNDRED,
  	},
  	{
  		.procname	= "dirty_background_bytes",
@@ -1207,7 +1202,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= dirty_ratio_handler,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_hundred,
+		.extra2		= SYSCTL_ONE_HUNDRED,
  	},
  	{
  		.procname	= "dirty_bytes",
@@ -1247,7 +1242,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= proc_dointvec_minmax,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_hundred,
+		.extra2		= SYSCTL_ONE_HUNDRED,
  	},
  #ifdef CONFIG_HUGETLB_PAGE
  	{
@@ -1304,7 +1299,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0200,
  		.proc_handler	= drop_caches_sysctl_handler,
  		.extra1		= SYSCTL_ONE,
-		.extra2		= &four,
+		.extra2		= SYSCTL_FOUR,
  	},
  #ifdef CONFIG_COMPACTION
  	{
@@ -1357,7 +1352,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= watermark_scale_factor_sysctl_handler,
  		.extra1		= SYSCTL_ONE,
-		.extra2		= &one_thousand,
+		.extra2		= SYSCTL_ONE_THOUSAND,
  	},
  	{
  		.procname	= "percpu_pagelist_fraction",
@@ -1436,7 +1431,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= sysctl_min_unmapped_ratio_sysctl_handler,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_hundred,
+		.extra2		= SYSCTL_ONE_HUNDRED,
  	},
  	{
  		.procname	= "min_slab_ratio",
@@ -1445,7 +1440,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= sysctl_min_slab_ratio_sysctl_handler,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &one_hundred,
+		.extra2		= SYSCTL_ONE_HUNDRED,
  	},
  #endif
  #ifdef CONFIG_SMP
@@ -1728,7 +1723,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0600,
  		.proc_handler	= proc_dointvec_minmax,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &two,
+		.extra2		= SYSCTL_TWO,
  	},
  	{
  		.procname	= "protected_regular",
@@ -1737,7 +1732,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0600,
  		.proc_handler	= proc_dointvec_minmax,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &two,
+		.extra2		= SYSCTL_TWO,
  	},
  	{
  		.procname	= "suid_dumpable",
@@ -1746,7 +1741,7 @@ static int sysrq_sysctl_handler(struct ctl_table 
*table, int write,
  		.mode		= 0644,
  		.proc_handler	= proc_dointvec_minmax_coredump,
  		.extra1		= SYSCTL_ZERO,
-		.extra2		= &two,
+		.extra2		= SYSCTL_TWO,
  	},
  #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
  	{

Re: [PATCH 2/4] proc/sysctl: add shared variables -1

[Kees Cook]

On Sat, May 16, 2020 at 10:32:19AM +0800, Xiaoming Ni wrote:
> On 2020/5/16 0:05, Kees Cook wrote:
> > On Fri, May 15, 2020 at 05:06:28PM +0800, Xiaoming Ni wrote:
> > > On 2020/5/15 16:06, Kees Cook wrote:
> > > > On Fri, May 15, 2020 at 12:33:42PM +0800, Xiaoming Ni wrote:
> > > > > Add the shared variable SYSCTL_NEG_ONE to replace the variable neg_one
> > > > > used in both sysctl_writes_strict and hung_task_warnings.
> > > > > 
> > > > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> > > > > ---
> > > > >    fs/proc/proc_sysctl.c     | 2 +-
> > > > >    include/linux/sysctl.h    | 1 +
> > > > >    kernel/hung_task_sysctl.c | 3 +--
> > > > >    kernel/sysctl.c           | 3 +--
> > > > 
> > > > How about doing this refactoring in advance of the extraction patch?
> > > Before  advance of the extraction patch, neg_one is only used in one file,
> > > does it seem to have no value for refactoring?
> > 
> > I guess it doesn't matter much, but I think it's easier to review in the
> > sense that neg_one is first extracted and then later everything else is
> > moved.
> > 
> Later, when more features sysctl interface is moved to the code file, there
> will be more variables that need to be extracted.
> So should I only extract the neg_one variable here, or should I extract all
> the variables used by multiple features?

Hmm -- if you're going to do a consolidation pass, then nevermind, I
don't think order will matter then.

Thank you for the cleanup! Sorry we're giving you back-and-forth advice!

-Kees

-- 
Kees Cook

Re: [PATCH 2/4] proc/sysctl: add shared variables -1

[Xiaoming Ni]

On 2020/5/16 10:47, Kees Cook wrote:
> On Sat, May 16, 2020 at 10:32:19AM +0800, Xiaoming Ni wrote:
>> On 2020/5/16 0:05, Kees Cook wrote:
>>> On Fri, May 15, 2020 at 05:06:28PM +0800, Xiaoming Ni wrote:
>>>> On 2020/5/15 16:06, Kees Cook wrote:
>>>>> On Fri, May 15, 2020 at 12:33:42PM +0800, Xiaoming Ni wrote:
>>>>>> Add the shared variable SYSCTL_NEG_ONE to replace the variable neg_one
>>>>>> used in both sysctl_writes_strict and hung_task_warnings.
>>>>>>
>>>>>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>>>>>> ---
>>>>>>     fs/proc/proc_sysctl.c     | 2 +-
>>>>>>     include/linux/sysctl.h    | 1 +
>>>>>>     kernel/hung_task_sysctl.c | 3 +--
>>>>>>     kernel/sysctl.c           | 3 +--
>>>>>
>>>>> How about doing this refactoring in advance of the extraction patch?
>>>> Before  advance of the extraction patch, neg_one is only used in one file,
>>>> does it seem to have no value for refactoring?
>>>
>>> I guess it doesn't matter much, but I think it's easier to review in the
>>> sense that neg_one is first extracted and then later everything else is
>>> moved.
>>>
>> Later, when more features sysctl interface is moved to the code file, there
>> will be more variables that need to be extracted.
>> So should I only extract the neg_one variable here, or should I extract all
>> the variables used by multiple features?
> 
> Hmm -- if you're going to do a consolidation pass, then nevermind, I
> don't think order will matter then.
> 
> Thank you for the cleanup! Sorry we're giving you back-and-forth advice!
> 
> -Kees
> 

Sorry, I don't fully understand.
Does this mean that there is no need to adjust the patch order or the 
order of variables in sysctl_vals?
Should I extract only SYSCTL_NEG_ONE or should I extract all variables?

Thanks
Xiaoming Ni

Re: [PATCH 2/4] proc/sysctl: add shared variables -1

[Kees Cook]

On Sat, May 16, 2020 at 11:05:53AM +0800, Xiaoming Ni wrote:
> On 2020/5/16 10:47, Kees Cook wrote:
> > On Sat, May 16, 2020 at 10:32:19AM +0800, Xiaoming Ni wrote:
> > > On 2020/5/16 0:05, Kees Cook wrote:
> > > > On Fri, May 15, 2020 at 05:06:28PM +0800, Xiaoming Ni wrote:
> > > > > On 2020/5/15 16:06, Kees Cook wrote:
> > > > > > On Fri, May 15, 2020 at 12:33:42PM +0800, Xiaoming Ni wrote:
> > > > > > > Add the shared variable SYSCTL_NEG_ONE to replace the variable neg_one
> > > > > > > used in both sysctl_writes_strict and hung_task_warnings.
> > > > > > > 
> > > > > > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> > > > > > > ---
> > > > > > >     fs/proc/proc_sysctl.c     | 2 +-
> > > > > > >     include/linux/sysctl.h    | 1 +
> > > > > > >     kernel/hung_task_sysctl.c | 3 +--
> > > > > > >     kernel/sysctl.c           | 3 +--
> > > > > > 
> > > > > > How about doing this refactoring in advance of the extraction patch?
> > > > > Before  advance of the extraction patch, neg_one is only used in one file,
> > > > > does it seem to have no value for refactoring?
> > > > 
> > > > I guess it doesn't matter much, but I think it's easier to review in the
> > > > sense that neg_one is first extracted and then later everything else is
> > > > moved.
> > > > 
> > > Later, when more features sysctl interface is moved to the code file, there
> > > will be more variables that need to be extracted.
> > > So should I only extract the neg_one variable here, or should I extract all
> > > the variables used by multiple features?
> > 
> > Hmm -- if you're going to do a consolidation pass, then nevermind, I
> > don't think order will matter then.
> > 
> > Thank you for the cleanup! Sorry we're giving you back-and-forth advice!
> > 
> > -Kees
> > 
> 
> Sorry, I don't fully understand.
> Does this mean that there is no need to adjust the patch order or the order
> of variables in sysctl_vals?
> Should I extract only SYSCTL_NEG_ONE or should I extract all variables?

I think either order is fine -- I though you were only doing 1 variable.
If you're don't a bunch, then I don't think order is important.

-- 
Kees Cook

[PATCH 3/4] watchdog: move watchdog sysctl to watchdog.c

[Xiaoming Ni]

Move watchdog sysctl interface to watchdog.c.
Use register_sysctl() to register the sysctl interface to avoid
merge conflicts when different features modify sysctl.c at the same time.

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
 kernel/sysctl.c   |  96 --------------------------------------------
 kernel/watchdog.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+), 96 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 01fc559..e394990 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -97,9 +97,6 @@
 #ifdef CONFIG_STACKLEAK_RUNTIME_DISABLE
 #include <linux/stackleak.h>
 #endif
-#ifdef CONFIG_LOCKUP_DETECTOR
-#include <linux/nmi.h>
-#endif
 
 #if defined(CONFIG_SYSCTL)
 
@@ -120,9 +117,6 @@
 #endif
 
 /* Constants used for minimum and  maximum */
-#ifdef CONFIG_LOCKUP_DETECTOR
-static int sixty = 60;
-#endif
 
 static int __maybe_unused two = 2;
 static int __maybe_unused four = 4;
@@ -887,96 +881,6 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
 		.mode		= 0444,
 		.proc_handler	= proc_dointvec,
 	},
-#if defined(CONFIG_LOCKUP_DETECTOR)
-	{
-		.procname       = "watchdog",
-		.data		= &watchdog_user_enabled,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler   = proc_watchdog,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_ONE,
-	},
-	{
-		.procname	= "watchdog_thresh",
-		.data		= &watchdog_thresh,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_watchdog_thresh,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= &sixty,
-	},
-	{
-		.procname       = "nmi_watchdog",
-		.data		= &nmi_watchdog_user_enabled,
-		.maxlen		= sizeof(int),
-		.mode		= NMI_WATCHDOG_SYSCTL_PERM,
-		.proc_handler   = proc_nmi_watchdog,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_ONE,
-	},
-	{
-		.procname	= "watchdog_cpumask",
-		.data		= &watchdog_cpumask_bits,
-		.maxlen		= NR_CPUS,
-		.mode		= 0644,
-		.proc_handler	= proc_watchdog_cpumask,
-	},
-#ifdef CONFIG_SOFTLOCKUP_DETECTOR
-	{
-		.procname       = "soft_watchdog",
-		.data		= &soft_watchdog_user_enabled,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler   = proc_soft_watchdog,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_ONE,
-	},
-	{
-		.procname	= "softlockup_panic",
-		.data		= &softlockup_panic,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_ONE,
-	},
-#ifdef CONFIG_SMP
-	{
-		.procname	= "softlockup_all_cpu_backtrace",
-		.data		= &sysctl_softlockup_all_cpu_backtrace,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_ONE,
-	},
-#endif /* CONFIG_SMP */
-#endif
-#ifdef CONFIG_HARDLOCKUP_DETECTOR
-	{
-		.procname	= "hardlockup_panic",
-		.data		= &hardlockup_panic,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_ONE,
-	},
-#ifdef CONFIG_SMP
-	{
-		.procname	= "hardlockup_all_cpu_backtrace",
-		.data		= &sysctl_hardlockup_all_cpu_backtrace,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
-		.extra1		= SYSCTL_ZERO,
-		.extra2		= SYSCTL_ONE,
-	},
-#endif /* CONFIG_SMP */
-#endif
-#endif
-
 #if defined(CONFIG_X86_LOCAL_APIC) && defined(CONFIG_X86)
 	{
 		.procname       = "unknown_nmi_panic",
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index b6b1f54..05e1d58 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -23,6 +23,9 @@
 #include <linux/sched/debug.h>
 #include <linux/sched/isolation.h>
 #include <linux/stop_machine.h>
+#ifdef CONFIG_SYSCTL
+#include <linux/kmemleak.h>
+#endif
 
 #include <asm/irq_regs.h>
 #include <linux/kvm_para.h>
@@ -756,10 +759,124 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 	mutex_unlock(&watchdog_mutex);
 	return err;
 }
+
+static int sixty = 60;
+
+static struct ctl_table watchdog_sysctls[] = {
+	{
+		.procname       = "watchdog",
+		.data		= &watchdog_user_enabled,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler   = proc_watchdog,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+	{
+		.procname	= "watchdog_thresh",
+		.data		= &watchdog_thresh,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_watchdog_thresh,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= &sixty,
+	},
+	{
+		.procname       = "nmi_watchdog",
+		.data		= &nmi_watchdog_user_enabled,
+		.maxlen		= sizeof(int),
+		.mode		= NMI_WATCHDOG_SYSCTL_PERM,
+		.proc_handler   = proc_nmi_watchdog,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+	{
+		.procname	= "watchdog_cpumask",
+		.data		= &watchdog_cpumask_bits,
+		.maxlen		= NR_CPUS,
+		.mode		= 0644,
+		.proc_handler	= proc_watchdog_cpumask,
+	},
+#ifdef CONFIG_SOFTLOCKUP_DETECTOR
+	{
+		.procname       = "soft_watchdog",
+		.data		= &soft_watchdog_user_enabled,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler   = proc_soft_watchdog,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+	{
+		.procname	= "softlockup_panic",
+		.data		= &softlockup_panic,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+#ifdef CONFIG_SMP
+	{
+		.procname	= "softlockup_all_cpu_backtrace",
+		.data		= &sysctl_softlockup_all_cpu_backtrace,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+#endif /* CONFIG_SMP */
+#endif
+#ifdef CONFIG_HARDLOCKUP_DETECTOR
+	{
+		.procname	= "hardlockup_panic",
+		.data		= &hardlockup_panic,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+#ifdef CONFIG_SMP
+	{
+		.procname	= "hardlockup_all_cpu_backtrace",
+		.data		= &sysctl_hardlockup_all_cpu_backtrace,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_ONE,
+	},
+#endif /* CONFIG_SMP */
+#endif
+	{}
+};
+
+/*
+ * The watchdog sysctl has a default value.
+ * Even if register_sysctl() fails, it does not affect the main function of
+ * the watchdog. At the same time, during the system initialization phase,
+ * malloc small memory will almost never fail.
+ * So the return value is ignored here
+ */
+static void __init watchdog_sysctl_init(void)
+{
+	struct ctl_table_header *p = register_sysctl("kernel", watchdog_sysctls);
+
+	if (unlikely(!p)) {
+		pr_err("%s fail\n", __func__);
+		return;
+	}
+	kmemleak_not_leak(p);
+}
+#else
+#define watchdog_sysctl_init() do { } while (0)
 #endif /* CONFIG_SYSCTL */
 
 void __init lockup_detector_init(void)
 {
+	watchdog_sysctl_init();
 	if (tick_nohz_full_enabled())
 		pr_info("Disabling watchdog on nohz_full cores by default\n");
 
-- 
1.8.5.6

Re: [PATCH 3/4] watchdog: move watchdog sysctl to watchdog.c

[Kees Cook]

On Fri, May 15, 2020 at 12:33:43PM +0800, Xiaoming Ni wrote:
> +static int sixty = 60;

This should be const. (Which will require a cast during extra2
assignment.)

-- 
Kees Cook

Re: [PATCH 3/4] watchdog: move watchdog sysctl to watchdog.c

[Xiaoming Ni]

On 2020/5/15 16:09, Kees Cook wrote:
> On Fri, May 15, 2020 at 12:33:43PM +0800, Xiaoming Ni wrote:
>> +static int sixty = 60;
> 
> This should be const. (Which will require a cast during extra2
> assignment.)
> 
Sorry, I forgot to append const.
Thanks for your guidance.

Thanks
Xiaoming Ni

[PATCH 4/4] sysctl: Add register_sysctl_init() interface

[Xiaoming Ni]

In order to eliminate the duplicate code for registering the sysctl
interface during the initialization of each feature, add the
register_sysctl_init() interface

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
 include/linux/sysctl.h    |  2 ++
 kernel/hung_task_sysctl.c | 15 +--------------
 kernel/sysctl.c           | 19 +++++++++++++++++++
 kernel/watchdog.c         | 18 +-----------------
 4 files changed, 23 insertions(+), 31 deletions(-)

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 6d741d6..3cdbe89 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -207,6 +207,8 @@ struct ctl_table_header *register_sysctl_paths(const struct ctl_path *path,
 void unregister_sysctl_table(struct ctl_table_header * table);
 
 extern int sysctl_init(void);
+extern void register_sysctl_init(const char *path, struct ctl_table *table,
+				 const char *table_name);
 
 extern struct ctl_table sysctl_mount_point[];
 
diff --git a/kernel/hung_task_sysctl.c b/kernel/hung_task_sysctl.c
index 62a51f5..14d2ed6 100644
--- a/kernel/hung_task_sysctl.c
+++ b/kernel/hung_task_sysctl.c
@@ -59,21 +59,8 @@
 	{}
 };
 
-/*
- * The hung task sysctl has a default value.
- * Even if register_sysctl() fails, it does not affect the main function of
- * the hung task. At the same time, during the system initialization phase,
- * malloc small memory will almost never fail.
- * So the return value is ignored here
- */
 void __init hung_task_sysctl_init(void)
 {
-	struct ctl_table_header *srt = register_sysctl("kernel", hung_task_sysctls);
-
-	if (unlikely(!srt)) {
-		pr_err("%s fail\n", __func__);
-		return;
-	}
-	kmemleak_not_leak(srt);
+	register_sysctl_init("kernel", hung_task_sysctls, "hung_task_sysctls");
 }
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index e394990..0a09742 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1823,6 +1823,25 @@ int __init sysctl_init(void)
 	return 0;
 }
 
+/*
+ * The sysctl interface is used to modify the interface value,
+ * but the feature interface has default values. Even if register_sysctl fails,
+ * the feature body function can also run. At the same time, malloc small
+ * fragment of memory during the system initialization phase, almost does
+ * not fail. Therefore, the function return is designed as void
+ */
+void __init register_sysctl_init(const char *path, struct ctl_table *table,
+				 const char *table_name)
+{
+	struct ctl_table_header *hdr = register_sysctl(path, table);
+
+	if (unlikely(!hdr)) {
+		pr_err("failed when register_sysctl %s to %s\n", table_name, path);
+		return;
+	}
+	kmemleak_not_leak(hdr);
+}
+
 #endif /* CONFIG_SYSCTL */
 
 /*
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 05e1d58..c1bebb1 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -23,9 +23,6 @@
 #include <linux/sched/debug.h>
 #include <linux/sched/isolation.h>
 #include <linux/stop_machine.h>
-#ifdef CONFIG_SYSCTL
-#include <linux/kmemleak.h>
-#endif
 
 #include <asm/irq_regs.h>
 #include <linux/kvm_para.h>
@@ -853,22 +850,9 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 	{}
 };
 
-/*
- * The watchdog sysctl has a default value.
- * Even if register_sysctl() fails, it does not affect the main function of
- * the watchdog. At the same time, during the system initialization phase,
- * malloc small memory will almost never fail.
- * So the return value is ignored here
- */
 static void __init watchdog_sysctl_init(void)
 {
-	struct ctl_table_header *p = register_sysctl("kernel", watchdog_sysctls);
-
-	if (unlikely(!p)) {
-		pr_err("%s fail\n", __func__);
-		return;
-	}
-	kmemleak_not_leak(p);
+	register_sysctl_init("kernel", watchdog_sysctls, "watchdog_sysctls");
 }
 #else
 #define watchdog_sysctl_init() do { } while (0)
-- 
1.8.5.6

Re: [PATCH 4/4] sysctl: Add register_sysctl_init() interface

[Kees Cook]

On Fri, May 15, 2020 at 12:33:44PM +0800, Xiaoming Ni wrote:
> In order to eliminate the duplicate code for registering the sysctl
> interface during the initialization of each feature, add the
> register_sysctl_init() interface

I think this should come before the code relocations.

-- 
Kees Cook

Re: [PATCH 4/4] sysctl: Add register_sysctl_init() interface

[Xiaoming Ni]

On 2020/5/15 16:10, Kees Cook wrote:
> On Fri, May 15, 2020 at 12:33:44PM +0800, Xiaoming Ni wrote:
>> In order to eliminate the duplicate code for registering the sysctl
>> interface during the initialization of each feature, add the
>> register_sysctl_init() interface
> 
> I think this should come before the code relocations.
> 
Thanks for your guidance, I will adjust it in v2 version.

Thanks
Xiaoming Ni