[lustre-devel] [PATCH 09/37] lustre: obdclass: fix module load locking.

James Simmons jsimmons at infradead.org
Sun Feb 24 09:04:24 PST 2019


> Safe module loading requires that we try_module_get() in a context
> where the module cannot be unloaded, typically protected by
> a spinlock that module-unload has to take.
> This doesn't currently happen in class_get_type().
> 
> As free_module() calls synchronize_rcu() between calling the
> exit function and freeing the module, we can use rcu_read_lock()
> to check if the exit function has been called, and try_module_get()
> if it hasn't.
> 
> We must also check the return status of try_module_get().

Reviewed-by: James Simmons <jsimmons at infradead.org>
 
> Signed-off-by: NeilBrown <neilb at suse.com>
> ---
>  drivers/staging/lustre/lustre/obdclass/genops.c |   24 ++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
> index d013e9a358f1..1d1a6b2d436e 100644
> --- a/drivers/staging/lustre/lustre/obdclass/genops.c
> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c
> @@ -98,21 +98,31 @@ static struct obd_type *class_get_type(const char *name)
>  {
>  	struct obd_type *type;
>  
> +	rcu_read_lock();
>  	type = class_search_type(name);
>  
>  	if (!type) {
>  		const char *modname = name;
>  
> +		rcu_read_unlock();
>  		if (!request_module("%s", modname)) {
>  			CDEBUG(D_INFO, "Loaded module '%s'\n", modname);
> -			type = class_search_type(name);
>  		} else {
>  			LCONSOLE_ERROR_MSG(0x158, "Can't load module '%s'\n",
>  					   modname);
>  		}
> +		rcu_read_lock();
> +		type = class_search_type(name);
>  	}
>  	if (type) {
> -		if (try_module_get(type->typ_dt_ops->owner)) {
> +		/*
> +		 * Holding rcu_read_lock() matches the synchronize_rcu() call
> +		 * in free_module() and ensures that if type->typ_dt_ops is
> +		 * not yet NULL, then the module won't be freed until after
> +		 * we rcu_read_unlock().
> +		 */
> +		const struct obd_ops *dt_ops = READ_ONCE(type->typ_dt_ops);
> +		if (dt_ops && try_module_get(dt_ops->owner)) {
>  			refcount_inc(&type->typ_refcnt);
>  			/* class_search_type() returned a counted reference,
>  			 * but we don't need that count any more as
> @@ -124,6 +134,7 @@ static struct obd_type *class_get_type(const char *name)
>  			type = NULL;
>  		}
>  	}
> +	rcu_read_unlock();
>  	return type;
>  }
>  
> @@ -213,11 +224,18 @@ int class_unregister_type(const char *name)
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * Ensure that class_get_type doesn't try to get the module
> +	 * as it could be freed before the obd_type is released.
> +	 * synchronize_rcu() will be called before the module
> +	 * is freed.
> +	 */
> +	type->typ_dt_ops = NULL;
> +
>  	if (refcount_read(&type->typ_refcnt)) {
>  		CERROR("type %s has refcount (%d)\n", name, refcount_read(&type->typ_refcnt));
>  		/* This is a bad situation, let's make the best of it */
>  		/* Remove ops, but leave the name for debugging */
> -		type->typ_dt_ops = NULL;
>  		type->typ_md_ops = NULL;
>  		kobject_put(&type->typ_kobj);
>  		return -EBUSY;
> 
> 
> 


More information about the lustre-devel mailing list