usermode-helper code oddity query..

Message ID CAHk-=whjmfvccPgFSfbpZ4nQ6fkYwTEAZhqZvfW8=rKMDsZarQ@mail.gmail.com
State New
Headers
Series usermode-helper code oddity query.. |

Commit Message

Linus Torvalds March 2, 2023, 11:44 p.m. UTC
  So this is a bit out of the blue, but I cleaned up some really old
legacy capability code in commit f122a08b197d ("capability: just use a
'u64' instead of a 'u32[2]' array") and in the process I became the
last person to touch kernel/umh.c.

Tag, I'm clearly it. Not that I want to take that glory away from
PeterZ, who was the previous last person to touch that code. In fact,
I'm just cc'ing everybody who has been touching that file at all in
the last years, and a few /proc sysctl maintainers too.

Anyway, I wanted to try to keep the capability code cleanups clear,
and really only touched the data structure conversion, but I'm just
left staring at that code and wondering why we have those odd CAP_BSET
/ CAP_PI dummy pointers. They've been there since the whole /proc
interface was introduced, but they seem strangely pointless.

It would _seem_ like it would be a lot simpler and more
straightforward to just put the actual pointer to the usermodehelper
capability in there instead, and not have that odd fake pointer
enumeration at all.

IOW, I'm wondering what's wrong with a patch like the attached. I
might be missing something.

I also would have like that array to be an array of "u32" rather than
"unsigned long" (because that is, sadly, the interface we have, like
it or not), but we don't seem to have a proc_dou32vec_minmax(). I
guess "uint" is the same thing, but it's not pretty. Anyway, that's a
separate and independent issue from this.

And no, none of this is important. Just random cleanup of code I
happened to look at for other reasons.

           Linus
  

Comments

Luis Chamberlain March 3, 2023, 11:07 p.m. UTC | #1
On Thu, Mar 02, 2023 at 03:44:17PM -0800, Linus Torvalds wrote:
> So this is a bit out of the blue, but I cleaned up some really old
> legacy capability code in commit f122a08b197d ("capability: just use a
> 'u64' instead of a 'u32[2]' array") and in the process I became the
> last person to touch kernel/umh.c.
> 
> Tag, I'm clearly it. Not that I want to take that glory away from
> PeterZ, who was the previous last person to touch that code. In fact,
> I'm just cc'ing everybody who has been touching that file at all in
> the last years, and a few /proc sysctl maintainers too.
> 
> Anyway, I wanted to try to keep the capability code cleanups clear,
> and really only touched the data structure conversion, but I'm just
> left staring at that code and wondering why we have those odd CAP_BSET
> / CAP_PI dummy pointers. They've been there since the whole /proc
> interface was introduced, but they seem strangely pointless.

Actually that seems to have come from Eric Paris on v3.0 via commit 17f60a7da150f
("capabilites: allow the application of capability limits to usermode helpers")

mcgrof@fulton ~/linux (git::master)$ git describe --contains 17f60a7da150f
v3.0-rc1~309^2~1^2~12

> It would _seem_ like it would be a lot simpler and more
> straightforward to just put the actual pointer to the usermodehelper
> capability in there instead, and not have that odd fake pointer
> enumeration at all.

Agreed.

> IOW, I'm wondering what's wrong with a patch like the attached. I
> might be missing something.

Yes, the only thing I think think of is that at first it just seemed
like a good way to abstract access to usage of the same routine for
two separate variables. I can't really see *why* its done that way
though.

The only thing I can think of is perhaps it was a sort of defensive
mechanism back from the days we had tons of sysctls on kernel/sysctl.c
large kitchen sink to prevent someone from thinking they could use
proc_cap_handler() for other variables. That file used to be hell.

> I also would have like that array to be an array of "u32" rather than
> "unsigned long" (because that is, sadly, the interface we have, like
> it or not), but we don't seem to have a proc_dou32vec_minmax(). I
> guess "uint" is the same thing, but it's not pretty. Anyway, that's a
> separate and independent issue from this.
> 
> And no, none of this is important. Just random cleanup of code I
> happened to look at for other reasons.
> 
>            Linus

>  kernel/umh.c | 18 +++++-------------
>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> diff --git a/kernel/umh.c b/kernel/umh.c
> index 2a4708277335..60aa9e764a38 100644
> --- a/kernel/umh.c
> +++ b/kernel/umh.c
> @@ -32,9 +32,6 @@
>  
>  #include <trace/events/module.h>
>  
> -#define CAP_BSET	(void *)1
> -#define CAP_PI		(void *)2
> -
>  static kernel_cap_t usermodehelper_bset = CAP_FULL_SET;
>  static kernel_cap_t usermodehelper_inheritable = CAP_FULL_SET;
>  static DEFINE_SPINLOCK(umh_sysctl_lock);
> @@ -512,16 +509,11 @@ static int proc_cap_handler(struct ctl_table *table, int write,
>  	/*
>  	 * convert from the global kernel_cap_t to the ulong array to print to
>  	 * userspace if this is a read.
> +	 *
> +	 * Legacy format: capabilities are exposed as two 32-bit values
>  	 */
> +	cap = table->data;
>  	spin_lock(&umh_sysctl_lock);
> -	if (table->data == CAP_BSET)
> -		cap = &usermodehelper_bset;
> -	else if (table->data == CAP_PI)
> -		cap = &usermodehelper_inheritable;
> -	else
> -		BUG();
> -
> -	/* Legacy format: capabilities are exposed as two 32-bit values */
>  	cap_array[0] = (u32) cap->val;
>  	cap_array[1] = cap->val >> 32;
>  	spin_unlock(&umh_sysctl_lock);
> @@ -555,14 +547,14 @@ static int proc_cap_handler(struct ctl_table *table, int write,
>  struct ctl_table usermodehelper_table[] = {
>  	{
>  		.procname	= "bset",
> -		.data		= CAP_BSET,
> +		.data		= &usermodehelper_bset,
>  		.maxlen		= 2 * sizeof(unsigned long),
>  		.mode		= 0600,
>  		.proc_handler	= proc_cap_handler,
>  	},
>  	{
>  		.procname	= "inheritable",
> -		.data		= CAP_PI,
> +		.data		= &usermodehelper_inheritable,
>  		.maxlen		= 2 * sizeof(unsigned long),
>  		.mode		= 0600,
>  		.proc_handler	= proc_cap_handler,

Feel free to add:

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis
  

Patch

 kernel/umh.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/kernel/umh.c b/kernel/umh.c
index 2a4708277335..60aa9e764a38 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -32,9 +32,6 @@ 
 
 #include <trace/events/module.h>
 
-#define CAP_BSET	(void *)1
-#define CAP_PI		(void *)2
-
 static kernel_cap_t usermodehelper_bset = CAP_FULL_SET;
 static kernel_cap_t usermodehelper_inheritable = CAP_FULL_SET;
 static DEFINE_SPINLOCK(umh_sysctl_lock);
@@ -512,16 +509,11 @@  static int proc_cap_handler(struct ctl_table *table, int write,
 	/*
 	 * convert from the global kernel_cap_t to the ulong array to print to
 	 * userspace if this is a read.
+	 *
+	 * Legacy format: capabilities are exposed as two 32-bit values
 	 */
+	cap = table->data;
 	spin_lock(&umh_sysctl_lock);
-	if (table->data == CAP_BSET)
-		cap = &usermodehelper_bset;
-	else if (table->data == CAP_PI)
-		cap = &usermodehelper_inheritable;
-	else
-		BUG();
-
-	/* Legacy format: capabilities are exposed as two 32-bit values */
 	cap_array[0] = (u32) cap->val;
 	cap_array[1] = cap->val >> 32;
 	spin_unlock(&umh_sysctl_lock);
@@ -555,14 +547,14 @@  static int proc_cap_handler(struct ctl_table *table, int write,
 struct ctl_table usermodehelper_table[] = {
 	{
 		.procname	= "bset",
-		.data		= CAP_BSET,
+		.data		= &usermodehelper_bset,
 		.maxlen		= 2 * sizeof(unsigned long),
 		.mode		= 0600,
 		.proc_handler	= proc_cap_handler,
 	},
 	{
 		.procname	= "inheritable",
-		.data		= CAP_PI,
+		.data		= &usermodehelper_inheritable,
 		.maxlen		= 2 * sizeof(unsigned long),
 		.mode		= 0600,
 		.proc_handler	= proc_cap_handler,