[v4,1/4] sysctl: Add register_sysctl_init() interface

Commit Message

Xiaoming Ni May 19, 2020, 3:31 a.m. UTC

In order to eliminate the duplicate code for registering the sysctl
interface during the initialization of each feature, add the
register_sysctl_init() interface

Signed-off-by: Xiaoming Ni <[e-mail]>
Reviewed-by: Kees Cook <[e-mail]>
---
 include/linux/sysctl.h |  2 ++
 kernel/sysctl.c        | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+)

Comments

Luis Chamberlain May 29, 2020, 7:09 a.m. UTC | #1

On Tue, May 19, 2020 at 11:31:08AM +0800, Xiaoming Ni wrote:
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -3358,6 +3358,25 @@ int __init sysctl_init(void)
>  	kmemleak_not_leak(hdr);
>  	return 0;
>  }
> +
> +/*
> + * The sysctl interface is used to modify the interface value,
> + * but the feature interface has default values. Even if register_sysctl fails,
> + * the feature body function can also run. At the same time, malloc small
> + * fragment of memory during the system initialization phase, almost does
> + * not fail. Therefore, the function return is designed as void
> + */

Let's use kdoc while at it. Can you convert this to proper kdoc?

> +void __init register_sysctl_init(const char *path, struct ctl_table *table,
> +				 const char *table_name)
> +{
> +	struct ctl_table_header *hdr = register_sysctl(path, table);
> +
> +	if (unlikely(!hdr)) {
> +		pr_err("failed when register_sysctl %s to %s\n", table_name, path);
> +		return;

table_name is only used for this, however we can easily just make
another _register_sysctl_init() helper first, and then use a macro
which will concatenate this to something useful if you want to print
a string. I see no point in the description for this, specially since
the way it was used was not to be descriptive, but instead just a name
followed by some underscore and something else.

> +	}
> +	kmemleak_not_leak(hdr);

Is it *wrong* to run kmemleak_not_leak() when hdr was not allocated?
If so, can you fix the sysctl __init call itself?

PS. Since you have given me your series, feel free to send me a patch
as a follow up to this in privae and I can integrate it into my tree.

  Luis

Xiaoming Ni May 29, 2020, 7:27 a.m. UTC | #2

On 2020/5/29 15:09, Luis Chamberlain wrote:
> On Tue, May 19, 2020 at 11:31:08AM +0800, Xiaoming Ni wrote:
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -3358,6 +3358,25 @@ int __init sysctl_init(void)
>>   	kmemleak_not_leak(hdr);
>>   	return 0;
>>   }
>> +
>> +/*
>> + * The sysctl interface is used to modify the interface value,
>> + * but the feature interface has default values. Even if register_sysctl fails,
>> + * the feature body function can also run. At the same time, malloc small
>> + * fragment of memory during the system initialization phase, almost does
>> + * not fail. Therefore, the function return is designed as void
>> + */
> 
> Let's use kdoc while at it. Can you convert this to proper kdoc?
> 
Sorry, I do n’t know the format requirements of Kdoc, can you give me 
some tips for writing?


>> +void __init register_sysctl_init(const char *path, struct ctl_table *table,
>> +				 const char *table_name)
>> +{
>> +	struct ctl_table_header *hdr = register_sysctl(path, table);
>> +
>> +	if (unlikely(!hdr)) {
>> +		pr_err("failed when register_sysctl %s to %s\n", table_name, path);
>> +		return;
> 
> table_name is only used for this, however we can easily just make
> another _register_sysctl_init() helper first, and then use a macro
> which will concatenate this to something useful if you want to print
> a string. I see no point in the description for this, specially since
> the way it was used was not to be descriptive, but instead just a name
> followed by some underscore and something else.
> 
Good idea, I will fix and send the patch to you as soon as possible

>> +	}
>> +	kmemleak_not_leak(hdr);
> 
> Is it *wrong* to run kmemleak_not_leak() when hdr was not allocated?
> If so, can you fix the sysctl __init call itself?
I don't understand here, do you mean that register_sysctl_init () does 
not need to call kmemleak_not_leak (hdr), or does it mean to add check 
hdr before calling kmemleak_not_leak (hdr) in sysctl_init ()?


> PS. Since you have given me your series, feel free to send me a patch
> as a follow up to this in privae and I can integrate it into my tree.
> 
>    Luis
> 

Thanks
Xiaoming Ni

Luis Chamberlain May 29, 2020, 7:36 a.m. UTC | #3

On Fri, May 29, 2020 at 03:27:22PM +0800, Xiaoming Ni wrote:
> On 2020/5/29 15:09, Luis Chamberlain wrote:
> > On Tue, May 19, 2020 at 11:31:08AM +0800, Xiaoming Ni wrote:
> > > --- a/kernel/sysctl.c
> > > +++ b/kernel/sysctl.c
> > > @@ -3358,6 +3358,25 @@ int __init sysctl_init(void)
> > >   	kmemleak_not_leak(hdr);
> > >   	return 0;
> > >   }
> > > +
> > > +/*
> > > + * The sysctl interface is used to modify the interface value,
> > > + * but the feature interface has default values. Even if register_sysctl fails,
> > > + * the feature body function can also run. At the same time, malloc small
> > > + * fragment of memory during the system initialization phase, almost does
> > > + * not fail. Therefore, the function return is designed as void
> > > + */
> > 
> > Let's use kdoc while at it. Can you convert this to proper kdoc?
> > 
> Sorry, I do n’t know the format requirements of Kdoc, can you give me some
> tips for writing?

Sure, include/net/mac80211.h is a good example.

> > > +void __init register_sysctl_init(const char *path, struct ctl_table *table,
> > > +				 const char *table_name)
> > > +{
> > > +	struct ctl_table_header *hdr = register_sysctl(path, table);
> > > +
> > > +	if (unlikely(!hdr)) {
> > > +		pr_err("failed when register_sysctl %s to %s\n", table_name, path);
> > > +		return;
> > 
> > table_name is only used for this, however we can easily just make
> > another _register_sysctl_init() helper first, and then use a macro
> > which will concatenate this to something useful if you want to print
> > a string. I see no point in the description for this, specially since
> > the way it was used was not to be descriptive, but instead just a name
> > followed by some underscore and something else.
> > 
> Good idea, I will fix and send the patch to you as soon as possible

No rush :)

> > > +	}
> > > +	kmemleak_not_leak(hdr);
> > 
> > Is it *wrong* to run kmemleak_not_leak() when hdr was not allocated?
> > If so, can you fix the sysctl __init call itself?
> I don't understand here, do you mean that register_sysctl_init () does not
> need to call kmemleak_not_leak (hdr), or does it mean to add check hdr
> before calling kmemleak_not_leak (hdr) in sysctl_init ()?

I'm asking that the way you are adding it, you don't run
kmemleak_not_leak(hdr) if the hdr allocation filed. If that is
right then it seems that sysctl_init() might not be doing it
right.

Can that code be shared somehow?

  Luis

Xiaoming Ni May 29, 2020, 8:33 a.m. UTC | #4

On 2020/5/29 15:36, Luis Chamberlain wrote:
> On Fri, May 29, 2020 at 03:27:22PM +0800, Xiaoming Ni wrote:
>> On 2020/5/29 15:09, Luis Chamberlain wrote:
>>> On Tue, May 19, 2020 at 11:31:08AM +0800, Xiaoming Ni wrote:
>>>> --- a/kernel/sysctl.c
>>>> +++ b/kernel/sysctl.c
>>>> @@ -3358,6 +3358,25 @@ int __init sysctl_init(void)
>>>>    	kmemleak_not_leak(hdr);
>>>>    	return 0;
>>>>    }
>>>> +
>>>> +/*
>>>> + * The sysctl interface is used to modify the interface value,
>>>> + * but the feature interface has default values. Even if register_sysctl fails,
>>>> + * the feature body function can also run. At the same time, malloc small
>>>> + * fragment of memory during the system initialization phase, almost does
>>>> + * not fail. Therefore, the function return is designed as void
>>>> + */
>>>
>>> Let's use kdoc while at it. Can you convert this to proper kdoc?
>>>
>> Sorry, I do n’t know the format requirements of Kdoc, can you give me some
>> tips for writing?
> 
> Sure, include/net/mac80211.h is a good example.
> 
>>>> +void __init register_sysctl_init(const char *path, struct ctl_table *table,
>>>> +				 const char *table_name)
>>>> +{
>>>> +	struct ctl_table_header *hdr = register_sysctl(path, table);
>>>> +
>>>> +	if (unlikely(!hdr)) {
>>>> +		pr_err("failed when register_sysctl %s to %s\n", table_name, path);
>>>> +		return;
>>>
>>> table_name is only used for this, however we can easily just make
>>> another _register_sysctl_init() helper first, and then use a macro
>>> which will concatenate this to something useful if you want to print
>>> a string. I see no point in the description for this, specially since
>>> the way it was used was not to be descriptive, but instead just a name
>>> followed by some underscore and something else.
>>>
>> Good idea, I will fix and send the patch to you as soon as possible
> 
> No rush :)
> 
>>>> +	}
>>>> +	kmemleak_not_leak(hdr);
>>>
>>> Is it *wrong* to run kmemleak_not_leak() when hdr was not allocated?
>>> If so, can you fix the sysctl __init call itself?
>> I don't understand here, do you mean that register_sysctl_init () does not
>> need to call kmemleak_not_leak (hdr), or does it mean to add check hdr
>> before calling kmemleak_not_leak (hdr) in sysctl_init ()?
> 
> I'm asking that the way you are adding it, you don't run
> kmemleak_not_leak(hdr) if the hdr allocation filed. If that is
> right then it seems that sysctl_init() might not be doing it
> right.
> 
> Can that code be shared somehow?
> 
>    Luis

void __ref kmemleak_not_leak(const void *ptr)
{
	pr_debug("%s(0x%p)\n", __func__, ptr);

	if (kmemleak_enabled && ptr && !IS_ERR(ptr))
		make_gray_object((unsigned long)ptr);
}
EXPORT_SYMBOL(kmemleak_not_leak);

In the code of kmemleak_not_leak(), it is verified that the pointer is 
valid, so kmemleak_not_leak (NULL) will not be a problem.
At the same time, there is no need to call kmemleak_not_leak() in the 
failed branch of register_sysctl_init().

Thanks
Xiaoming Ni

Luis Chamberlain May 29, 2020, 11:31 a.m. UTC | #5

On Fri, May 29, 2020 at 04:33:01PM +0800, Xiaoming Ni wrote:
> On 2020/5/29 15:36, Luis Chamberlain wrote:
> > On Fri, May 29, 2020 at 03:27:22PM +0800, Xiaoming Ni wrote:
> > > On 2020/5/29 15:09, Luis Chamberlain wrote:
> > > > On Tue, May 19, 2020 at 11:31:08AM +0800, Xiaoming Ni wrote:
> > > > > --- a/kernel/sysctl.c
> > > > > +++ b/kernel/sysctl.c
> > > > > @@ -3358,6 +3358,25 @@ int __init sysctl_init(void)
> > > > >    	kmemleak_not_leak(hdr);
> > > > >    	return 0;
> > > > >    }
> > > > > +
> > > > > +/*
> > > > > + * The sysctl interface is used to modify the interface value,
> > > > > + * but the feature interface has default values. Even if register_sysctl fails,
> > > > > + * the feature body function can also run. At the same time, malloc small
> > > > > + * fragment of memory during the system initialization phase, almost does
> > > > > + * not fail. Therefore, the function return is designed as void
> > > > > + */
> > > > 
> > > > Let's use kdoc while at it. Can you convert this to proper kdoc?
> > > > 
> > > Sorry, I do n’t know the format requirements of Kdoc, can you give me some
> > > tips for writing?
> > 
> > Sure, include/net/mac80211.h is a good example.
> > 
> > > > > +void __init register_sysctl_init(const char *path, struct ctl_table *table,
> > > > > +				 const char *table_name)
> > > > > +{
> > > > > +	struct ctl_table_header *hdr = register_sysctl(path, table);
> > > > > +
> > > > > +	if (unlikely(!hdr)) {
> > > > > +		pr_err("failed when register_sysctl %s to %s\n", table_name, path);
> > > > > +		return;
> > > > 
> > > > table_name is only used for this, however we can easily just make
> > > > another _register_sysctl_init() helper first, and then use a macro
> > > > which will concatenate this to something useful if you want to print
> > > > a string. I see no point in the description for this, specially since
> > > > the way it was used was not to be descriptive, but instead just a name
> > > > followed by some underscore and something else.
> > > > 
> > > Good idea, I will fix and send the patch to you as soon as possible
> > 
> > No rush :)
> > 
> > > > > +	}
> > > > > +	kmemleak_not_leak(hdr);
> > > > 
> > > > Is it *wrong* to run kmemleak_not_leak() when hdr was not allocated?
> > > > If so, can you fix the sysctl __init call itself?
> > > I don't understand here, do you mean that register_sysctl_init () does not
> > > need to call kmemleak_not_leak (hdr), or does it mean to add check hdr
> > > before calling kmemleak_not_leak (hdr) in sysctl_init ()?
> > 
> > I'm asking that the way you are adding it, you don't run
> > kmemleak_not_leak(hdr) if the hdr allocation filed. If that is
> > right then it seems that sysctl_init() might not be doing it
> > right.
> > 
> > Can that code be shared somehow?
> > 
> >    Luis
> 
> void __ref kmemleak_not_leak(const void *ptr)
> {
> 	pr_debug("%s(0x%p)\n", __func__, ptr);
> 
> 	if (kmemleak_enabled && ptr && !IS_ERR(ptr))
> 		make_gray_object((unsigned long)ptr);
> }
> EXPORT_SYMBOL(kmemleak_not_leak);
> 
> In the code of kmemleak_not_leak(), it is verified that the pointer is
> valid, so kmemleak_not_leak (NULL) will not be a problem.
> At the same time, there is no need to call kmemleak_not_leak() in the failed
> branch of register_sysctl_init().

Thanks for the confirmation.

   Luis

Patch

diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 50bb7f3..857ba93 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -197,6 +197,8 @@  struct ctl_table_header *register_sysctl_paths(const struct ctl_path *path,
 void unregister_sysctl_table(struct ctl_table_header * table);
 
 extern int sysctl_init(void);
+extern void register_sysctl_init(const char *path, struct ctl_table *table,
+				 const char *table_name);
 void do_sysctl_args(void);
 
 extern int pwrsw_enabled;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index cc1fcba..8afd713 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -3358,6 +3358,25 @@  int __init sysctl_init(void)
 	kmemleak_not_leak(hdr);
 	return 0;
 }
+
+/*
+ * The sysctl interface is used to modify the interface value,
+ * but the feature interface has default values. Even if register_sysctl fails,
+ * the feature body function can also run. At the same time, malloc small
+ * fragment of memory during the system initialization phase, almost does
+ * not fail. Therefore, the function return is designed as void
+ */
+void __init register_sysctl_init(const char *path, struct ctl_table *table,
+				 const char *table_name)
+{
+	struct ctl_table_header *hdr = register_sysctl(path, table);
+
+	if (unlikely(!hdr)) {
+		pr_err("failed when register_sysctl %s to %s\n", table_name, path);
+		return;
+	}
+	kmemleak_not_leak(hdr);
+}
 #endif /* CONFIG_SYSCTL */
 /*
  * No sense putting this after each symbol definition, twice,