[PATCH v2 0/4] debugfs: Add simple min/max "files" to debugfs to fix sched debug code.

[PATCH v2 0/4] debugfs: Add simple min/max "files" to debugfs to fix sched debug code.

[chris hyser]

v2:
Apologies. I sent this the first time without including lkml.

v1:
This originally started as an attempt to solve a divide by zero issue in sched
debug code that was introduced when a sysctl value with non-zero checking was
moved to a simple u32 debugfs file. In looking at ways to solve this, it was
mentioned I should look at providing general debugfs files with min/max
checking. 

One problem was that a check for greater than zero for say a u8 succeeds for a
number like 256 (but stores a zero anyway) as the upper bits that don't fit into
storage are silently dropped. Therefore values greater than the storage capacity
must also fail. Getting an error when what the user wrote is not what was
actually stored, seems like a useful requirement for the other simple files and
so I moved the check into there.

To enable easy testing, a test module and test script are provided which can
validate the new functions as well as check the new limits on the older
functions. This was stuck under selftests, but it is not currently tied into the
testing infrastructure.

 Documentation/filesystems/debugfs.rst          |  47 +++++++++++++++--
 fs/debugfs/file.c                              | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/debugfs.h                        |  60 +++++++++++++++++++++
 kernel/sched/debug.c                           |   9 +++-
 tools/testing/selftests/debugfs/Makefile       |  15 ++++++
 tools/testing/selftests/debugfs/minmax_test.c  | 140 +++++++++++++++++++++++++++++++++++++++++++++++++
 tools/testing/selftests/debugfs/test_minmax.sh | 147 +++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 609 insertions(+), 4 deletions(-)

[PATCH 1/4] debugfs: return EINVAL if write to unsigned simple files exceed storage.

[chris hyser]

Writes to the debug files created by "debugfs_create_*" (u8/u16/u32/u64),
(x8/x16/x32/x64) should not silently succeed if the value exceeds the
storage space for the type and upper written bits are lost. Absent an
error, a read should return the last written value.  Current behaviour is
to down cast the storage type thus losing upper bits (thus u64/x64
files are unaffected).

This patch ensures the written value fits into the specified storage space
returning EINVAL on error.

Signed-off-by: Chris Hyser <chris.hyser@oracle.com>
---
 Documentation/filesystems/debugfs.rst | 7 ++++---
 fs/debugfs/file.c                     | 6 ++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/Documentation/filesystems/debugfs.rst b/Documentation/filesystems/debugfs.rst
index dc35da8b8792..6f1ac8d7f108 100644
--- a/Documentation/filesystems/debugfs.rst
+++ b/Documentation/filesystems/debugfs.rst
@@ -85,9 +85,10 @@ created with any of::
 			    struct dentry *parent, u64 *value);
 
 These files support both reading and writing the given value; if a specific
-file should not be written to, simply set the mode bits accordingly.  The
-values in these files are in decimal; if hexadecimal is more appropriate,
-the following functions can be used instead::
+file should not be written to, simply set the mode bits accordingly.  Written
+values that exceed the storage for the type return EINVAL. The values in these
+files are in decimal; if hexadecimal is more appropriate, the following
+functions can be used instead::
 
     void debugfs_create_x8(const char *name, umode_t mode,
 			   struct dentry *parent, u8 *value);
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 1f971c880dde..743ddd04f8d8 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -429,6 +429,8 @@ static struct dentry *debugfs_create_mode_unsafe(const char *name, umode_t mode,
 
 static int debugfs_u8_set(void *data, u64 val)
 {
+	if (val > (1 << sizeof(u8) * 8) - 1)
+		return -EINVAL;
 	*(u8 *)data = val;
 	return 0;
 }
@@ -465,6 +467,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_u8);
 
 static int debugfs_u16_set(void *data, u64 val)
 {
+	if (val > (1 << sizeof(u16) * 8) - 1)
+		return -EINVAL;
 	*(u16 *)data = val;
 	return 0;
 }
@@ -501,6 +505,8 @@ EXPORT_SYMBOL_GPL(debugfs_create_u16);
 
 static int debugfs_u32_set(void *data, u64 val)
 {
+	if (val > (1ull << sizeof(u32) * 8) - 1)
+		return -EINVAL;
 	*(u32 *)data = val;
 	return 0;
 }
-- 
2.31.1

Re: [PATCH 1/4] debugfs: return EINVAL if write to unsigned simple files exceed storage.

[Greg KH]

On Tue, May 30, 2023 at 03:40:09PM -0400, chris hyser wrote:
> Writes to the debug files created by "debugfs_create_*" (u8/u16/u32/u64),
> (x8/x16/x32/x64) should not silently succeed if the value exceeds the
> storage space for the type and upper written bits are lost. Absent an
> error, a read should return the last written value.  Current behaviour is
> to down cast the storage type thus losing upper bits (thus u64/x64
> files are unaffected).
> 
> This patch ensures the written value fits into the specified storage space
> returning EINVAL on error.
> 
> Signed-off-by: Chris Hyser <chris.hyser@oracle.com>
> ---
>  Documentation/filesystems/debugfs.rst | 7 ++++---
>  fs/debugfs/file.c                     | 6 ++++++
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/filesystems/debugfs.rst b/Documentation/filesystems/debugfs.rst
> index dc35da8b8792..6f1ac8d7f108 100644
> --- a/Documentation/filesystems/debugfs.rst
> +++ b/Documentation/filesystems/debugfs.rst
> @@ -85,9 +85,10 @@ created with any of::
>  			    struct dentry *parent, u64 *value);
>  
>  These files support both reading and writing the given value; if a specific
> -file should not be written to, simply set the mode bits accordingly.  The
> -values in these files are in decimal; if hexadecimal is more appropriate,
> -the following functions can be used instead::
> +file should not be written to, simply set the mode bits accordingly.  Written
> +values that exceed the storage for the type return EINVAL. The values in these
> +files are in decimal; if hexadecimal is more appropriate, the following
> +functions can be used instead::
>  
>      void debugfs_create_x8(const char *name, umode_t mode,
>  			   struct dentry *parent, u8 *value);
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 1f971c880dde..743ddd04f8d8 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -429,6 +429,8 @@ static struct dentry *debugfs_create_mode_unsafe(const char *name, umode_t mode,
>  
>  static int debugfs_u8_set(void *data, u64 val)
>  {
> +	if (val > (1 << sizeof(u8) * 8) - 1)
> +		return -EINVAL;

We do have U8_MAX and friends, please don't reinvent the wheel.

But really, why?  This is debugfs, if userspace messes something up like
this, why not just cast and be done with it?

In other words, what existing workflow is now going to break with this
patchset?  :)

thanks,

greg k-h

Re: [PATCH 1/4] debugfs: return EINVAL if write to unsigned simple files exceed storage.

[chris hyser]

On 5/30/23 16:14, Greg KH wrote:
> On Tue, May 30, 2023 at 03:40:09PM -0400, chris hyser wrote:
>> Writes to the debug files created by "debugfs_create_*" (u8/u16/u32/u64),
>> (x8/x16/x32/x64) should not silently succeed if the value exceeds the
>> storage space for the type and upper written bits are lost. Absent an
>> error, a read should return the last written value.  Current behaviour is
>> to down cast the storage type thus losing upper bits (thus u64/x64
>> files are unaffected).
>>
>> This patch ensures the written value fits into the specified storage space
>> returning EINVAL on error.
>>
>> Signed-off-by: Chris Hyser <chris.hyser@oracle.com>
>> ---
>>   Documentation/filesystems/debugfs.rst | 7 ++++---
>>   fs/debugfs/file.c                     | 6 ++++++
>>   2 files changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/filesystems/debugfs.rst b/Documentation/filesystems/debugfs.rst
>> index dc35da8b8792..6f1ac8d7f108 100644
>> --- a/Documentation/filesystems/debugfs.rst
>> +++ b/Documentation/filesystems/debugfs.rst
>> @@ -85,9 +85,10 @@ created with any of::
>>   			    struct dentry *parent, u64 *value);
>>   
>>   These files support both reading and writing the given value; if a specific
>> -file should not be written to, simply set the mode bits accordingly.  The
>> -values in these files are in decimal; if hexadecimal is more appropriate,
>> -the following functions can be used instead::
>> +file should not be written to, simply set the mode bits accordingly.  Written
>> +values that exceed the storage for the type return EINVAL. The values in these
>> +files are in decimal; if hexadecimal is more appropriate, the following
>> +functions can be used instead::
>>   
>>       void debugfs_create_x8(const char *name, umode_t mode,
>>   			   struct dentry *parent, u8 *value);
>> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
>> index 1f971c880dde..743ddd04f8d8 100644
>> --- a/fs/debugfs/file.c
>> +++ b/fs/debugfs/file.c
>> @@ -429,6 +429,8 @@ static struct dentry *debugfs_create_mode_unsafe(const char *name, umode_t mode,
>>   
>>   static int debugfs_u8_set(void *data, u64 val)
>>   {
>> +	if (val > (1 << sizeof(u8) * 8) - 1)
>> +		return -EINVAL;
> 
> We do have U8_MAX and friends, please don't reinvent the whee >
> But really, why?  This is debugfs, if userspace messes something up like
> this, why not just cast and be done with it?

So PeterZ asked much the same question when I submitted something for 
the divide by zero bug mentioned. Just don't write 0. Ultimately, he 
recommended looking into the more generic min/max stuff.

Now, if it was only "really" debug I think even then catching an easy to 
catch error is useful. Unfortunately, many distros ship with this stuff 
turned on and tools access these things ... which is the answer to your 
next question, what might this break. I'm not sure something (likely 
accidentally) relying on silently dropping bits really works, but it 
would appear to break with this change.

> 
> In other words, what existing workflow is now going to break with this
> patchset?  :)

I would answer this with your point that this is debugfs. I remember 
when a bunch of the sched debug sysctls were originally moved to debugfs 
files. That broke tools etc, but the argument was that it is debugfs. 
You should not use it in production and the API is what it is. Not an 
exact analogy for sure, but that was my thinking.

Honestly, I wasn't really sure if this patch would fly, but simple 
checks and feedback even in debugging seemed useful to me.

Are you open to the min/max files patch? I can resend w/o basing on this 
patch.

-chrish

[PATCH 2/4] debugfs: Provide min/max checking for simple u8, u16, u32, u64 debugfs files.

[chris hyser]

This patch extends the simple file interface to include min/max checking
files.  Writes to a file are checked such that the written value must
satisfy the specified check (as well as fit into the specified storage):

minmax: 'min' <= value <= 'max'
min:    'min' <= value
max:    value <= 'max'

Failure of the check returns EINVAL.

While the same checks could be done by providing a custom "set(void *data,
u64 val) function" in DEFINE_DEBUGFS_ATTRIBUTE() for each file needing said
check, each instance would require a private struct file_operations.

Using the same trick as the unsigned simple files (u8/u16/u32/u64), this
patch supports "unlimited" users with only two struct file_operations per
unsigned type. As min/max checking only applies to set/writing, read-only
files make no sense.

Various macros are provided to simplify setting up the params struct.

Signed-off-by: Chris Hyser <chris.hyser@oracle.com>
---
 Documentation/filesystems/debugfs.rst |  39 ++++++
 fs/debugfs/file.c                     | 189 ++++++++++++++++++++++++++
 include/linux/debugfs.h               |  60 ++++++++
 3 files changed, 288 insertions(+)

diff --git a/Documentation/filesystems/debugfs.rst b/Documentation/filesystems/debugfs.rst
index 6f1ac8d7f108..31d186e952eb 100644
--- a/Documentation/filesystems/debugfs.rst
+++ b/Documentation/filesystems/debugfs.rst
@@ -99,6 +99,45 @@ functions can be used instead::
     void debugfs_create_x64(const char *name, umode_t mode,
 			    struct dentry *parent, u64 *value);
 
+Some use cases require min/max checking of the written values preserving the
+initial value on failure and returning EINVAL for the write.
+
+    void debugfs_create_u8_minmax(const char *name, umode_t mode,
+			       struct dentry *parent,
+			       struct debugfs_minmax_params *mnxp);
+    void debugfs_create_u16_minmax(const char *name, umode_t mode,
+			       struct dentry *parent,
+			       struct debugfs_minmax_params *mnxp);
+    void debugfs_create_u32_minmax(const char *name, umode_t mode,
+			       struct dentry *parent,
+			       struct debugfs_minmax_params *mnxp);
+    void debugfs_create_u64_minmax(const char *name, umode_t mode,
+			       struct dentry *parent,
+			       struct debugfs_minmax_params *mnxp);
+
+These functions are called with the following parameter structure, flags and
+helper macros. The parameter structure must be available for the duration of
+the file, thus requiring global or malloced memory. Failure of the specified
+check(s) return EINVAL.
+
+struct debugfs_minmax_params {
+	void *value;
+	u64 min;
+	u64 max;
+	u8 flags;
+};
+
+Flags are defined as such:
+    DEBUGFS_ATTR_MIN 1
+    DEBUGFS_ATTR_MAX 2
+    DEBUGFS_ATTR_MINMAX (DEBUGFS_ATTR_MIN | DEBUGFS_ATTR_MAX)
+
+Additional helper macros provide for
+    DEBUGFS_MINMAX_ATTRIBUTES_BASE(name_parm_ptr, value, min, max, flags);
+    DEBUGFS_MIN_ATTRIBUTES(name_parm_ptr, value, min);
+    DEBUGFS_MAX_ATTRIBUTES(name_parm_ptr, value, max);
+    DEBUGFS_MINMAX_ATTRIBUTES(name_parm_ptr, value, min, max);
+
 These functions are useful as long as the developer knows the size of the
 value to be exported.  Some types can have different widths on different
 architectures, though, complicating the situation somewhat.  There are
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index 743ddd04f8d8..bd655b286da6 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -578,6 +578,195 @@ void debugfs_create_u64(const char *name, umode_t mode, struct dentry *parent,
 }
 EXPORT_SYMBOL_GPL(debugfs_create_u64);
 
+static int debugfs_minmax_chk(struct debugfs_minmax_params *mnxp, u64 val)
+{
+	if ((mnxp->flags & 0x1) && val < mnxp->min)
+		return -EINVAL;
+	if ((mnxp->flags & 0x2) && val > mnxp->max)
+		return -EINVAL;
+	return 0;
+}
+
+static int debugfs_u8_minmax_set(void *data, u64 val)
+{
+	struct debugfs_minmax_params *mnxp = data;
+	int err = debugfs_minmax_chk(mnxp, val);
+
+	if (err)
+		return err;
+	return debugfs_u8_set(mnxp->value, val);
+}
+
+static int debugfs_u8_minmax_get(void *data, u64 *val)
+{
+	struct debugfs_minmax_params *mnxp = data;
+
+	return debugfs_u16_get(mnxp->value, val);
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u8_minmax, debugfs_u8_minmax_get, debugfs_u8_minmax_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u8_minmax_wo, NULL, debugfs_u8_minmax_set, "%llu\n");
+
+/**
+ * debugfs_create_u8_minmax - create a debugfs file that is used to both read
+ *     and write an unsigned 8-bit value if it satisfies a min check, max check
+ *     or both.
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file.  This should be a
+ *          directory dentry if set.  If this parameter is %NULL, then the
+ *          file will be created in the root of the debugfs filesystem.
+ * @mnxp: a pointer to the parameter struct which holds the test limits, the
+ *        test to perform and a pointer to the variable to display and modify.
+ *
+ * This function creates a file in debugfs with the given name that contains
+ * the value of the specified variable.  If the @mode variable is so
+ * set, it can be read from, and written to.
+ */
+void debugfs_create_u8_minmax(const char *name, umode_t mode,
+			      struct dentry *parent,
+			      struct debugfs_minmax_params *mnxp)
+{
+	debugfs_create_mode_unsafe(name, mode, parent, mnxp, &fops_u8_minmax,
+				   NULL, &fops_u8_minmax_wo);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_u8_minmax);
+
+static int debugfs_u16_minmax_set(void *data, u64 val)
+{
+	struct debugfs_minmax_params *mnxp = data;
+	int err = debugfs_minmax_chk(mnxp, val);
+
+	if (err)
+		return err;
+	return debugfs_u16_set(mnxp->value, val);
+}
+
+static int debugfs_u16_minmax_get(void *data, u64 *val)
+{
+	struct debugfs_minmax_params *mnxp = data;
+
+	return debugfs_u16_get(mnxp->value, val);
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u16_minmax, debugfs_u16_minmax_get, debugfs_u16_minmax_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u16_minmax_wo, NULL, debugfs_u16_minmax_set, "%llu\n");
+
+/**
+ * debugfs_create_u16_minmax - create a debugfs file that is used to both read
+ *     and write an unsigned 16-bit value if it satisfies a min check, max check
+ *     or both.
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file.  This should be a
+ *          directory dentry if set.  If this parameter is %NULL, then the
+ *          file will be created in the root of the debugfs filesystem.
+ * @mnxp: a pointer to the parameter struct which holds the test limits, the
+ *        test to perform and a pointer to the variable to display and modify.
+ *
+ * This function creates a file in debugfs with the given name that contains
+ * the value of the specified variable.  If the @mode variable is so
+ * set, it can be read from, and written to.
+ */
+void debugfs_create_u16_minmax(const char *name, umode_t mode,
+			       struct dentry *parent,
+			       struct debugfs_minmax_params *mnxp)
+{
+	debugfs_create_mode_unsafe(name, mode, parent, mnxp, &fops_u16_minmax,
+				   NULL, &fops_u16_minmax_wo);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_u16_minmax);
+
+static int debugfs_u32_minmax_set(void *data, u64 val)
+{
+	struct debugfs_minmax_params *mnxp = data;
+	int err = debugfs_minmax_chk(mnxp, val);
+
+	if (err)
+		return err;
+	return debugfs_u32_set(mnxp->value, val);
+}
+
+static int debugfs_u32_minmax_get(void *data, u64 *val)
+{
+	struct debugfs_minmax_params *mnxp = data;
+
+	return debugfs_u32_get(mnxp->value, val);
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u32_minmax, debugfs_u32_minmax_get, debugfs_u32_minmax_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u32_minmax_wo, NULL, debugfs_u32_minmax_set, "%llu\n");
+
+/**
+ * debugfs_create_u32_minmax - create a debugfs file that is used to both read
+ *     and write an unsigned 32-bit value if it satisfies a min check, max check
+ *     or both.
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file.  This should be a
+ *          directory dentry if set.  If this parameter is %NULL, then the
+ *          file will be created in the root of the debugfs filesystem.
+ * @mnxp: a pointer to the parameter struct which holds the test limits, the
+ *        test to perform and a pointer to the variable to display and modify.
+ *
+ * This function creates a file in debugfs with the given name that contains
+ * the value of the specified variable.  If the @mode variable is so
+ * set, it can be read from, and written to.
+ */
+void debugfs_create_u32_minmax(const char *name, umode_t mode,
+			       struct dentry *parent,
+			       struct debugfs_minmax_params *mnxp)
+{
+	debugfs_create_mode_unsafe(name, mode, parent, mnxp, &fops_u32_minmax,
+				   NULL, &fops_u32_minmax_wo);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_u32_minmax);
+
+static int debugfs_u64_minmax_set(void *data, u64 val)
+{
+	struct debugfs_minmax_params *mnxp = data;
+	int err = debugfs_minmax_chk(mnxp, val);
+
+	if (err)
+		return err;
+	return debugfs_u64_set(mnxp->value, val);
+}
+
+static int debugfs_u64_minmax_get(void *data, u64 *val)
+{
+	struct debugfs_minmax_params *mnxp = data;
+
+	return debugfs_u64_get(mnxp->value, val);
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u64_minmax, debugfs_u64_minmax_get, debugfs_u64_minmax_set, "%llu\n");
+DEFINE_DEBUGFS_ATTRIBUTE(fops_u64_minmax_wo, NULL, debugfs_u64_minmax_set, "%llu\n");
+
+/**
+ * debugfs_create_u64_minmax - create a debugfs file that is used both to read
+ *     and write an unsigned 64-bit value if it satisfies a min check, max check
+ *     or both.
+ * @name: a pointer to a string containing the name of the file to create.
+ * @mode: the permission that the file should have
+ * @parent: a pointer to the parent dentry for this file.  This should be a
+ *          directory dentry if set.  If this parameter is %NULL, then the
+ *          file will be created in the root of the debugfs filesystem.
+ * @mnxp: a pointer to the parameter struct which holds the test limits, the
+ *        test to perform and a pointer to the variable to display and modify.
+ *
+ * This function creates a file in debugfs with the given name that contains
+ * the value of the specified variable.  If the @mode variable is so
+ * set, it can be read from, and written to.
+ */
+void debugfs_create_u64_minmax(const char *name, umode_t mode,
+			       struct dentry *parent,
+			       struct debugfs_minmax_params *mnxp)
+{
+	debugfs_create_mode_unsafe(name, mode, parent, mnxp, &fops_u64_minmax,
+				   NULL, &fops_u64_minmax_wo);
+}
+EXPORT_SYMBOL_GPL(debugfs_create_u64_minmax);
+
 static int debugfs_ulong_set(void *data, u64 val)
 {
 	*(unsigned long *)data = val;
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index ea2d919fd9c7..322fecb22a0a 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -43,6 +43,36 @@ struct debugfs_u32_array {
 	u32 n_elements;
 };
 
+struct debugfs_minmax_params {
+	void *value;
+	u64 min;
+	u64 max;
+	u8 flags;
+};
+
+/* debugfs_minmax_params "flag" values
+ */
+#define DEBUGFS_ATTR_MIN 1
+#define DEBUGFS_ATTR_MAX 2
+#define DEBUGFS_ATTR_MINMAX (DEBUGFS_ATTR_MIN | DEBUGFS_ATTR_MAX)
+
+#define DEBUGFS_MINMAX_ATTRIBUTES_BASE(__name, __value, __min, __max, __flags) \
+static struct debugfs_minmax_params __name = {				\
+	.value = (__value),						\
+	.min = (__min),							\
+	.max = (__max),							\
+	.flags = (__flags),						\
+}
+
+#define DEBUGFS_MIN_ATTRIBUTES(__name, __value, __min) \
+	DEBUGFS_MINMAX_ATTRIBUTES_BASE(__name, __value, __min, 0, DEBUGFS_ATTR_MIN)
+
+#define DEBUGFS_MAX_ATTRIBUTES(__name, __value, __max) \
+	DEBUGFS_MINMAX_ATTRIBUTES_BASE(__name, __value, 0, __max, DEBUGFS_ATTR_MAX)
+
+#define DEBUGFS_MINMAX_ATTRIBUTES(__name, __value, __min, __max) \
+	DEBUGFS_MINMAX_ATTRIBUTES_BASE(__name, __value, __min, __max, DEBUGFS_ATTR_MINMAX)
+
 extern struct dentry *arch_debugfs_dir;
 
 #define DEFINE_DEBUGFS_ATTRIBUTE_XSIGNED(__fops, __get, __set, __fmt, __is_signed)	\
@@ -122,6 +152,20 @@ void debugfs_create_u32(const char *name, umode_t mode, struct dentry *parent,
 			u32 *value);
 void debugfs_create_u64(const char *name, umode_t mode, struct dentry *parent,
 			u64 *value);
+
+void debugfs_create_u8_minmax(const char *name, umode_t mode,
+			      struct dentry *parent,
+			      struct debugfs_minmax_params *mnxp);
+void debugfs_create_u16_minmax(const char *name, umode_t mode,
+			       struct dentry *parent,
+			       struct debugfs_minmax_params *mnxp);
+void debugfs_create_u32_minmax(const char *name, umode_t mode,
+			       struct dentry *parent,
+			       struct debugfs_minmax_params *mnxp);
+void debugfs_create_u64_minmax(const char *name, umode_t mode,
+			       struct dentry *parent,
+			       struct debugfs_minmax_params *mnxp);
+
 void debugfs_create_ulong(const char *name, umode_t mode, struct dentry *parent,
 			  unsigned long *value);
 void debugfs_create_x8(const char *name, umode_t mode, struct dentry *parent,
@@ -287,6 +331,22 @@ static inline void debugfs_create_u32(const char *name, umode_t mode,
 static inline void debugfs_create_u64(const char *name, umode_t mode,
 				      struct dentry *parent, u64 *value) { }
 
+static inline void debugfs_create_u8_minmax(const char *name, umode_t mode,
+					    struct dentry *parent,
+					    struct debugfs_minmax_params *mnxp) { }
+
+static inline void debugfs_create_u16_minmax(const char *name, umode_t mode,
+					     struct dentry *parent,
+					     struct debugfs_minmax_params *mnxp) { }
+
+static inline void debugfs_create_u32_minmax(const char *name, umode_t mode,
+					     struct dentry *parent,
+					     struct debugfs_minmax_params *mnxp) { }
+
+static inline void debugfs_create_u64_minmax(const char *name, umode_t mode,
+					     struct dentry *parent,
+					     struct debugfs_minmax_params *mnxp) { }
+
 static inline void debugfs_create_ulong(const char *name, umode_t mode,
 					struct dentry *parent,
 					unsigned long *value) { }
-- 
2.31.1

[PATCH 3/4] debugfs: provide testing for the min/max simple debug files.

[chris hyser]

This patch contains a module and a bash script which tests the new simple
"unsigned min/max checking" files.

The module creates the debugfs files and encodes the test type and test
parameters in the file name which the script decodes and tests.

Currently the files were placed under testscripts though auto testing is
not yet enabled. The hope is that this eases testing of the patches.

Signed-off-by: Chris Hyser <chris.hyser@oracle.com>
---
 tools/testing/selftests/debugfs/Makefile      |  15 ++
 tools/testing/selftests/debugfs/minmax_test.c | 140 +++++++++++++++++
 .../testing/selftests/debugfs/test_minmax.sh  | 147 ++++++++++++++++++
 3 files changed, 302 insertions(+)
 create mode 100644 tools/testing/selftests/debugfs/Makefile
 create mode 100644 tools/testing/selftests/debugfs/minmax_test.c
 create mode 100755 tools/testing/selftests/debugfs/test_minmax.sh

diff --git a/tools/testing/selftests/debugfs/Makefile b/tools/testing/selftests/debugfs/Makefile
new file mode 100644
index 000000000000..5dddaaf6c7a2
--- /dev/null
+++ b/tools/testing/selftests/debugfs/Makefile
@@ -0,0 +1,15 @@
+obj-m := minmax_test.o
+
+KDIR := ../../../..
+PWD := $(shell pwd)
+WARN_FLAGS += -Wall
+
+.PHONY: all
+all:
+	$(MAKE) -C $(KDIR) M=$(PWD) modules
+
+clean:
+	$(MAKE) -C $(KDIR) M=$(PWD) clean
+
+
+include ../lib.mk
diff --git a/tools/testing/selftests/debugfs/minmax_test.c b/tools/testing/selftests/debugfs/minmax_test.c
new file mode 100644
index 000000000000..0e0f6bcd6302
--- /dev/null
+++ b/tools/testing/selftests/debugfs/minmax_test.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Test debugfs minmax simple files.
+ *
+ * Copyright (c) 2023 Oracle and/or its affiliates.
+ * Author: Chris Hyser <chris.hyser@oracle.com>
+ *
+ * This library is free software; you can redistribute it and/or modify it
+ * under the terms of version 2.1 of the GNU Lesser General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this library; if not, see <http://www.gnu.org/licenses>.
+ */
+
+#include <linux/debugfs.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+MODULE_LICENSE("GPL");
+
+#define MIN_MAX 1
+
+static struct dentry *dir = NULL;
+
+#ifdef MIN_MAX
+u8 test_max_8 = 250;
+DEBUGFS_MAX_ATTRIBUTES(test_max_8_p, &test_max_8, 252);
+
+u8 test_min_8 = 250;
+DEBUGFS_MIN_ATTRIBUTES(test_min_8_p, &test_min_8, 25);
+
+u8 test_minmax_8 = 250;
+DEBUGFS_MINMAX_ATTRIBUTES(test_minmax_8_p, &test_minmax_8, 100, 253);
+
+
+u16 test_max_16 = 250;
+DEBUGFS_MAX_ATTRIBUTES(test_max_16_p, &test_max_16, 0x1fff);
+
+u16 test_min_16 = 250;
+DEBUGFS_MIN_ATTRIBUTES(test_min_16_p, &test_min_16, 100);
+
+u16 test_minmax_16 = 250;
+DEBUGFS_MINMAX_ATTRIBUTES(test_minmax_16_p, &test_minmax_16, 100, 300);
+
+
+u32 test_max_32 = 250;
+DEBUGFS_MAX_ATTRIBUTES(test_max_32_p, &test_max_32, 0x1fffffff);
+
+u32 test_min_32 = 250;
+DEBUGFS_MIN_ATTRIBUTES(test_min_32_p, &test_min_32, 100);
+
+u32 test_minmax_32 = 250;
+DEBUGFS_MINMAX_ATTRIBUTES(test_minmax_32_p, &test_minmax_32, 100, 0x1fffffff);
+
+/* test setting everything directly */
+u32 test_minmax_32_s = 250;
+static struct debugfs_minmax_params test_minmax_32_ps = {
+	.value = &test_minmax_32_s,
+	.min = 100,
+	.max = 0x1fffffff,
+	.flags = 3,
+};
+
+u64 test_max_64 = 250;
+DEBUGFS_MAX_ATTRIBUTES(test_max_64_p, &test_max_64, 0x1fffffff);
+
+u64 test_min_64 = 250;
+DEBUGFS_MIN_ATTRIBUTES(test_min_64_p, &test_min_64, 100);
+
+u64 test_minmax_64 = 250;
+DEBUGFS_MINMAX_ATTRIBUTES(test_minmax_64_p, &test_minmax_64, 100, 300);
+#endif
+
+u8 test_reg_8 = 250;
+u16 test_reg_16 = 250;
+u32 test_reg_32 = 250;
+u64 test_reg_64 = 250;
+
+u8 test_regx_8 = 250;
+u16 test_regx_16 = 250;
+u32 test_regx_32 = 250;
+u64 test_regx_64 = 250;
+
+
+/* File names consist of test_<test_type>_<1st param>_<optional sec
+ * param>_<storage size in bits>.
+ */
+
+int init_module(void)
+{
+	pr_err("minmax_test: init_module()\n");
+	dir = debugfs_create_dir("minmax_test", 0);
+	if (dir == NULL) {
+		printk(KERN_ALERT "minmax_test: can't create dir 'minmax_test'\n");
+		return -1;
+	}
+
+#ifdef MIN_MAX
+        debugfs_create_u8_minmax("test_max_252_8", 0644, dir, &test_max_8_p);
+        debugfs_create_u8_minmax("test_min_25_8", 0644, dir, &test_min_8_p);
+        debugfs_create_u8_minmax("test_minmax_100_253_8", 0644, dir, &test_minmax_8_p);
+
+        debugfs_create_u16_minmax("test_max_0x1fff_16", 0644, dir, &test_max_16_p);
+        debugfs_create_u16_minmax("test_min_100_16", 0644, dir, &test_min_16_p);
+        debugfs_create_u16_minmax("test_minmax_100_300_16", 0644, dir, &test_minmax_16_p);
+
+        debugfs_create_u32_minmax("test_max_0x1fffffff_32", 0644, dir, &test_max_32_p);
+        debugfs_create_u32_minmax("test_min_100_32", 0644, dir, &test_min_32_p);
+        debugfs_create_u32_minmax("test_minmax_100_0x1fffffff_32", 0644, dir, &test_minmax_32_p);
+        debugfs_create_u32_minmax("testss_minmax_100_0x1fffffff_32", 0644, dir, &test_minmax_32_ps);
+
+        debugfs_create_u64_minmax("test_max_0x1fffffff_64", 0644, dir, &test_max_64_p);
+        debugfs_create_u64_minmax("test_min_100_64", 0644, dir, &test_min_64_p);
+        debugfs_create_u64_minmax("test_minmax_100_300_64", 0644, dir, &test_minmax_64_p);
+#endif
+
+        debugfs_create_u8("test_reg_8", 0644, dir, &test_reg_8);
+        debugfs_create_u16("test_reg_16", 0644, dir, &test_reg_16);
+        debugfs_create_u32("test_reg_32", 0644, dir, &test_reg_32);
+        debugfs_create_u64("test_reg_64", 0644, dir, &test_reg_64);
+
+        debugfs_create_x8("test_regx_8", 0644, dir, &test_regx_8);
+        debugfs_create_x16("test_regx_16", 0644, dir, &test_regx_16);
+        debugfs_create_x32("test_regx_32", 0644, dir, &test_regx_32);
+        debugfs_create_x64("test_regx_64", 0644, dir, &test_regx_64);
+	return 0;
+}
+
+void cleanup_module(void)
+{
+	pr_err("minmax_test: cleanup_module()\n");
+	debugfs_remove_recursive(dir);
+}
diff --git a/tools/testing/selftests/debugfs/test_minmax.sh b/tools/testing/selftests/debugfs/test_minmax.sh
new file mode 100755
index 000000000000..67ed7b9abac2
--- /dev/null
+++ b/tools/testing/selftests/debugfs/test_minmax.sh
@@ -0,0 +1,147 @@
+#!/bin/bash
+
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Test debugfs minmax simple files.
+#
+# Copyright (c) 2023 Oracle and/or its affiliates.
+# Author: Chris Hyser <chris.hyser@oracle.com>
+#
+# This library is free software; you can redistribute it and/or modify it
+# under the terms of version 2.1 of the GNU Lesser General Public License as
+# published by the Free Software Foundation.
+#
+# This library is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License
+# for more details.
+#
+# You should have received a copy of the GNU Lesser General Public License
+# along with this library; if not, see <http://www.gnu.org/licenses>.
+
+rmmod minmax_test 2>/dev/null
+insmod minmax_test.ko
+
+cd /sys/kernel/debug/minmax_test
+pwd
+
+function split_wds()
+{
+    words=()
+    local IFS=_
+    for w in $1; do
+	words+=($w)
+    done
+}
+
+function test_max()
+{
+    echo $2 > $1
+    local res=$?
+    local rv=$(cat $1)
+    if ! (( res == 0 && $2 == rv )); then
+	return 1
+    fi
+
+    local max=$(($2 + 1))
+    echo $max > $1 2>/dev/null
+    res=$?
+    rv=$(cat $1)
+    if ! [[ $res -ne 0 && $2 -eq $rv ]]; then
+	return 1
+    fi
+    return 0
+}
+
+function test_min()
+{
+    echo $2 > $1
+    local res=$?
+    local rv=$(cat $1)
+    if ! [[ $res -eq 0 && $2 -eq $rv ]]; then
+	return 1
+    fi
+
+    local min=$(($2 - 1))
+    echo $min > $1 2>/dev/null
+    res=$?
+    rv=$(cat $1)
+    if ! [[ $res -ne 0 && $2 -eq $rv ]]; then
+	return 1
+    fi
+    return 0
+}
+
+function test_minmax()
+{
+    test_min $1 ${words[2]}
+    if [[ $? -ne 0 ]]; then
+	echo "$1: min error"
+        return 1
+    fi
+    test_max $1 ${words[3]}
+    if [[ $? -ne 0 ]]; then
+	echo "$1: max error"
+        return 1
+    fi
+    return 0
+}
+
+function test_reg()
+{
+    if [[ $2 -eq 64 ]]; then
+	local V=313
+
+	echo $V > $1 2>/dev/null
+
+	local rv=$(cat $1)
+	if ! (( rv == V )); then
+	    return 1
+	fi
+	return 0
+    fi
+
+    local max=$(((1 << $2) - 1))
+    test_max $1 $max
+    if [[ $? -ne 0 ]]; then
+	echo "$1: reg/x error"
+        return 1
+    fi
+    return 0
+}
+
+rc=0
+for f in *; do
+    split_wds $f
+    echo file: $f
+
+    if [[ ${words[1]} == "min" ]]; then
+	test_min $f ${words[2]}
+	if [[ $? -ne 0 ]]; then
+	    echo "$f: min error"
+	    rc=1
+	fi
+    elif [[ ${words[1]} == "max" ]]; then
+	test_max $f ${words[2]}
+	if [[ $? -ne 0 ]]; then
+	    echo "$f: max error"
+	    rc=1
+	fi
+    elif [[ ${words[1]} == "minmax" ]]; then
+	test_minmax $f ${words[2]} ${words[3]}
+	if [[ $? -ne 0 ]]; then
+	    echo "$f: minmax error"
+	    rc=1
+	fi
+    elif [[ ${words[1]} == "reg" || ${words[1]} == "regx" ]]; then
+	test_reg $f ${words[2]}
+	if [[ $? -ne 0 ]]; then
+	    echo "$f: reg/x error"
+	    rc=1
+	fi
+    fi
+done
+
+rmmod minmax_test
+
+exit $rc
-- 
2.31.1

[PATCH 4/4] sched/numa: Fix divide by zero for sysctl_numa_balancing_scan_size.

[chris hyser]

Commit 6419265899d9 ("sched/fair: Fix division by zero
sysctl_numa_balancing_scan_size") prevented a divide by zero by using
sysctl mechanisms to return EINVAL for a sysctl_numa_balancing_scan_size
value of zero. When moved from a sysctl to a debugfs file, this checking
was lost.

This patch puts zero checking back in place using the debugfs minmax
checking mechanism.

Cc: stable@vger.kernel.org
Fixes: 8a99b6833c88 ("sched: Move SCHED_DEBUG sysctl to debugfs")
Signed-off-by: Chris Hyser <chris.hyser@oracle.com>
---
 kernel/sched/debug.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/debug.c b/kernel/sched/debug.c
index 066ff1c8ae4e..b9e53ecc539a 100644
--- a/kernel/sched/debug.c
+++ b/kernel/sched/debug.c
@@ -216,6 +216,13 @@ static const struct file_operations sched_scaling_fops = {
 
 #endif /* SMP */
 
+#ifdef CONFIG_NUMA_BALANCING
+
+/* prevent divide by zero */
+DEBUGFS_MIN_ATTRIBUTES(scan_size_mb_params, &sysctl_numa_balancing_scan_size, 1);
+
+#endif /* CONFIG_NUMA_BALANCING */
+
 #ifdef CONFIG_PREEMPT_DYNAMIC
 
 static ssize_t sched_dynamic_write(struct file *filp, const char __user *ubuf,
@@ -371,7 +378,7 @@ static __init int sched_init_debug(void)
 	debugfs_create_u32("scan_delay_ms", 0644, numa, &sysctl_numa_balancing_scan_delay);
 	debugfs_create_u32("scan_period_min_ms", 0644, numa, &sysctl_numa_balancing_scan_period_min);
 	debugfs_create_u32("scan_period_max_ms", 0644, numa, &sysctl_numa_balancing_scan_period_max);
-	debugfs_create_u32("scan_size_mb", 0644, numa, &sysctl_numa_balancing_scan_size);
+	debugfs_create_u32_minmax("scan_size_mb", 0644, numa, &scan_size_mb_params);
 	debugfs_create_u32("hot_threshold_ms", 0644, numa, &sysctl_numa_balancing_hot_threshold);
 #endif
 
-- 
2.31.1

Re: [PATCH v2 0/4] debugfs: Add simple min/max "files" to debugfs to fix sched debug code.

[Greg KH]

On Tue, May 30, 2023 at 03:40:08PM -0400, chris hyser wrote:
> v2:
> Apologies. I sent this the first time without including lkml.
> 
> v1:
> This originally started as an attempt to solve a divide by zero issue in sched
> debug code that was introduced when a sysctl value with non-zero checking was
> moved to a simple u32 debugfs file. In looking at ways to solve this, it was
> mentioned I should look at providing general debugfs files with min/max
> checking. 
> 
> One problem was that a check for greater than zero for say a u8 succeeds for a
> number like 256 (but stores a zero anyway) as the upper bits that don't fit into
> storage are silently dropped. Therefore values greater than the storage capacity
> must also fail. Getting an error when what the user wrote is not what was
> actually stored, seems like a useful requirement for the other simple files and
> so I moved the check into there.
> 
> To enable easy testing, a test module and test script are provided which can
> validate the new functions as well as check the new limits on the older
> functions. This was stuck under selftests, but it is not currently tied into the
> testing infrastructure.

This is a lot of new infrastructure for a single debugfs file that you
could just check for in the file write callback instead.

I'm all for cool new features, but wow, this seems like major overkill.
Are you _SURE_ you need it all?

thanks,

greg k-h

Re: [PATCH v2 0/4] debugfs: Add simple min/max "files" to debugfs to fix sched debug code.

[chris hyser]

On 5/30/23 16:18, Greg KH wrote:
> On Tue, May 30, 2023 at 03:40:08PM -0400, chris hyser wrote:
>> v2:
>> Apologies. I sent this the first time without including lkml.
>>
>> v1:
>> This originally started as an attempt to solve a divide by zero issue in sched
>> debug code that was introduced when a sysctl value with non-zero checking was
>> moved to a simple u32 debugfs file. In looking at ways to solve this, it was
>> mentioned I should look at providing general debugfs files with min/max
>> checking.
>>
>> One problem was that a check for greater than zero for say a u8 succeeds for a
>> number like 256 (but stores a zero anyway) as the upper bits that don't fit into
>> storage are silently dropped. Therefore values greater than the storage capacity
>> must also fail. Getting an error when what the user wrote is not what was
>> actually stored, seems like a useful requirement for the other simple files and
>> so I moved the check into there.
>>
>> To enable easy testing, a test module and test script are provided which can
>> validate the new functions as well as check the new limits on the older
>> functions. This was stuck under selftests, but it is not currently tied into the
>> testing infrastructure.
> 
> This is a lot of new infrastructure for a single debugfs file that you
> could just check for in the file write callback instead.
> 
> I'm all for cool new features, but wow, this seems like major overkill.
> Are you _SURE_ you need it all?

The current use case obviously does not require all of this. The idea 
was instead of a one off fix, it might be useful to provide a generic 
solution to a common class of problems.

So sending this out hopefully would help determine the perceived usefulness.

-chrish

Re: [PATCH v2 0/4] debugfs: Add simple min/max "files" to debugfs to fix sched debug code.

[chris hyser]

On 5/30/23 17:41, chris hyser wrote:
> On 5/30/23 16:18, Greg KH wrote:
>> On Tue, May 30, 2023 at 03:40:08PM -0400, chris hyser wrote:
>>> v2:
>>> Apologies. I sent this the first time without including lkml.
>>>
>>> v1:
>>> This originally started as an attempt to solve a divide by zero issue 
>>> in sched
>>> debug code that was introduced when a sysctl value with non-zero 
>>> checking was
>>> moved to a simple u32 debugfs file. In looking at ways to solve this, 
>>> it was
>>> mentioned I should look at providing general debugfs files with min/max
>>> checking.
>>>
>>> One problem was that a check for greater than zero for say a u8 
>>> succeeds for a
>>> number like 256 (but stores a zero anyway) as the upper bits that 
>>> don't fit into
>>> storage are silently dropped. Therefore values greater than the 
>>> storage capacity
>>> must also fail. Getting an error when what the user wrote is not what 
>>> was
>>> actually stored, seems like a useful requirement for the other simple 
>>> files and
>>> so I moved the check into there.
>>>
>>> To enable easy testing, a test module and test script are provided 
>>> which can
>>> validate the new functions as well as check the new limits on the older
>>> functions. This was stuck under selftests, but it is not currently 
>>> tied into the
>>> testing infrastructure.
>>
>> This is a lot of new infrastructure for a single debugfs file that you
>> could just check for in the file write callback instead.
>>
>> I'm all for cool new features, but wow, this seems like major overkill.
>> Are you _SURE_ you need it all?

I do want to clarify about the size of this. It is 4 new create file 
functions, 8 static get/sets, a new struct and some simple macros. In 
keeping the style of the new code similar to prior, the lines of 
function comments for the new creates() almost equals the lines of code.

This doesn't seem to me to be as much overkill as say the xsigned 
version of these files which consumes 12 struct file_operations simply 
to provide a different string format.

-chrish