[1/3] sunrpc: check that domain table is empty at module unload.
diff mbox series

Message ID 159003130168.24897.13206733830315341548.stgit@noble
State New
Headers show
Series
  • SUNRPC/svc: fix gss flavour registration problems.
Related show

Commit Message

NeilBrown May 21, 2020, 3:21 a.m. UTC
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(+)

Comments

kernel test robot May 21, 2020, 7:09 a.m. UTC | #1
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
kernel test robot May 21, 2020, 12:39 p.m. UTC | #2
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
Chuck Lever May 21, 2020, 2:06 p.m. UTC | #3
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
NeilBrown May 21, 2020, 11:44 p.m. UTC | #4
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
Chuck Lever May 22, 2020, 12:33 a.m. UTC | #5
> 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

Patch
diff mbox series

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");
+}