This is part of the general push to deprecate register_sysctl_paths and
register_sysctl_table. Parport driver uses the "CHILD" pointer
in the ctl_table structure to create its directory structure. We move to
the newer register_sysctl call and remove the pointer madness.
I have separated the parport into 5 patches to clarify the different
changes needed for the 3 calls to register_sysctl_paths. I can squash
them together if need be.
We no longer export the register_sysctl_table call as parport was the
last user from outside proc_sysctl.c. Also modified documentation slightly
so register_sysctl_table is no longer mentioned.
I'm waiting on the 0-day tests results.
Best
Joel
Joel Granados (6):
parport: Move magic number "15" to a define
parport: Remove register_sysctl_table from parport_proc_register
parport: Remove register_sysctl_table from
parport_device_proc_register
parport: Remove register_sysctl_table from
parport_default_proc_register
parport: Removed sysctl related defines
sysctl: stop exporting register_sysctl_table
drivers/parport/procfs.c | 171 +++++++++++++++++++++------------------
drivers/parport/share.c | 2 +-
fs/proc/proc_sysctl.c | 5 +-
include/linux/parport.h | 2 +
include/linux/sysctl.h | 8 +-
5 files changed, 97 insertions(+), 91 deletions(-)
--
2.30.2
This is part of the general push to deprecate register_sysctl_paths and
register_sysctl_table. We use a temp allocation to include both port and
device name in proc. Allocated mem is freed at the end. The unused
parport_device_sysctl_template struct elements that are not used are
removed.
Signed-off-by: Joel Granados <[email protected]>
---
drivers/parport/procfs.c | 57 +++++++++++++++++++++++-----------------
1 file changed, 33 insertions(+), 24 deletions(-)
diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index 53ae5cb98190..902547eb045c 100644
--- a/drivers/parport/procfs.c
+++ b/drivers/parport/procfs.c
@@ -384,6 +384,7 @@ parport_device_sysctl_template = {
.extra1 = (void*) &parport_min_timeslice_value,
.extra2 = (void*) &parport_max_timeslice_value
},
+ {}
},
{
{
@@ -394,22 +395,6 @@ parport_device_sysctl_template = {
.child = NULL
},
{}
- },
- {
- PARPORT_DEVICES_ROOT_DIR,
- {}
- },
- {
- PARPORT_PORT_DIR(NULL),
- {}
- },
- {
- PARPORT_PARPORT_DIR(NULL),
- {}
- },
- {
- PARPORT_DEV_DIR(NULL),
- {}
}
};
@@ -547,30 +532,54 @@ int parport_proc_unregister(struct parport *port)
int parport_device_proc_register(struct pardevice *device)
{
+ int err = 0;
struct parport_device_sysctl_table *t;
struct parport * port = device->port;
+ size_t port_name_len, device_name_len, tmp_dir_path_len;
+ char *tmp_dir_path;
t = kmemdup(&parport_device_sysctl_template, sizeof(*t), GFP_KERNEL);
if (t == NULL)
return -ENOMEM;
- t->dev_dir[0].child = t->parport_dir;
- t->parport_dir[0].child = t->port_dir;
- t->port_dir[0].procname = port->name;
- t->port_dir[0].child = t->devices_root_dir;
- t->devices_root_dir[0].child = t->device_dir;
+ port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
+ device_name_len = strnlen(device->name, PATH_MAX);
+
+ /* Allocate a buffer for two paths: dev/parport/PORT/devices/DEVICE. */
+ tmp_dir_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len + device_name_len;
+ tmp_dir_path = kmalloc(tmp_dir_path_len, GFP_KERNEL);
+ if (!tmp_dir_path) {
+ err = -ENOMEM;
+ goto exit_free_t;
+ }
+
+ if (tmp_dir_path_len
+ <= snprintf(tmp_dir_path, tmp_dir_path_len, "dev/parport/%s/devices/%s",
+ port->name, device->name)) {
+ err = -ENOENT;
+ goto exit_free_path;
+ }
- t->device_dir[0].procname = device->name;
- t->device_dir[0].child = t->vars;
t->vars[0].data = &device->timeslice;
- t->sysctl_header = register_sysctl_table(t->dev_dir);
+ t->sysctl_header = register_sysctl(tmp_dir_path, t->vars);
if (t->sysctl_header == NULL) {
kfree(t);
t = NULL;
}
device->sysctl_table = t;
+
+ kfree(tmp_dir_path);
return 0;
+
+exit_free_path:
+ kfree(tmp_dir_path);
+
+exit_free_t:
+ kfree(t);
+ t = NULL;
+
+ return err;
}
int parport_device_proc_unregister(struct pardevice *device)
--
2.30.2
This is part of the general push to deprecate register_sysctl_paths and
register_sysctl_table. Simply change the full path "dev/parport/default"
to point to an already existing set of table entries (vars). We also
remove the unused elements from parport_default_table.
Signed-off-by: Joel Granados <[email protected]>
---
drivers/parport/procfs.c | 18 +-----------------
1 file changed, 1 insertion(+), 17 deletions(-)
diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index 902547eb045c..56f825fcfae6 100644
--- a/drivers/parport/procfs.c
+++ b/drivers/parport/procfs.c
@@ -430,22 +430,6 @@ parport_default_sysctl_table = {
.extra2 = (void*) &parport_max_spintime_value
},
{}
- },
- {
- {
- .procname = "default",
- .mode = 0555,
- .child = parport_default_sysctl_table.vars
- },
- {}
- },
- {
- PARPORT_PARPORT_DIR(parport_default_sysctl_table.default_dir),
- {}
- },
- {
- PARPORT_DEV_DIR(parport_default_sysctl_table.parport_dir),
- {}
}
};
@@ -598,7 +582,7 @@ static int __init parport_default_proc_register(void)
int ret;
parport_default_sysctl_table.sysctl_header =
- register_sysctl_table(parport_default_sysctl_table.dev_dir);
+ register_sysctl("dev/parport/default", parport_default_sysctl_table.vars);
if (!parport_default_sysctl_table.sysctl_header)
return -ENOMEM;
ret = parport_bus_init();
--
2.30.2
Put the size of a parport name behind a define so we can use it in other
files. This is a preparation patch to be able to use this size in
parport/procfs.c.
Signed-off-by: Joel Granados <[email protected]>
---
drivers/parport/share.c | 2 +-
include/linux/parport.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/parport/share.c b/drivers/parport/share.c
index 62f8407923d4..2d46b1d4fd69 100644
--- a/drivers/parport/share.c
+++ b/drivers/parport/share.c
@@ -467,7 +467,7 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma,
atomic_set(&tmp->ref_count, 1);
INIT_LIST_HEAD(&tmp->full_list);
- name = kmalloc(15, GFP_KERNEL);
+ name = kmalloc(PARPORT_NAME_MAX_LEN, GFP_KERNEL);
if (!name) {
kfree(tmp);
return NULL;
diff --git a/include/linux/parport.h b/include/linux/parport.h
index a0bc9e0267b7..243c82d7f852 100644
--- a/include/linux/parport.h
+++ b/include/linux/parport.h
@@ -180,6 +180,8 @@ struct ieee1284_info {
struct semaphore irq;
};
+#define PARPORT_NAME_MAX_LEN 15
+
/* A parallel port */
struct parport {
unsigned long base; /* base address */
--
2.30.2
We make register_sysctl_table static because the only function calling
it is in fs/proc/proc_sysctl.c (__register_sysctl_base). We remove it
from the sysctl.h header and modify the documentation in both the header
and proc_sysctl.c files to mention "register_sysctl" instead of
"register_sysctl_table".
Signed-off-by: Joel Granados <[email protected]>
---
fs/proc/proc_sysctl.c | 5 ++---
include/linux/sysctl.h | 8 +-------
2 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
index 8038833ff5b0..f8f19e000d76 100644
--- a/fs/proc/proc_sysctl.c
+++ b/fs/proc/proc_sysctl.c
@@ -1582,7 +1582,7 @@ static int register_leaf_sysctl_tables(const char *path, char *pos,
* array. A completely 0 filled entry terminates the table.
* We are slowly deprecating this call so avoid its use.
*/
-struct ctl_table_header *register_sysctl_table(struct ctl_table *table)
+static struct ctl_table_header *register_sysctl_table(struct ctl_table *table)
{
struct ctl_table *ctl_table_arg = table;
int nr_subheaders = count_subheaders(table);
@@ -1634,7 +1634,6 @@ struct ctl_table_header *register_sysctl_table(struct ctl_table *table)
header = NULL;
goto out;
}
-EXPORT_SYMBOL(register_sysctl_table);
int __register_sysctl_base(struct ctl_table *base_table)
{
@@ -1700,7 +1699,7 @@ static void drop_sysctl_table(struct ctl_table_header *header)
/**
* unregister_sysctl_table - unregister a sysctl table hierarchy
- * @header: the header returned from register_sysctl_table
+ * @header: the header returned from register_sysctl or __register_sysctl_table
*
* Unregisters the sysctl table and all children. proc entries may not
* actually be removed until they are no longer used by anyone.
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 3d08277959af..218e56a26fb0 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -89,7 +89,7 @@ int proc_do_static_key(struct ctl_table *table, int write, void *buffer,
size_t *lenp, loff_t *ppos);
/*
- * Register a set of sysctl names by calling register_sysctl_table
+ * Register a set of sysctl names by calling register_sysctl
* with an initialised array of struct ctl_table's. An entry with
* NULL procname terminates the table. table->de will be
* set up by the registration and need not be initialised in advance.
@@ -222,7 +222,6 @@ struct ctl_table_header *__register_sysctl_table(
struct ctl_table_set *set,
const char *path, struct ctl_table *table);
struct ctl_table_header *register_sysctl(const char *path, struct ctl_table *table);
-struct ctl_table_header *register_sysctl_table(struct ctl_table * table);
void unregister_sysctl_table(struct ctl_table_header * table);
extern int sysctl_init_bases(void);
@@ -257,11 +256,6 @@ static inline int __register_sysctl_base(struct ctl_table *base_table)
#define register_sysctl_base(table) __register_sysctl_base(table)
-static inline struct ctl_table_header *register_sysctl_table(struct ctl_table * table)
-{
- return NULL;
-}
-
static inline void register_sysctl_init(const char *path, struct ctl_table *table)
{
}
--
2.30.2
The partport driver used to rely on defines to include different
directories in sysctl. Now that we have made the transition to
register_sysctl from regsiter_sysctl_table, they are no longer needed.
Signed-off-by: Joel Granados <[email protected]>
---
drivers/parport/procfs.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index 56f825fcfae6..e3f773ea6b4f 100644
--- a/drivers/parport/procfs.c
+++ b/drivers/parport/procfs.c
@@ -243,13 +243,6 @@ do { \
return 0;
}
-#define PARPORT_PORT_DIR(CHILD) { .procname = NULL, .mode = 0555, .child = CHILD }
-#define PARPORT_PARPORT_DIR(CHILD) { .procname = "parport", \
- .mode = 0555, .child = CHILD }
-#define PARPORT_DEV_DIR(CHILD) { .procname = "dev", .mode = 0555, .child = CHILD }
-#define PARPORT_DEVICES_ROOT_DIR { .procname = "devices", \
- .mode = 0555, .child = NULL }
-
static const unsigned long parport_min_timeslice_value =
PARPORT_MIN_TIMESLICE_VALUE;
--
2.30.2
This is part of the general push to deprecate register_sysctl_paths and
register_sysctl_table. Register dev/parport/PORTNAME and
dev/parport/PORTNAME/devices. Temporary allocation for name is freed at
the end of the function. Remove all the struct elements that are no
longer used in the parport_device_sysctl_template struct. Add parport
specific defines that hide the base path sizes.
Signed-off-by: Joel Granados <[email protected]>
---
drivers/parport/procfs.c | 89 +++++++++++++++++++++++++---------------
1 file changed, 57 insertions(+), 32 deletions(-)
diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
index d740eba3c099..53ae5cb98190 100644
--- a/drivers/parport/procfs.c
+++ b/drivers/parport/procfs.c
@@ -32,6 +32,13 @@
#define PARPORT_MAX_TIMESLICE_VALUE ((unsigned long) HZ)
#define PARPORT_MIN_SPINTIME_VALUE 1
#define PARPORT_MAX_SPINTIME_VALUE 1000
+/*
+ * PARPORT_BASE_* is the size of the known parts of the sysctl path
+ * in dev/partport/%s/devices/%s. "dev/parport/"(12), "/devices/"(9
+ * and null char(1).
+ */
+#define PARPORT_BASE_PATH_SIZE 13
+#define PARPORT_BASE_DEVICES_PATH_SIZE 22
static int do_active_device(struct ctl_table *table, int write,
void *result, size_t *lenp, loff_t *ppos)
@@ -260,9 +267,6 @@ struct parport_sysctl_table {
struct ctl_table_header *sysctl_header;
struct ctl_table vars[12];
struct ctl_table device_dir[2];
- struct ctl_table port_dir[2];
- struct ctl_table parport_dir[2];
- struct ctl_table dev_dir[2];
};
static const struct parport_sysctl_table parport_sysctl_template = {
@@ -305,7 +309,6 @@ static const struct parport_sysctl_table parport_sysctl_template = {
.mode = 0444,
.proc_handler = do_hardware_modes
},
- PARPORT_DEVICES_ROOT_DIR,
#ifdef CONFIG_PARPORT_1284
{
.procname = "autoprobe",
@@ -355,18 +358,6 @@ static const struct parport_sysctl_table parport_sysctl_template = {
},
{}
},
- {
- PARPORT_PORT_DIR(NULL),
- {}
- },
- {
- PARPORT_PARPORT_DIR(NULL),
- {}
- },
- {
- PARPORT_DEV_DIR(NULL),
- {}
- }
};
struct parport_device_sysctl_table
@@ -477,7 +468,9 @@ parport_default_sysctl_table = {
int parport_proc_register(struct parport *port)
{
struct parport_sysctl_table *t;
- int i;
+ char *tmp_dir_path;
+ size_t tmp_path_len, port_name_len;
+ int i, err = 0;
t = kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL);
if (t == NULL)
@@ -485,28 +478,60 @@ int parport_proc_register(struct parport *port)
t->device_dir[0].extra1 = port;
- for (i = 0; i < 5; i++)
- t->vars[i].extra1 = port;
-
t->vars[0].data = &port->spintime;
- t->vars[5].child = t->device_dir;
-
- for (i = 0; i < 5; i++)
- t->vars[6 + i].extra2 = &port->probe_info[i];
+ for (i = 0; i < 5; i++) {
+ t->vars[i].extra1 = port;
+ t->vars[5 + i].extra2 = &port->probe_info[i];
+ }
+
+ port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
+ /*
+ * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
+ * We calculate for the second as that will give us enough for the first.
+ */
+ tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
+ tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
+ if (!tmp_dir_path) {
+ err = -ENOMEM;
+ goto exit_free_t;
+ }
- t->port_dir[0].procname = port->name;
+ if (tmp_path_len
+ <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
+ kfree(tmp_dir_path);
+ return -ENOENT;
+ }
+ if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
+ /*
+ * We keep the original behavior of parport where failure to register
+ * does not return error.
+ */
+ goto exit_free_tmp_dir_path;
- t->port_dir[0].child = t->vars;
- t->parport_dir[0].child = t->port_dir;
- t->dev_dir[0].child = t->parport_dir;
- t->sysctl_header = register_sysctl_table(t->dev_dir);
- if (t->sysctl_header == NULL) {
- kfree(t);
- t = NULL;
+ tmp_path_len = PARPORT_BASE_PATH_SIZE + port_name_len;
+ if (tmp_path_len
+ <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s", port->name)) {
+ err = -ENOENT;
+ goto exit_free_tmp_dir_path;
}
+
+ t->sysctl_header = register_sysctl(tmp_dir_path, t->vars);
+ if (t->sysctl_header == NULL)
+ goto exit_free_tmp_dir_path;
+
port->sysctl_table = t;
+
+ kfree(tmp_dir_path);
return 0;
+
+exit_free_tmp_dir_path:
+ kfree(tmp_dir_path);
+exit_free_t:
+ kfree(t);
+ t = NULL;
+ return err;
+
}
int parport_proc_unregister(struct parport *port)
--
2.30.2
On Mon, May 15, 2023 at 09:14:43AM +0200, Joel Granados wrote:
> This is part of the general push to deprecate register_sysctl_paths and
> register_sysctl_table. We use a temp allocation to include both port and
> device name in proc. Allocated mem is freed at the end. The unused
> parport_device_sysctl_template struct elements that are not used are
> removed.
>
> Signed-off-by: Joel Granados <[email protected]>
> ---
> drivers/parport/procfs.c | 57 +++++++++++++++++++++++-----------------
> 1 file changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c
> index 53ae5cb98190..902547eb045c 100644
> --- a/drivers/parport/procfs.c
> +++ b/drivers/parport/procfs.c
> @@ -384,6 +384,7 @@ parport_device_sysctl_template = {
> .extra1 = (void*) &parport_min_timeslice_value,
> .extra2 = (void*) &parport_max_timeslice_value
> },
> + {}
> },
> {
> {
> @@ -394,22 +395,6 @@ parport_device_sysctl_template = {
> .child = NULL
> },
> {}
> - },
> - {
> - PARPORT_DEVICES_ROOT_DIR,
> - {}
> - },
> - {
> - PARPORT_PORT_DIR(NULL),
> - {}
> - },
> - {
> - PARPORT_PARPORT_DIR(NULL),
> - {}
> - },
> - {
> - PARPORT_DEV_DIR(NULL),
> - {}
> }
> };
>
> @@ -547,30 +532,54 @@ int parport_proc_unregister(struct parport *port)
>
> int parport_device_proc_register(struct pardevice *device)
> {
> + int err = 0;
> struct parport_device_sysctl_table *t;
> struct parport * port = device->port;
> + size_t port_name_len, device_name_len, tmp_dir_path_len;
> + char *tmp_dir_path;
>
> t = kmemdup(&parport_device_sysctl_template, sizeof(*t), GFP_KERNEL);
> if (t == NULL)
> return -ENOMEM;
>
> - t->dev_dir[0].child = t->parport_dir;
> - t->parport_dir[0].child = t->port_dir;
> - t->port_dir[0].procname = port->name;
> - t->port_dir[0].child = t->devices_root_dir;
> - t->devices_root_dir[0].child = t->device_dir;
> + port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> + device_name_len = strnlen(device->name, PATH_MAX);
> +
> + /* Allocate a buffer for two paths: dev/parport/PORT/devices/DEVICE. */
> + tmp_dir_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len + device_name_len;
> + tmp_dir_path = kmalloc(tmp_dir_path_len, GFP_KERNEL);
> + if (!tmp_dir_path) {
> + err = -ENOMEM;
> + goto exit_free_t;
> + }
> +
> + if (tmp_dir_path_len
> + <= snprintf(tmp_dir_path, tmp_dir_path_len, "dev/parport/%s/devices/%s",
> + port->name, device->name)) {
> + err = -ENOENT;
> + goto exit_free_path;
> + }
>
> - t->device_dir[0].procname = device->name;
> - t->device_dir[0].child = t->vars;
> t->vars[0].data = &device->timeslice;
>
> - t->sysctl_header = register_sysctl_table(t->dev_dir);
> + t->sysctl_header = register_sysctl(tmp_dir_path, t->vars);
> if (t->sysctl_header == NULL) {
> kfree(t);
> t = NULL;
In the paprport_proc_register there is the same logic where we do not
return error code on error. Additionally, noone checks the return values
of parport_proc_register nor parport_device_proc_register. Should we
just change these to void and be done with it? Or is it better to change
parport/share.c to take care of the error codes?
I realized this after some comments from 0-day.
Best
> }
> device->sysctl_table = t;
> +
> + kfree(tmp_dir_path);
> return 0;
> +
> +exit_free_path:
> + kfree(tmp_dir_path);
> +
> +exit_free_t:
> + kfree(t);
> + t = NULL;
> +
> + return err;
> }
>
> int parport_device_proc_unregister(struct pardevice *device)
> --
> 2.30.2
>
--
Joel Granados
On Mon, May 15, 2023 at 09:14:40AM +0200, Joel Granados wrote:
> This is part of the general push to deprecate register_sysctl_paths and
> register_sysctl_table. Parport driver uses the "CHILD" pointer
> in the ctl_table structure to create its directory structure. We move to
> the newer register_sysctl call and remove the pointer madness.
Nice! Thanks for doing this and unwinding the sysctl use case which
takes the cake for the obfuscation use case of sysctls.
> I have separated the parport into 5 patches to clarify the different
> changes needed for the 3 calls to register_sysctl_paths. I can squash
> them together if need be.
I think it makes sense to keep them that way.
> We no longer export the register_sysctl_table call as parport was the
> last user from outside proc_sysctl.c. Also modified documentation slightly
> so register_sysctl_table is no longer mentioned.
We can go further, but I'll explain in patch review on the patches.
> I'm waiting on the 0-day tests results.
Nice!
Luis
Awesome!
On Mon, May 15, 2023 at 09:14:42AM +0200, Joel Granados wrote:
> +
> + port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> + /*
> + * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> + * We calculate for the second as that will give us enough for the first.
> + */
> + tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> + tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
Any reason why not kzalloc()?
> + if (!tmp_dir_path) {
> + err = -ENOMEM;
> + goto exit_free_t;
> + }
>
> - t->port_dir[0].procname = port->name;
> + if (tmp_path_len
> + <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
Since we are clearing up obfuscation code, it would be nicer to
make this easier to read and split the snprintf() into one line, capture
the error there. And then in a new line do the check. Even if we have to
add a new int value here.
Other than this I'd just ask to extend the commit log to use
the before and after of vmlinux (when this module is compiled in with all
the bells and whistles) with ./scripts/bloat-o-meter.
Ie build before the patch and cp vmlinux to vmlinux.old and then compare
with:
./scripts/bloat-o-meter vmlinux.old vmlinux
Can you also describe any testing if any.
With the above changes, feel free to add to all these patches:
Reviewed-by: Luis Chamberlain
> + if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
BTW, we should be able to remove now replace register_sysctl_base() with a simple
register_sysctl("kernel", foo), and then one for "fs", and one of "vm"
on kernel/sysctl.c and just remove:
* DECLARE_SYSCTL_BASE() & register_sysctl_base() & __register_sysctl_base()
* and then after all this register_sysctl_table() completely
Let me know if you'd like a stab at it, or if you prefer me to do that.
Luis
On Mon, May 15, 2023 at 01:24:06PM -0700, Luis Chamberlain wrote:
> On Mon, May 15, 2023 at 09:14:40AM +0200, Joel Granados wrote:
> > This is part of the general push to deprecate register_sysctl_paths and
> > register_sysctl_table. Parport driver uses the "CHILD" pointer
> > in the ctl_table structure to create its directory structure. We move to
> > the newer register_sysctl call and remove the pointer madness.
>
> Nice! Thanks for doing this and unwinding the sysctl use case which
> takes the cake for the obfuscation use case of sysctls.
Indeed. They were being very creative :)
>
> > I have separated the parport into 5 patches to clarify the different
> > changes needed for the 3 calls to register_sysctl_paths. I can squash
> > them together if need be.
>
> I think it makes sense to keep them that way.
Fine by me :).
>
> > We no longer export the register_sysctl_table call as parport was the
> > last user from outside proc_sysctl.c. Also modified documentation slightly
> > so register_sysctl_table is no longer mentioned.
>
> We can go further, but I'll explain in patch review on the patches.
ok
>
> > I'm waiting on the 0-day tests results.
>
> Nice!
>
> Luis
--
Joel Granados
On Mon, May 15, 2023 at 09:17:39PM -0700, Luis Chamberlain wrote:
> Awesome!
>
> On Mon, May 15, 2023 at 09:14:42AM +0200, Joel Granados wrote:
> > +
> > + port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> > + /*
> > + * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> > + * We calculate for the second as that will give us enough for the first.
> > + */
> > + tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> > + tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
>
> Any reason why not kzalloc()?
nope. Will zero it out.
>
> > + if (!tmp_dir_path) {
> > + err = -ENOMEM;
> > + goto exit_free_t;
> > + }
> >
> > - t->port_dir[0].procname = port->name;
> > + if (tmp_path_len
> > + <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
>
> Since we are clearing up obfuscation code, it would be nicer to
> make this easier to read and split the snprintf() into one line, capture
> the error there. And then in a new line do the check. Even if we have to
> add a new int value here.
np. Will do something like this:
num_chars_sprinted = snprintf(....
if(tmp_path_len <= num_chars_sprinted) {
err = -ENOENT;
...
}
>
> Other than this I'd just ask to extend the commit log to use
> the before and after of vmlinux (when this module is compiled in with all
> the bells and whistles) with ./scripts/bloat-o-meter.
>
> Ie build before the patch and cp vmlinux to vmlinux.old and then compare
> with:
>
> ./scripts/bloat-o-meter vmlinux.old vmlinux
>
> Can you also describe any testing if any.
Sure thing. Will add the bloat-o-meter output on the last patch so as to
gather the results for all the patches.
I'll write some testing info on the patches.
>
> With the above changes, feel free to add to all these patches:
>
> Reviewed-by: Luis Chamberlain
Ack
>
> > + if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
>
> BTW, we should be able to remove now replace register_sysctl_base() with a simple
> register_sysctl("kernel", foo), and then one for "fs", and one of "vm"
> on kernel/sysctl.c and just remove:
>
> * DECLARE_SYSCTL_BASE() & register_sysctl_base() & __register_sysctl_base()
> * and then after all this register_sysctl_table() completely
>
> Let me know if you'd like a stab at it, or if you prefer me to do that.
I think I can give it a go. Should I just add that to these set of
patches? or should we create a new patch set?
>
> Luis
>
>
best
--
Joel Granados
On Tue, May 16, 2023 at 04:31:16PM +0200, Joel Granados wrote:
> On Mon, May 15, 2023 at 09:17:39PM -0700, Luis Chamberlain wrote:
> > Awesome!
> >
> > On Mon, May 15, 2023 at 09:14:42AM +0200, Joel Granados wrote:
> > > +
> > > + port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> > > + /*
> > > + * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> > > + * We calculate for the second as that will give us enough for the first.
> > > + */
> > > + tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> > > + tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
> >
> > Any reason why not kzalloc()?
> nope. Will zero it out.
>
> >
> > > + if (!tmp_dir_path) {
> > > + err = -ENOMEM;
> > > + goto exit_free_t;
> > > + }
> > >
> > > - t->port_dir[0].procname = port->name;
> > > + if (tmp_path_len
> > > + <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
> >
> > Since we are clearing up obfuscation code, it would be nicer to
> > make this easier to read and split the snprintf() into one line, capture
> > the error there. And then in a new line do the check. Even if we have to
> > add a new int value here.
> np. Will do something like this:
>
> num_chars_sprinted = snprintf(....
> if(tmp_path_len <= num_chars_sprinted) {
> err = -ENOENT;
> ...
> }
>
> >
> > Other than this I'd just ask to extend the commit log to use
> > the before and after of vmlinux (when this module is compiled in with all
> > the bells and whistles) with ./scripts/bloat-o-meter.
> >
> > Ie build before the patch and cp vmlinux to vmlinux.old and then compare
> > with:
> >
> > ./scripts/bloat-o-meter vmlinux.old vmlinux
> >
> > Can you also describe any testing if any.
> Sure thing. Will add the bloat-o-meter output on the last patch so as to
> gather the results for all the patches.
>
> I'll write some testing info on the patches.
>
>
> >
> > With the above changes, feel free to add to all these patches:
> >
> > Reviewed-by: Luis Chamberlain
> Ack
>
> >
> > > + if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
> >
> > BTW, we should be able to remove now replace register_sysctl_base() with a simple
> > register_sysctl("kernel", foo), and then one for "fs", and one of "vm"
> > on kernel/sysctl.c and just remove:
> >
> > * DECLARE_SYSCTL_BASE() & register_sysctl_base() & __register_sysctl_base()
> > * and then after all this register_sysctl_table() completely
> >
> > Let me know if you'd like a stab at it, or if you prefer me to do that.
> I think I can give it a go. Should I just add that to these set of
> patches? or should we create a new patch set?
I'll send the V2 of this patch set without this. Will add it to the
patch set when I finish if it makes sense. Else I'll just create a new
series.
Best
--
Joel Granados
On Tue, May 16, 2023 at 06:12:21PM +0200, Joel Granados wrote:
> On Tue, May 16, 2023 at 04:31:16PM +0200, Joel Granados wrote:
> > On Mon, May 15, 2023 at 09:17:39PM -0700, Luis Chamberlain wrote:
> > > Awesome!
> > >
> > > On Mon, May 15, 2023 at 09:14:42AM +0200, Joel Granados wrote:
> > > > +
> > > > + port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN);
> > > > + /*
> > > > + * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices.
> > > > + * We calculate for the second as that will give us enough for the first.
> > > > + */
> > > > + tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len;
> > > > + tmp_dir_path = kmalloc(tmp_path_len, GFP_KERNEL);
> > >
> > > Any reason why not kzalloc()?
> > nope. Will zero it out.
> >
> > >
> > > > + if (!tmp_dir_path) {
> > > > + err = -ENOMEM;
> > > > + goto exit_free_t;
> > > > + }
> > > >
> > > > - t->port_dir[0].procname = port->name;
> > > > + if (tmp_path_len
> > > > + <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
> > >
> > > Since we are clearing up obfuscation code, it would be nicer to
> > > make this easier to read and split the snprintf() into one line, capture
> > > the error there. And then in a new line do the check. Even if we have to
> > > add a new int value here.
> > np. Will do something like this:
> >
> > num_chars_sprinted = snprintf(....
> > if(tmp_path_len <= num_chars_sprinted) {
> > err = -ENOENT;
> > ...
> > }
> >
> > >
> > > Other than this I'd just ask to extend the commit log to use
> > > the before and after of vmlinux (when this module is compiled in with all
> > > the bells and whistles) with ./scripts/bloat-o-meter.
> > >
> > > Ie build before the patch and cp vmlinux to vmlinux.old and then compare
> > > with:
> > >
> > > ./scripts/bloat-o-meter vmlinux.old vmlinux
> > >
> > > Can you also describe any testing if any.
> > Sure thing. Will add the bloat-o-meter output on the last patch so as to
> > gather the results for all the patches.
> >
> > I'll write some testing info on the patches.
> >
> >
> > >
> > > With the above changes, feel free to add to all these patches:
> > >
> > > Reviewed-by: Luis Chamberlain
> > Ack
> >
> > >
> > > > + if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
> > >
> > > BTW, we should be able to remove now replace register_sysctl_base() with a simple
> > > register_sysctl("kernel", foo), and then one for "fs", and one of "vm"
> > > on kernel/sysctl.c and just remove:
> > >
> > > * DECLARE_SYSCTL_BASE() & register_sysctl_base() & __register_sysctl_base()
> > > * and then after all this register_sysctl_table() completely
> > >
> > > Let me know if you'd like a stab at it, or if you prefer me to do that.
> > I think I can give it a go. Should I just add that to these set of
> > patches? or should we create a new patch set?
> I'll send the V2 of this patch set without this. Will add it to the
> patch set when I finish if it makes sense. Else I'll just create a new
> series.
New series is good.
Luis