diff mbox series

[v2] sysctl: drop unused argument set_ownership()::table

Message ID 20240223-sysctl-const-ownership-v2-1-f9ba1795aaf2@weissschuh.net (mailing list archive)
State New, archived
Headers show
Series [v2] sysctl: drop unused argument set_ownership()::table | expand

Commit Message

Thomas Weißschuh Feb. 23, 2024, 3:50 p.m. UTC 
The argument is never used and can be removed.

In a future commit the sysctl core will only use
"const struct ctl_table". Removing it here is a preparation for this
consitifcation.

The patch was created with the following coccinelle script:

  @@
  identifier func, head, table, uid, gid;
  @@

  void func(
    struct ctl_table_header *head,
  - struct ctl_table *table,
    kuid_t *uid, kgid_t *gid)
  { ... }

The single changed location was validate through manual inspection and
compilation.

In addition, a search for 'set_ownership' was done over the full tree to
look for places that were missed by coccinelle.
None were found.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v2:
- Rework commit message
- Mention potential conflict with upcoming per-namespace kernel.pid_max
  sysctl
- Delete unused parameter table
- Link to v1: https://lore.kernel.org/r/20231226-sysctl-const-ownership-v1-1-d78fdd744ba1@weissschuh.net
---
The patch is meant to be merged via the sysctl tree.

There is an upcoming series that will introduce a new implementation of
.set_ownership which would need to be adapted [0].
The adaption would be trivial as the 'table' parameter also unused
there.

This change was originally part of the sysctl-const series [1].
To slim down that series and reduce the message load on other
maintainers to a minimumble, submit this patch on its own.

[0] https://lore.kernel.org/lkml/20240222160915.315255-1-aleksandr.mikhalitsyn@canonical.com/
[1] https://lore.kernel.org/lkml/20231204-const-sysctl-v2-2-7a5060b11447@weissschuh.net/
---
 include/linux/sysctl.h | 1 -
 net/sysctl_net.c       | 1 -
 2 files changed, 2 deletions(-)


---
base-commit: ffd2cb6b718e189e7e2d5d0c19c25611f92e061a
change-id: 20231226-sysctl-const-ownership-ff75e67b4eea

Best regards,

Comments

Joel Granados March 15, 2024, 3:41 p.m. UTC | #1 
Hey Thomas

Did you forget to compile? I'm seeing the following error when I
compile:

  ...
  ../fs/proc/proc_sysctl.c: In function ‘proc_sys_make_inode’:
  ../fs/proc/proc_sysctl.c:483:43: error: passing argument 2 of ‘root->set_ownership’ from incompatible pointer type [-Werror=incompatible-pointer-types]
    483 |                 root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
        |                                           ^~~~~
        |                                           |
        |                                           struct ctl_table *
  ../fs/proc/proc_sysctl.c:483:43: note: expected ‘kuid_t *’ but argument is of type ‘struct ctl_table *’
  ../fs/proc/proc_sysctl.c:483:50: error: passing argument 3 of ‘root->set_ownership’ from incompatible pointer type [-Werror=incompatible-pointer-types]
    483 |                 root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
        |                                                  ^~~~~~~~~~~~~
        |                                                  |
        |                                                  kuid_t *
  ../fs/proc/proc_sysctl.c:483:50: note: expected ‘kgid_t *’ but argument is of type ‘kuid_t *’
  ../fs/proc/proc_sysctl.c:483:17: error: too many arguments to function ‘root->set_ownership’
    483 |                 root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
        |                 ^~~~
  cc1: some warnings being treated as errors
  make[5]: *** [../scripts/Makefile.build:243: fs/proc/proc_sysctl.o] Error 1
    CC      fs/xfs/libxfs/xfs_dir2_node.o
  make[5]: *** Waiting for unfinished jobs....
  ...

I'm guessing its just a matter of removing the table arg from
proc_sys_make_inode?

Best

On Fri, Feb 23, 2024 at 04:50:00PM +0100, Thomas Weißschuh wrote:
> The argument is never used and can be removed.
> 
> In a future commit the sysctl core will only use
> "const struct ctl_table". Removing it here is a preparation for this
> consitifcation.
> 
> The patch was created with the following coccinelle script:
> 
>   @@
>   identifier func, head, table, uid, gid;
>   @@
> 
>   void func(
>     struct ctl_table_header *head,
>   - struct ctl_table *table,
>     kuid_t *uid, kgid_t *gid)
>   { ... }
> 
> The single changed location was validate through manual inspection and
> compilation.
> 
> In addition, a search for 'set_ownership' was done over the full tree to
> look for places that were missed by coccinelle.
> None were found.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> Changes in v2:
> - Rework commit message
> - Mention potential conflict with upcoming per-namespace kernel.pid_max
>   sysctl
> - Delete unused parameter table
> - Link to v1: https://lore.kernel.org/r/20231226-sysctl-const-ownership-v1-1-d78fdd744ba1@weissschuh.net
> ---
> The patch is meant to be merged via the sysctl tree.
> 
> There is an upcoming series that will introduce a new implementation of
> .set_ownership which would need to be adapted [0].
> The adaption would be trivial as the 'table' parameter also unused
> there.
> 
> This change was originally part of the sysctl-const series [1].
> To slim down that series and reduce the message load on other
> maintainers to a minimumble, submit this patch on its own.
> 
> [0] https://lore.kernel.org/lkml/20240222160915.315255-1-aleksandr.mikhalitsyn@canonical.com/
> [1] https://lore.kernel.org/lkml/20231204-const-sysctl-v2-2-7a5060b11447@weissschuh.net/
> ---
>  include/linux/sysctl.h | 1 -
>  net/sysctl_net.c       | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index ee7d33b89e9e..60333a6b9370 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -205,7 +205,6 @@ struct ctl_table_root {
>  	struct ctl_table_set default_set;
>  	struct ctl_table_set *(*lookup)(struct ctl_table_root *root);
>  	void (*set_ownership)(struct ctl_table_header *head,
> -			      struct ctl_table *table,
>  			      kuid_t *uid, kgid_t *gid);
>  	int (*permissions)(struct ctl_table_header *head, struct ctl_table *table);
>  };
> diff --git a/net/sysctl_net.c b/net/sysctl_net.c
> index 051ed5f6fc93..a0a7a79991f9 100644
> --- a/net/sysctl_net.c
> +++ b/net/sysctl_net.c
> @@ -54,7 +54,6 @@ static int net_ctl_permissions(struct ctl_table_header *head,
>  }
>  
>  static void net_ctl_set_ownership(struct ctl_table_header *head,
> -				  struct ctl_table *table,
>  				  kuid_t *uid, kgid_t *gid)
>  {
>  	struct net *net = container_of(head->set, struct net, sysctls);
> 
> ---
> base-commit: ffd2cb6b718e189e7e2d5d0c19c25611f92e061a
> change-id: 20231226-sysctl-const-ownership-ff75e67b4eea
> 
> Best regards,
> -- 
> Thomas Weißschuh <linux@weissschuh.net>
>
Thomas Weißschuh March 15, 2024, 3:56 p.m. UTC | #2 
On 2024-03-15 16:41:34+0100, Joel Granados wrote:
> Hey Thomas
> 
> Did you forget to compile? I'm seeing the following error when I
> compile:

Welp...

>   ...
>   ../fs/proc/proc_sysctl.c: In function ‘proc_sys_make_inode’:
>   ../fs/proc/proc_sysctl.c:483:43: error: passing argument 2 of ‘root->set_ownership’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>     483 |                 root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
>         |                                           ^~~~~
>         |                                           |
>         |                                           struct ctl_table *
>   ../fs/proc/proc_sysctl.c:483:43: note: expected ‘kuid_t *’ but argument is of type ‘struct ctl_table *’
>   ../fs/proc/proc_sysctl.c:483:50: error: passing argument 3 of ‘root->set_ownership’ from incompatible pointer type [-Werror=incompatible-pointer-types]
>     483 |                 root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
>         |                                                  ^~~~~~~~~~~~~
>         |                                                  |
>         |                                                  kuid_t *
>   ../fs/proc/proc_sysctl.c:483:50: note: expected ‘kgid_t *’ but argument is of type ‘kuid_t *’
>   ../fs/proc/proc_sysctl.c:483:17: error: too many arguments to function ‘root->set_ownership’
>     483 |                 root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
>         |                 ^~~~
>   cc1: some warnings being treated as errors
>   make[5]: *** [../scripts/Makefile.build:243: fs/proc/proc_sysctl.o] Error 1
>     CC      fs/xfs/libxfs/xfs_dir2_node.o
>   make[5]: *** Waiting for unfinished jobs....
>   ...
> 
> I'm guessing its just a matter of removing the table arg from
> proc_sys_make_inode?

Yes, for this error it's correct.

There are also new implementors of the set_ownership callback in ipc/
which need to be adapted.

I'll send a new revision.

Thomas
Joel Granados March 15, 2024, 4:19 p.m. UTC | #3 
On Fri, Mar 15, 2024 at 04:56:54PM +0100, Thomas Weißschuh wrote:
> On 2024-03-15 16:41:34+0100, Joel Granados wrote:
> > Hey Thomas
> > 
> > Did you forget to compile? I'm seeing the following error when I
> > compile:
> 
> Welp...
> 
> >   ...
> >   ../fs/proc/proc_sysctl.c: In function ‘proc_sys_make_inode’:
> >   ../fs/proc/proc_sysctl.c:483:43: error: passing argument 2 of ‘root->set_ownership’ from incompatible pointer type [-Werror=incompatible-pointer-types]
> >     483 |                 root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
> >         |                                           ^~~~~
> >         |                                           |
> >         |                                           struct ctl_table *
> >   ../fs/proc/proc_sysctl.c:483:43: note: expected ‘kuid_t *’ but argument is of type ‘struct ctl_table *’
> >   ../fs/proc/proc_sysctl.c:483:50: error: passing argument 3 of ‘root->set_ownership’ from incompatible pointer type [-Werror=incompatible-pointer-types]
> >     483 |                 root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
> >         |                                                  ^~~~~~~~~~~~~
> >         |                                                  |
> >         |                                                  kuid_t *
> >   ../fs/proc/proc_sysctl.c:483:50: note: expected ‘kgid_t *’ but argument is of type ‘kuid_t *’
> >   ../fs/proc/proc_sysctl.c:483:17: error: too many arguments to function ‘root->set_ownership’
> >     483 |                 root->set_ownership(head, table, &inode->i_uid, &inode->i_gid);
> >         |                 ^~~~
> >   cc1: some warnings being treated as errors
> >   make[5]: *** [../scripts/Makefile.build:243: fs/proc/proc_sysctl.o] Error 1
> >     CC      fs/xfs/libxfs/xfs_dir2_node.o
> >   make[5]: *** Waiting for unfinished jobs....
> >   ...
> > 
> > I'm guessing its just a matter of removing the table arg from
> > proc_sys_make_inode?
> 
> Yes, for this error it's correct.
> 
> There are also new implementors of the set_ownership callback in ipc/
> which need to be adapted.
> 
> I'll send a new revision.
Thx. Not sure what your using as a base, but I suggest you use
linux-next so you get al the stuff going into the rc.

best
diff mbox series

Patch

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index ee7d33b89e9e..60333a6b9370 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -205,7 +205,6 @@  struct ctl_table_root {
 	struct ctl_table_set default_set;
 	struct ctl_table_set *(*lookup)(struct ctl_table_root *root);
 	void (*set_ownership)(struct ctl_table_header *head,
-			      struct ctl_table *table,
 			      kuid_t *uid, kgid_t *gid);
 	int (*permissions)(struct ctl_table_header *head, struct ctl_table *table);
 };
diff --git a/net/sysctl_net.c b/net/sysctl_net.c
index 051ed5f6fc93..a0a7a79991f9 100644
--- a/net/sysctl_net.c
+++ b/net/sysctl_net.c
@@ -54,7 +54,6 @@  static int net_ctl_permissions(struct ctl_table_header *head,
 }
 
 static void net_ctl_set_ownership(struct ctl_table_header *head,
-				  struct ctl_table *table,
 				  kuid_t *uid, kgid_t *gid)
 {
 	struct net *net = container_of(head->set, struct net, sysctls);