Linux-NFS Archive on lore.kernel.org
 help / color / Atom feed
From: NeilBrown <neilb@suse.de>
To: Chuck Lever <chuck.lever@oracle.com>
Cc: Bruce Fields <bfields@fieldses.org>,
	kircherlike@outlook.com,
	Stephen Hemminger <stephen@networkplumber.org>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 1/3] sunrpc: check that domain table is empty at module unload.
Date: Fri, 22 May 2020 09:44:50 +1000
Message-ID: <878shkam4t.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <AC992FCD-FB50-494D-ACDA-A021428D7F90@oracle.com>


[-- Attachment #1: Type: text/plain, Size: 4109 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-21  3:21 [PATCH 0/3] SUNRPC/svc: fix gss flavour registration problems NeilBrown
2020-05-21  3:21 ` [PATCH 1/3] sunrpc: check that domain table is empty at module unload NeilBrown
2020-05-21  7:09   ` kbuild test robot
2020-05-21 12:39   ` kbuild test robot
2020-05-21 14:06   ` Chuck Lever
2020-05-21 23:44     ` NeilBrown [this message]
2020-05-22  0:33       ` Chuck Lever
2020-05-21  3:21 ` [PATCH 3/3] sunrpc: clean up properly in gss_mech_unregister() NeilBrown
2020-05-21  3:21 ` [PATCH 2/3] sunrpc: svcauth_gss_register_pseudoflavor must reject duplicate registrations NeilBrown
2020-05-22  2:01 [PATCH 0/3 - V2] SUNRPC/svc: fix gss flavour registration problems NeilBrown
2020-05-22  2:01 ` [PATCH 1/3] sunrpc: check that domain table is empty at module unload NeilBrown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=878shkam4t.fsf@notabene.neil.brown.name \
    --to=neilb@suse.de \
    --cc=bfields@fieldses.org \
    --cc=chuck.lever@oracle.com \
    --cc=kircherlike@outlook.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=stephen@networkplumber.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-NFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-nfs/0 linux-nfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-nfs linux-nfs/ https://lore.kernel.org/linux-nfs \
		linux-nfs@vger.kernel.org
	public-inbox-index linux-nfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-nfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git