Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Chuck Lever <chuck.lever@oracle.com>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH 1/3] sunrpc: check that domain table is empty at module unload.
Date:   Thu, 21 May 2020 20:33:05 -0400
Message-Id: <91AD6EF0-21DD-4FF2-B193-2DABE84D7F75@oracle.com>
References: <878shkam4t.fsf@notabene.neil.brown.name>
Cc:     Bruce Fields <bfields@fieldses.org>, kircherlike@outlook.com,
        Stephen Hemminger <stephen@networkplumber.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
In-Reply-To: <878shkam4t.fsf@notabene.neil.brown.name>
To:     NeilBrown <neilb@suse.de>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On May 21, 2020, at 7:45 PM, NeilBrown <neilb@suse.de> wrote:
>=20
> =EF=BB=BFOn Thu, May 21 2020, Chuck Lever wrote:
>=20
>> Hi Neil!
>>=20
>> Thanks for the patches. Seems to me like a good fix overall.
>>=20
>> 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.
>>=20
>>=20
>>>> On May 20, 2020, at 11:21 PM, NeilBrown <neilb@suse.de> wrote:
>>>=20
>>> The domain table should be empty at module unload.  If it isn't there is=

>>> a bug somewhere.  So check and report.
>>>=20
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=3D206651
>>> 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(+)
>>>=20
>>> 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)
>>>=20
>>> 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>
>>>=20
>>> +#include "sunrpc.h"
>>> #include "netns.h"
>>>=20
>>> 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 */
>>=20
>> 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.
>=20
> 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.
>=20
>>=20
>>=20
>>> +    int h;
>>> +    bool found =3D false;
>>> +
>>> +    for (h =3D 0; h < DN_HASHMAX; h++) {
>>> +        struct auth_domain *hp;
>>> +
>>> +        hlist_for_each_entry(hp, auth_domain_table+h, hash) {
>>> +            found =3D true;
>>> +            printk(KERN_WARNING "sunrpc: domain %s still present at mod=
ule unload.\n",
>>> +                   hp->name);
>>=20
>> 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.
>=20
> I'll fix that, thanks.
>=20
>>=20
>> I'm wondering... is it safe to release an auth_domain here if one is foun=
d,
>> so that it is not actually orphaned? The warning is information for
>> developers; there's nothing, say, an administrator can do about this
>> situation.
>=20
> 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")=
;
>>=20
>> Not sure a stack trace in addition to the above warning messages adds
>> relevant information. Can you provide a little justification for that?
>=20
> 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 :-)
>=20
> Maybe I'll just leave it as pr_warn...
>=20
> Thanks for the review.
>=20
> NeilBrown
>=20
>=20
>>=20
>> Thanks!
>>=20
>>=20
>>> +}
>>>=20
>>>=20
>>=20
>> --
>> Chuck Lever