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 possibleNo 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
