netdev - Re: [PATCH v2] sysctl: drop unused argument set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240315154134.5qfq6wucxid5kfmc@joelS2.panther.com>
Date: Fri, 15 Mar 2024 16:41:34 +0100
From: Joel Granados <j.granados@...sung.com>
To: Thomas Weißschuh <linux@...ssschuh.net>
CC: Luis Chamberlain <mcgrof@...nel.org>, Kees Cook <keescook@...omium.org>,
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	<linux-kernel@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH v2] sysctl: drop unused argument set_ownership()::table

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@...ssschuh.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@...ssschuh.net>
> 

-- 

Joel Granados

Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)