[19/21] lustre: obdclass: avoid races in class_register_type()
diff mbox series

Message ID 154949781342.10620.11880180011933935219.stgit@noble.brown
State New
Headers show
Series
  • lustre: Assorted cleanups for obdclass
Related show

Commit Message

NeilBrown Feb. 7, 2019, 12:03 a.m. UTC
If there are two parallel attempts to register the
same class name, it could get registered twice.
So re-check after allocation to make sure the name is
still unique.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/obdclass/genops.c |   29 +++++++++++++++--------
 1 file changed, 19 insertions(+), 10 deletions(-)

Comments

Andreas Dilger Feb. 8, 2019, 6:41 a.m. UTC | #1
On Feb 6, 2019, at 17:03, NeilBrown <neilb@suse.com> wrote:
> 
> If there are two parallel attempts to register the
> same class name, it could get registered twice.
> So re-check after allocation to make sure the name is
> still unique.

The patch itself seems reasonable, but I don't see how this
scenario could ever happen?  class_register_type() is only
called once per device type from the module init code, and
I don't think it is possible to insert the same module twice?

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud
NeilBrown Feb. 11, 2019, 12:58 a.m. UTC | #2
On Fri, Feb 08 2019, Andreas Dilger wrote:

> On Feb 6, 2019, at 17:03, NeilBrown <neilb@suse.com> wrote:
>> 
>> If there are two parallel attempts to register the
>> same class name, it could get registered twice.
>> So re-check after allocation to make sure the name is
>> still unique.
>
> The patch itself seems reasonable, but I don't see how this
> scenario could ever happen?  class_register_type() is only
> called once per device type from the module init code, and
> I don't think it is possible to insert the same module twice?

I agree. I don't think it can happen.
By the same logic, the test

	if (class_search_type(name)) {
		CDEBUG(D_IOCTL, "Type %s already registered\n", name);
		return -EEXIST;
	}

at the start of class_register_type() is pointless, as it is not
possible for the same name to be registered twice.

But I think it is still good to have this test, and to fail-safe.
To me the test as it is "looks" prone to races, so I wanted to fix it.
When I'm analysining obscure bugs, it helps a lot to have code that is
"obviously correct" without having the understand (and double check)
various assumptions about usage which are necessary for it to be
correct.

So this patch isn't needed for correctness. It is needed (I think) to
make the code understandable and maintainable.

Thanks,
NeilBrown


>
> Cheers, Andreas
> ---
> Andreas Dilger
> Principal Lustre Architect
> Whamcloud
James Simmons Feb. 12, 2019, 5:03 a.m. UTC | #3
> If there are two parallel attempts to register the
> same class name, it could get registered twice.
> So re-check after allocation to make sure the name is
> still unique.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  drivers/staging/lustre/lustre/obdclass/genops.c |   29 +++++++++++++++--------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
> index 382eaf519a79..a174f538dd0d 100644
> --- a/drivers/staging/lustre/lustre/obdclass/genops.c
> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c
> @@ -86,17 +86,23 @@ static void obd_device_free(struct obd_device *obd)
>  	kmem_cache_free(obd_device_cachep, obd);
>  }
>  
> -static struct obd_type *class_search_type(const char *name)
> +static struct obd_type *__class_search_type(const char *name)
>  {
>  	struct obd_type *type;
>  
> -	spin_lock(&obd_types_lock);
>  	list_for_each_entry(type, &obd_types, typ_chain) {

This change is fine but we really don't need typ_chain anymore.
We have a list of obd_types already due to the power of kobjects!!!!

We can do an

static struct obd_type *class_search_type(const char *name)
{
	struct kobject *kobj;
	struct obd_type *type;

	kobj = kset_find_obj(lustre_kset, name);

	return "some_mapping from kobj to type"

Hmm. This kobj is not embedded in struct obd_types. Might need more
surgery to make that work.

> -		if (strcmp(type->typ_name, name) == 0) {
> -			spin_unlock(&obd_types_lock);
> +		if (strcmp(type->typ_name, name) == 0)
>  			return type;
> -		}
>  	}
> +	return NULL;
> +}
> +
> +static struct obd_type *class_search_type(const char *name)
> +{
> +	struct obd_type *type;
> +
> +	spin_lock(&obd_types_lock);
> +	type = __class_search_type(name);
>  	spin_unlock(&obd_types_lock);
>  	return NULL;
>  }
> @@ -214,19 +220,22 @@ int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
>  	if (ldt) {
>  		type->typ_lu = ldt;
>  		rc = lu_device_type_init(ldt);
> -		if (rc != 0) {
> -			kobject_put(type->typ_kobj);
> +		if (rc != 0)
>  			goto failed;
> -		}
>  	}
>  
> +	INIT_LIST_HEAD(&type->typ_chain);
>  	spin_lock(&obd_types_lock);
> -	list_add(&type->typ_chain, &obd_types);
> +	if (__class_search_type(name) == NULL)
> +		list_add(&type->typ_chain, &obd_types);
>  	spin_unlock(&obd_types_lock);
>  
> -	return 0;
> +	if (!list_empty(&type->typ_chain))
> +		return 0;
>  
>  failed:
> +	if (!IS_ERR_OR_NULL(type->typ_kobj))
> +		kobject_put(type->typ_kobj);
>  	kfree(type->typ_name);
>  	kfree(type->typ_md_ops);
>  	kfree(type->typ_dt_ops);
> 
> 
>
NeilBrown Feb. 14, 2019, 3:43 a.m. UTC | #4
On Tue, Feb 12 2019, James Simmons wrote:

>> If there are two parallel attempts to register the
>> same class name, it could get registered twice.
>> So re-check after allocation to make sure the name is
>> still unique.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  drivers/staging/lustre/lustre/obdclass/genops.c |   29 +++++++++++++++--------
>>  1 file changed, 19 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
>> index 382eaf519a79..a174f538dd0d 100644
>> --- a/drivers/staging/lustre/lustre/obdclass/genops.c
>> +++ b/drivers/staging/lustre/lustre/obdclass/genops.c
>> @@ -86,17 +86,23 @@ static void obd_device_free(struct obd_device *obd)
>>  	kmem_cache_free(obd_device_cachep, obd);
>>  }
>>  
>> -static struct obd_type *class_search_type(const char *name)
>> +static struct obd_type *__class_search_type(const char *name)
>>  {
>>  	struct obd_type *type;
>>  
>> -	spin_lock(&obd_types_lock);
>>  	list_for_each_entry(type, &obd_types, typ_chain) {
>
> This change is fine but we really don't need typ_chain anymore.
> We have a list of obd_types already due to the power of kobjects!!!!
>
> We can do an
>
> static struct obd_type *class_search_type(const char *name)
> {
> 	struct kobject *kobj;
> 	struct obd_type *type;
>
> 	kobj = kset_find_obj(lustre_kset, name);
>
> 	return "some_mapping from kobj to type"
>
> Hmm. This kobj is not embedded in struct obd_types. Might need more
> surgery to make that work.
>

Well.... that was a fun rabbit-hole to fall down.
I now have 8 patches where before I had two - definitely a better
result. typ_chain and several other fields are gone.

I'll post them in my next series which will probably go out tomorrow.

Thanks,
NeilBrown

Patch
diff mbox series

diff --git a/drivers/staging/lustre/lustre/obdclass/genops.c b/drivers/staging/lustre/lustre/obdclass/genops.c
index 382eaf519a79..a174f538dd0d 100644
--- a/drivers/staging/lustre/lustre/obdclass/genops.c
+++ b/drivers/staging/lustre/lustre/obdclass/genops.c
@@ -86,17 +86,23 @@  static void obd_device_free(struct obd_device *obd)
 	kmem_cache_free(obd_device_cachep, obd);
 }
 
-static struct obd_type *class_search_type(const char *name)
+static struct obd_type *__class_search_type(const char *name)
 {
 	struct obd_type *type;
 
-	spin_lock(&obd_types_lock);
 	list_for_each_entry(type, &obd_types, typ_chain) {
-		if (strcmp(type->typ_name, name) == 0) {
-			spin_unlock(&obd_types_lock);
+		if (strcmp(type->typ_name, name) == 0)
 			return type;
-		}
 	}
+	return NULL;
+}
+
+static struct obd_type *class_search_type(const char *name)
+{
+	struct obd_type *type;
+
+	spin_lock(&obd_types_lock);
+	type = __class_search_type(name);
 	spin_unlock(&obd_types_lock);
 	return NULL;
 }
@@ -214,19 +220,22 @@  int class_register_type(struct obd_ops *dt_ops, struct md_ops *md_ops,
 	if (ldt) {
 		type->typ_lu = ldt;
 		rc = lu_device_type_init(ldt);
-		if (rc != 0) {
-			kobject_put(type->typ_kobj);
+		if (rc != 0)
 			goto failed;
-		}
 	}
 
+	INIT_LIST_HEAD(&type->typ_chain);
 	spin_lock(&obd_types_lock);
-	list_add(&type->typ_chain, &obd_types);
+	if (__class_search_type(name) == NULL)
+		list_add(&type->typ_chain, &obd_types);
 	spin_unlock(&obd_types_lock);
 
-	return 0;
+	if (!list_empty(&type->typ_chain))
+		return 0;
 
 failed:
+	if (!IS_ERR_OR_NULL(type->typ_kobj))
+		kobject_put(type->typ_kobj);
 	kfree(type->typ_name);
 	kfree(type->typ_md_ops);
 	kfree(type->typ_dt_ops);