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(-)
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * 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 <[email protected]>
+#
+# 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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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
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 <[email protected]>
>> ---
>> 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
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
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