Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 3 Mar 2023 15:07:01 -0800 | From | Luis Chamberlain <> | Subject | Re: usermode-helper code oddity query.. |
| |
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
| ![\](/images/icornerr.gif) |