The domain table should be empty at module unload. If it isn't there is
a bug somewhere. So check and report.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206651
Cc: [email protected]
Signed-off-by: NeilBrown <[email protected]>
---
net/sunrpc/sunrpc.h | 1 +
net/sunrpc/sunrpc_syms.c | 2 ++
net/sunrpc/svcauth.c | 18 ++++++++++++++++++
3 files changed, 21 insertions(+)
diff --git a/net/sunrpc/sunrpc.h b/net/sunrpc/sunrpc.h
index 47a756503d11..f6fe2e6cd65a 100644
--- a/net/sunrpc/sunrpc.h
+++ b/net/sunrpc/sunrpc.h
@@ -52,4 +52,5 @@ static inline int sock_is_loopback(struct sock *sk)
int rpc_clients_notifier_register(void);
void rpc_clients_notifier_unregister(void);
+void auth_domain_cleanup(void);
#endif /* _NET_SUNRPC_SUNRPC_H */
diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
index f9edaa9174a4..236fadc4a439 100644
--- a/net/sunrpc/sunrpc_syms.c
+++ b/net/sunrpc/sunrpc_syms.c
@@ -23,6 +23,7 @@
#include <linux/sunrpc/rpc_pipe_fs.h>
#include <linux/sunrpc/xprtsock.h>
+#include "sunrpc.h"
#include "netns.h"
unsigned int sunrpc_net_id;
@@ -131,6 +132,7 @@ cleanup_sunrpc(void)
unregister_rpc_pipefs();
rpc_destroy_mempool();
unregister_pernet_subsys(&sunrpc_net_ops);
+ auth_domain_cleanup();
#if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
rpc_unregister_sysctl();
#endif
diff --git a/net/sunrpc/svcauth.c b/net/sunrpc/svcauth.c
index 552617e3467b..477890e8b9d8 100644
--- a/net/sunrpc/svcauth.c
+++ b/net/sunrpc/svcauth.c
@@ -205,3 +205,21 @@ struct auth_domain *auth_domain_find(char *name)
return NULL;
}
EXPORT_SYMBOL_GPL(auth_domain_find);
+
+void auth_domain_cleanup(void)
+{
+ /* There should be no auth_domains left at module unload */
+ int h;
+ bool found = false;
+
+ for (h = 0; h < DN_HASHMAX; h++) {
+ struct auth_domain *hp;
+
+ hlist_for_each_entry(hp, auth_domain_table+h, hash) {
+ found = true;
+ printk(KERN_WARNING "sunrpc: domain %s still present at module unload.\n",
+ hp->name);
+ }
+ }
+ WARN(found, "sunrpc: auth_domain_table not clean -> memory leak\n");
+}
Hi NeilBrown,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on nfsd/nfsd-next]
[also build test WARNING on nfs/linux-next cel/for-next v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/NeilBrown/SUNRPC-svc-fix-gss-flavour-registration-problems/20200521-112443
base: git://linux-nfs.org/~bfields/linux.git nfsd-next
config: um-allmodconfig (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build):
# save the attached .config to linux build tree
make ARCH=um
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
cc1: warning: arch/um/include/uapi: No such file or directory [-Wmissing-include-dirs]
In file included from include/linux/uaccess.h:11:0,
from include/linux/sched/task.h:11,
from include/linux/sched/signal.h:9,
from include/linux/sunrpc/types.h:14,
from net/sunrpc/svcauth.c:15:
arch/um/include/asm/uaccess.h: In function '__access_ok':
arch/um/include/asm/uaccess.h:17:29: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
(((unsigned long) (addr) >= FIXADDR_USER_START) && ^
arch/um/include/asm/uaccess.h:45:3: note: in expansion of macro '__access_ok_vsyscall'
__access_ok_vsyscall(addr, size) ||
^~~~~~~~~~~~~~~~~~~~
In file included from include/linux/kernel.h:11:0,
from include/linux/list.h:9,
from include/linux/module.h:12,
from net/sunrpc/svcauth.c:14:
include/asm-generic/fixmap.h: In function 'fix_to_virt':
include/asm-generic/fixmap.h:32:19: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
^
include/linux/compiler.h:330:9: note: in definition of macro '__compiletime_assert'
if (!(condition)) ^~~~~~~~~
include/linux/compiler.h:350:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:50:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
^~~~~~~~~~~~~~~~
include/asm-generic/fixmap.h:32:2: note: in expansion of macro 'BUILD_BUG_ON'
BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
^~~~~~~~~~~~
net/sunrpc/svcauth.c: At top level:
>> net/sunrpc/svcauth.c:209:6: warning: no previous prototype for 'auth_domain_cleanup' [-Wmissing-prototypes]
void auth_domain_cleanup(void)
^~~~~~~~~~~~~~~~~~~
vim +/auth_domain_cleanup +209 net/sunrpc/svcauth.c
208
> 209 void auth_domain_cleanup(void)
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi NeilBrown,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on nfsd/nfsd-next]
[also build test WARNING on nfs/linux-next v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/NeilBrown/SUNRPC-svc-fix-gss-flavour-registration-problems/20200521-112443
base: git://linux-nfs.org/~bfields/linux.git nfsd-next
config: arm-randconfig-r035-20200521 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 3393cc4cebf9969db94dc424b7a2b6195589c33b)
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> net/sunrpc/svcauth.c:209:6: warning: no previous prototype for function 'auth_domain_cleanup' [-Wmissing-prototypes]
void auth_domain_cleanup(void)
^
net/sunrpc/svcauth.c:209:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
void auth_domain_cleanup(void)
^
static
1 warning generated.
vim +/auth_domain_cleanup +209 net/sunrpc/svcauth.c
208
> 209 void auth_domain_cleanup(void)
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Neil!
Thanks for the patches. Seems to me like a good fix overall.
Judging by the syzbot e-mail, you might be posting a refresh of this
patch series, so I proffer a few minor review comments below.
> On May 20, 2020, at 11:21 PM, NeilBrown <[email protected]> wrote:
>
> The domain table should be empty at module unload. If it isn't there is
> a bug somewhere. So check and report.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206651
> Cc: [email protected]
> Signed-off-by: NeilBrown <[email protected]>
> ---
> net/sunrpc/sunrpc.h | 1 +
> net/sunrpc/sunrpc_syms.c | 2 ++
> net/sunrpc/svcauth.c | 18 ++++++++++++++++++
> 3 files changed, 21 insertions(+)
>
> diff --git a/net/sunrpc/sunrpc.h b/net/sunrpc/sunrpc.h
> index 47a756503d11..f6fe2e6cd65a 100644
> --- a/net/sunrpc/sunrpc.h
> +++ b/net/sunrpc/sunrpc.h
> @@ -52,4 +52,5 @@ static inline int sock_is_loopback(struct sock *sk)
>
> int rpc_clients_notifier_register(void);
> void rpc_clients_notifier_unregister(void);
> +void auth_domain_cleanup(void);
> #endif /* _NET_SUNRPC_SUNRPC_H */
> diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
> index f9edaa9174a4..236fadc4a439 100644
> --- a/net/sunrpc/sunrpc_syms.c
> +++ b/net/sunrpc/sunrpc_syms.c
> @@ -23,6 +23,7 @@
> #include <linux/sunrpc/rpc_pipe_fs.h>
> #include <linux/sunrpc/xprtsock.h>
>
> +#include "sunrpc.h"
> #include "netns.h"
>
> unsigned int sunrpc_net_id;
> @@ -131,6 +132,7 @@ cleanup_sunrpc(void)
> unregister_rpc_pipefs();
> rpc_destroy_mempool();
> unregister_pernet_subsys(&sunrpc_net_ops);
> + auth_domain_cleanup();
> #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
> rpc_unregister_sysctl();
> #endif
> diff --git a/net/sunrpc/svcauth.c b/net/sunrpc/svcauth.c
> index 552617e3467b..477890e8b9d8 100644
> --- a/net/sunrpc/svcauth.c
> +++ b/net/sunrpc/svcauth.c
> @@ -205,3 +205,21 @@ struct auth_domain *auth_domain_find(char *name)
> return NULL;
> }
> EXPORT_SYMBOL_GPL(auth_domain_find);
> +
> +void auth_domain_cleanup(void)
> +{
> + /* There should be no auth_domains left at module unload */
Since this is a globally-visible function, could you move this comment
into a Doxy documenting comment before the function? It should make clear
that the purpose of this function is only for debugging.
> + int h;
> + bool found = false;
> +
> + for (h = 0; h < DN_HASHMAX; h++) {
> + struct auth_domain *hp;
> +
> + hlist_for_each_entry(hp, auth_domain_table+h, hash) {
> + found = true;
> + printk(KERN_WARNING "sunrpc: domain %s still present at module unload.\n",
> + hp->name);
Nit: Documentation/process/coding-style.rst recommends using the pr_warn()
macro here (and equivalents in other patches)... And note that "svc:" is
the conventional prefix for server-side warnings.
I'm wondering... is it safe to release an auth_domain here if one is found,
so that it is not actually orphaned? The warning is information for
developers; there's nothing, say, an administrator can do about this
situation.
> + }
> + }
> + WARN(found, "sunrpc: auth_domain_table not clean -> memory leak\n");
Not sure a stack trace in addition to the above warning messages adds
relevant information. Can you provide a little justification for that?
Thanks!
> +}
>
>
--
Chuck Lever
On Thu, May 21 2020, Chuck Lever wrote:
> Hi Neil!
>
> Thanks for the patches. Seems to me like a good fix overall.
>
> Judging by the syzbot e-mail, you might be posting a refresh of this
> patch series, so I proffer a few minor review comments below.
>
>
>> On May 20, 2020, at 11:21 PM, NeilBrown <[email protected]> wrote:
>>
>> The domain table should be empty at module unload. If it isn't there is
>> a bug somewhere. So check and report.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206651
>> Cc: [email protected]
>> Signed-off-by: NeilBrown <[email protected]>
>> ---
>> net/sunrpc/sunrpc.h | 1 +
>> net/sunrpc/sunrpc_syms.c | 2 ++
>> net/sunrpc/svcauth.c | 18 ++++++++++++++++++
>> 3 files changed, 21 insertions(+)
>>
>> diff --git a/net/sunrpc/sunrpc.h b/net/sunrpc/sunrpc.h
>> index 47a756503d11..f6fe2e6cd65a 100644
>> --- a/net/sunrpc/sunrpc.h
>> +++ b/net/sunrpc/sunrpc.h
>> @@ -52,4 +52,5 @@ static inline int sock_is_loopback(struct sock *sk)
>>
>> int rpc_clients_notifier_register(void);
>> void rpc_clients_notifier_unregister(void);
>> +void auth_domain_cleanup(void);
>> #endif /* _NET_SUNRPC_SUNRPC_H */
>> diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
>> index f9edaa9174a4..236fadc4a439 100644
>> --- a/net/sunrpc/sunrpc_syms.c
>> +++ b/net/sunrpc/sunrpc_syms.c
>> @@ -23,6 +23,7 @@
>> #include <linux/sunrpc/rpc_pipe_fs.h>
>> #include <linux/sunrpc/xprtsock.h>
>>
>> +#include "sunrpc.h"
>> #include "netns.h"
>>
>> unsigned int sunrpc_net_id;
>> @@ -131,6 +132,7 @@ cleanup_sunrpc(void)
>> unregister_rpc_pipefs();
>> rpc_destroy_mempool();
>> unregister_pernet_subsys(&sunrpc_net_ops);
>> + auth_domain_cleanup();
>> #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>> rpc_unregister_sysctl();
>> #endif
>> diff --git a/net/sunrpc/svcauth.c b/net/sunrpc/svcauth.c
>> index 552617e3467b..477890e8b9d8 100644
>> --- a/net/sunrpc/svcauth.c
>> +++ b/net/sunrpc/svcauth.c
>> @@ -205,3 +205,21 @@ struct auth_domain *auth_domain_find(char *name)
>> return NULL;
>> }
>> EXPORT_SYMBOL_GPL(auth_domain_find);
>> +
>> +void auth_domain_cleanup(void)
>> +{
>> + /* There should be no auth_domains left at module unload */
>
> Since this is a globally-visible function, could you move this comment
> into a Doxy documenting comment before the function? It should make clear
> that the purpose of this function is only for debugging.
I wouldn't call it "globally-visible" as it isn't exported, and isn't
even declared in linux/include/...
But a Doxy comment is probably justified.
>
>
>> + int h;
>> + bool found = false;
>> +
>> + for (h = 0; h < DN_HASHMAX; h++) {
>> + struct auth_domain *hp;
>> +
>> + hlist_for_each_entry(hp, auth_domain_table+h, hash) {
>> + found = true;
>> + printk(KERN_WARNING "sunrpc: domain %s still present at module unload.\n",
>> + hp->name);
>
> Nit: Documentation/process/coding-style.rst recommends using the pr_warn()
> macro here (and equivalents in other patches)... And note that "svc:" is
> the conventional prefix for server-side warnings.
I'll fix that, thanks.
>
> I'm wondering... is it safe to release an auth_domain here if one is found,
> so that it is not actually orphaned? The warning is information for
> developers; there's nothing, say, an administrator can do about this
> situation.
I don't think it is safe to release the domain. The ->release()
function could be in a module that has already been unloaded.
>
>
>> + }
>> + }
>> + WARN(found, "sunrpc: auth_domain_table not clean -> memory leak\n");
>
> Not sure a stack trace in addition to the above warning messages adds
> relevant information. Can you provide a little justification for that?
I guess so. I wanted a nice loud warning - and people tend to notice
stack traces more than they notice printks - it was an attempt at human
engineering :-)
Maybe I'll just leave it as pr_warn...
Thanks for the review.
NeilBrown
>
> Thanks!
>
>
>> +}
>>
>>
>
> --
> Chuck Lever
> On May 21, 2020, at 7:45 PM, NeilBrown <[email protected]> wrote:
>
> On Thu, May 21 2020, Chuck Lever wrote:
>
>> Hi Neil!
>>
>> Thanks for the patches. Seems to me like a good fix overall.
>>
>> Judging by the syzbot e-mail, you might be posting a refresh of this
>> patch series, so I proffer a few minor review comments below.
>>
>>
>>>> On May 20, 2020, at 11:21 PM, NeilBrown <[email protected]> wrote:
>>>
>>> The domain table should be empty at module unload. If it isn't there is
>>> a bug somewhere. So check and report.
>>>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206651
>>> Cc: [email protected]
>>> Signed-off-by: NeilBrown <[email protected]>
>>> ---
>>> net/sunrpc/sunrpc.h | 1 +
>>> net/sunrpc/sunrpc_syms.c | 2 ++
>>> net/sunrpc/svcauth.c | 18 ++++++++++++++++++
>>> 3 files changed, 21 insertions(+)
>>>
>>> diff --git a/net/sunrpc/sunrpc.h b/net/sunrpc/sunrpc.h
>>> index 47a756503d11..f6fe2e6cd65a 100644
>>> --- a/net/sunrpc/sunrpc.h
>>> +++ b/net/sunrpc/sunrpc.h
>>> @@ -52,4 +52,5 @@ static inline int sock_is_loopback(struct sock *sk)
>>>
>>> int rpc_clients_notifier_register(void);
>>> void rpc_clients_notifier_unregister(void);
>>> +void auth_domain_cleanup(void);
>>> #endif /* _NET_SUNRPC_SUNRPC_H */
>>> diff --git a/net/sunrpc/sunrpc_syms.c b/net/sunrpc/sunrpc_syms.c
>>> index f9edaa9174a4..236fadc4a439 100644
>>> --- a/net/sunrpc/sunrpc_syms.c
>>> +++ b/net/sunrpc/sunrpc_syms.c
>>> @@ -23,6 +23,7 @@
>>> #include <linux/sunrpc/rpc_pipe_fs.h>
>>> #include <linux/sunrpc/xprtsock.h>
>>>
>>> +#include "sunrpc.h"
>>> #include "netns.h"
>>>
>>> unsigned int sunrpc_net_id;
>>> @@ -131,6 +132,7 @@ cleanup_sunrpc(void)
>>> unregister_rpc_pipefs();
>>> rpc_destroy_mempool();
>>> unregister_pernet_subsys(&sunrpc_net_ops);
>>> + auth_domain_cleanup();
>>> #if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
>>> rpc_unregister_sysctl();
>>> #endif
>>> diff --git a/net/sunrpc/svcauth.c b/net/sunrpc/svcauth.c
>>> index 552617e3467b..477890e8b9d8 100644
>>> --- a/net/sunrpc/svcauth.c
>>> +++ b/net/sunrpc/svcauth.c
>>> @@ -205,3 +205,21 @@ struct auth_domain *auth_domain_find(char *name)
>>> return NULL;
>>> }
>>> EXPORT_SYMBOL_GPL(auth_domain_find);
>>> +
>>> +void auth_domain_cleanup(void)
>>> +{
>>> + /* There should be no auth_domains left at module unload */
>>
>> Since this is a globally-visible function, could you move this comment
>> into a Doxy documenting comment before the function? It should make clear
>> that the purpose of this function is only for debugging.
>
> I wouldn't call it "globally-visible" as it isn't exported, and isn't
> even declared in linux/include/...
> But a Doxy comment is probably justified.
>
>>
>>
>>> + int h;
>>> + bool found = false;
>>> +
>>> + for (h = 0; h < DN_HASHMAX; h++) {
>>> + struct auth_domain *hp;
>>> +
>>> + hlist_for_each_entry(hp, auth_domain_table+h, hash) {
>>> + found = true;
>>> + printk(KERN_WARNING "sunrpc: domain %s still present at module unload.\n",
>>> + hp->name);
>>
>> Nit: Documentation/process/coding-style.rst recommends using the pr_warn()
>> macro here (and equivalents in other patches)... And note that "svc:" is
>> the conventional prefix for server-side warnings.
>
> I'll fix that, thanks.
>
>>
>> I'm wondering... is it safe to release an auth_domain here if one is found,
>> so that it is not actually orphaned? The warning is information for
>> developers; there's nothing, say, an administrator can do about this
>> situation.
>
> I don't think it is safe to release the domain. The ->release()
> function could be in a module that has already been unloaded.
A comment to that effect would be good. Up to you!
>>> + }
>>> + }
>>> + WARN(found, "sunrpc: auth_domain_table not clean -> memory leak\n");
>>
>> Not sure a stack trace in addition to the above warning messages adds
>> relevant information. Can you provide a little justification for that?
>
> I guess so. I wanted a nice loud warning - and people tend to notice
> stack traces more than they notice printks - it was an attempt at human
> engineering :-)
>
> Maybe I'll just leave it as pr_warn...
>
> Thanks for the review.
>
> NeilBrown
>
>
>>
>> Thanks!
>>
>>
>>> +}
>>>
>>>
>>
>> --
>> Chuck Lever