On Mon, Oct 01, 2018 at 10:41:43AM -0400, Trond Myklebust wrote:
> Module removal is RCU safe by design, so we really have no need to
> lock the 'authtab[]' array.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> ---
> net/sunrpc/svcauth.c | 52 +++++++++++++++++++++++++++++---------------
> 1 file changed, 34 insertions(+), 18 deletions(-)
>
> diff --git a/net/sunrpc/svcauth.c b/net/sunrpc/svcauth.c
> index bb8db3cb8032..f83443856cd1 100644
> --- a/net/sunrpc/svcauth.c
> +++ b/net/sunrpc/svcauth.c
> @@ -27,12 +27,32 @@
> extern struct auth_ops svcauth_null;
> extern struct auth_ops svcauth_unix;
>
> -static DEFINE_SPINLOCK(authtab_lock);
> -static struct auth_ops *authtab[RPC_AUTH_MAXFLAVOR] = {
> - [0] = &svcauth_null,
> - [1] = &svcauth_unix,
> +static struct auth_ops __rcu *authtab[RPC_AUTH_MAXFLAVOR] = {
> + [RPC_AUTH_NULL] = (struct auth_ops __force __rcu *)&svcauth_null,
> + [RPC_AUTH_UNIX] = (struct auth_ops __force __rcu *)&svcauth_unix,
I didn't know about "__rcu" and "__force __rcu". Grepping turns up
relevant documentation for RCU_INIT_POINTER in include/linux/rcupdate.h,
OK, I think I get it.--b.
> };
>
> +static struct auth_ops *
> +svc_get_auth_ops(rpc_authflavor_t flavor)
> +{
> + struct auth_ops *aops;
> +
> + if (flavor >= RPC_AUTH_MAXFLAVOR)
> + return NULL;
> + rcu_read_lock();
> + aops = rcu_dereference(authtab[flavor]);
> + if (aops != NULL && !try_module_get(aops->owner))
> + aops = NULL;
> + rcu_read_unlock();
> + return aops;
> +}
> +
> +static void
> +svc_put_auth_ops(struct auth_ops *aops)
> +{
> + module_put(aops->owner);
> +}
> +
> int
> svc_authenticate(struct svc_rqst *rqstp, __be32 *authp)
> {
> @@ -45,14 +65,11 @@ svc_authenticate(struct svc_rqst *rqstp, __be32 *authp)
>
> dprintk("svc: svc_authenticate (%d)\n", flavor);
>
> - spin_lock(&authtab_lock);
> - if (flavor >= RPC_AUTH_MAXFLAVOR || !(aops = authtab[flavor]) ||
> - !try_module_get(aops->owner)) {
> - spin_unlock(&authtab_lock);
> + aops = svc_get_auth_ops(flavor);
> + if (aops == NULL) {
> *authp = rpc_autherr_badcred;
> return SVC_DENIED;
> }
> - spin_unlock(&authtab_lock);
>
> rqstp->rq_auth_slack = 0;
> init_svc_cred(&rqstp->rq_cred);
> @@ -82,7 +99,7 @@ int svc_authorise(struct svc_rqst *rqstp)
>
> if (aops) {
> rv = aops->release(rqstp);
> - module_put(aops->owner);
> + svc_put_auth_ops(aops);
> }
> return rv;
> }
> @@ -90,13 +107,14 @@ int svc_authorise(struct svc_rqst *rqstp)
> int
> svc_auth_register(rpc_authflavor_t flavor, struct auth_ops *aops)
> {
> + struct auth_ops *old;
> int rv = -EINVAL;
> - spin_lock(&authtab_lock);
> - if (flavor < RPC_AUTH_MAXFLAVOR && authtab[flavor] == NULL) {
> - authtab[flavor] = aops;
> - rv = 0;
> +
> + if (flavor < RPC_AUTH_MAXFLAVOR) {
> + old = cmpxchg((struct auth_ops ** __force)&authtab[flavor], NULL, aops);
> + if (old == NULL || old == aops)
> + rv = 0;
> }
> - spin_unlock(&authtab_lock);
> return rv;
> }
> EXPORT_SYMBOL_GPL(svc_auth_register);
> @@ -104,10 +122,8 @@ EXPORT_SYMBOL_GPL(svc_auth_register);
> void
> svc_auth_unregister(rpc_authflavor_t flavor)
> {
> - spin_lock(&authtab_lock);
> if (flavor < RPC_AUTH_MAXFLAVOR)
> - authtab[flavor] = NULL;
> - spin_unlock(&authtab_lock);
> + rcu_assign_pointer(authtab[flavor], NULL);
> }
> EXPORT_SYMBOL_GPL(svc_auth_unregister);
>
> --
> 2.17.1
