| Message ID | 159003130168.24897.13206733830315341548.stgit@noble (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SUNRPC/svc: fix gss flavour registration problems. | expand |
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 <lkp@intel.com>
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/kbuild-all@lists.01.org
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 <lkp@intel.com>
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/kbuild-all@lists.01.org
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 <neilb@suse.de> 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: stable@vger.kernel.org > Signed-off-by: NeilBrown <neilb@suse.de> > --- > 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 <neilb@suse.de> 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: stable@vger.kernel.org >> Signed-off-by: NeilBrown <neilb@suse.de> >> --- >> 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 <neilb@suse.de> 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 <neilb@suse.de> 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: stable@vger.kernel.org >>> Signed-off-by: NeilBrown <neilb@suse.de> >>> --- >>> 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
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"); +}
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: stable@vger.kernel.org Signed-off-by: NeilBrown <neilb@suse.de> --- net/sunrpc/sunrpc.h | 1 + net/sunrpc/sunrpc_syms.c | 2 ++ net/sunrpc/svcauth.c | 18 ++++++++++++++++++ 3 files changed, 21 insertions(+)