lustre: mdc: fix possible deadlock in chlg_open()
diff mbox series

Message ID 87bm7a3nue.fsf@notabene.neil.brown.name
State New
Headers show
Series
  • lustre: mdc: fix possible deadlock in chlg_open()
Related show

Commit Message

NeilBrown Oct. 31, 2018, 1:29 a.m. UTC
Lockdep reports a possible deadlock between chlg_open() and
mdc_changelog_cdev_init()

mdc_changelog_cdev_init() takes chlg_registered_dev_lock and then
calls misc_register() which takes misc_mtx.
chlg_open() is called while misc_mtx is held, and tries to take
chlg_registered_dev_lock.
If these two functions race, a deadlock can occur as each thread will
hold one of the locks while trying to take the other.

chlg_open() does not need to take a lock.  It only uses the
lock to stablize a list while looking for the matching
chlg_registered_dev, and this can be found directly by examining
file->private_data.

So remove chlg_obd_get(), and use file->private_Data to find the
obd_device.

Signed-off-by: NeilBrown <neilb@suse.com>
---

Note that this bug exists in upstream (OpenSFS) lustre as well.

 drivers/staging/lustre/lustre/mdc/mdc_changelog.c | 35 +++++------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

Comments

quentin.bouget@cea.fr Oct. 31, 2018, 12:24 p.m. UTC | #1
Le 31/10/2018 à 02:29, NeilBrown a écrit :
> Lockdep reports a possible deadlock between chlg_open() and
> mdc_changelog_cdev_init()
>
> mdc_changelog_cdev_init() takes chlg_registered_dev_lock and then
> calls misc_register() which takes misc_mtx.
> chlg_open() is called while misc_mtx is held, and tries to take
> chlg_registered_dev_lock.
> If these two functions race, a deadlock can occur as each thread will
> hold one of the locks while trying to take the other.
>
> chlg_open() does not need to take a lock.  It only uses the
> lock to stablize a list while looking for the matching
> chlg_registered_dev, and this can be found directly by examining
> file->private_data.
>
> So remove chlg_obd_get(), and use file->private_Data to find the
> obd_device.
>
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>
> Note that this bug exists in upstream (OpenSFS) lustre as well.
I think you need to modify mdc_changelog_cdev_init() as well so that 
misc_register() comes after the two list_add_tail() statements.

If someone opens the device a bit a too fast after it becomes visible, 
they might find an empty list at `dev->ced_obds'.
Previously, holding `chlg_registered_dev_loc' implicitely prevented that.

Is it possible that maybe the kernel shouldn't hold `misc_mtx' while 
calling chlg_open()?

Quentin Bouget
>
>   drivers/staging/lustre/lustre/mdc/mdc_changelog.c | 35 +++++------------------
>   1 file changed, 7 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
> index d83507cbf95c..645e7f24a859 100644
> --- a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
> +++ b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
> @@ -444,31 +444,6 @@ static ssize_t chlg_write(struct file *file, const char __user *buff,
>   	return rc < 0 ? rc : count;
>   }
>   
> -/**
> - * Find the OBD device associated to a changelog character device.
> - * @param[in]  cdev  character device instance descriptor
> - * @return corresponding OBD device or NULL if none was found.
> - */
> -static struct obd_device *chlg_obd_get(dev_t cdev)
> -{
> -	int minor = MINOR(cdev);
> -	struct obd_device *obd = NULL;
> -	struct chlg_registered_dev *curr;
> -
> -	mutex_lock(&chlg_registered_dev_lock);
> -	list_for_each_entry(curr, &chlg_registered_devices, ced_link) {
> -		if (curr->ced_misc.minor == minor) {
> -			/* take the first available OBD device attached */
> -			obd = list_first_entry(&curr->ced_obds,
> -					       struct obd_device,
> -					       u.cli.cl_chg_dev_linkage);
> -			break;
> -		}
> -	}
> -	mutex_unlock(&chlg_registered_dev_lock);
> -	return obd;
> -}
> -
>   /**
>    * Open handler, initialize internal CRS state and spawn prefetch thread if
>    * needed.
> @@ -479,12 +454,16 @@ static struct obd_device *chlg_obd_get(dev_t cdev)
>   static int chlg_open(struct inode *inode, struct file *file)
>   {
>   	struct chlg_reader_state *crs;
> -	struct obd_device *obd = chlg_obd_get(inode->i_rdev);
> +	struct miscdevice *misc = file->private_data;
> +	struct chlg_registered_dev *dev;
> +	struct obd_device *obd;
>   	struct task_struct *task;
>   	int rc;
>   
> -	if (!obd)
> -		return -ENODEV;
> +	dev = container_of(misc, struct chlg_registered_dev, ced_misc);
> +	obd = list_first_entry(&dev->ced_obds,
> +			       struct obd_device,
> +			       u.cli.cl_chg_dev_linkage);
>   
>   	crs = kzalloc(sizeof(*crs), GFP_KERNEL);
>   	if (!crs)
>
>
> _______________________________________________
> lustre-devel mailing list
> lustre-devel@lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <div class="moz-cite-prefix">Le 31/10/2018 à 02:29, NeilBrown a
      écrit :<br>
    </div>
    <blockquote type="cite"
      cite="mid:87bm7a3nue.fsf@notabene.neil.brown.name">
      <pre wrap="">
Lockdep reports a possible deadlock between chlg_open() and
mdc_changelog_cdev_init()

mdc_changelog_cdev_init() takes chlg_registered_dev_lock and then
calls misc_register() which takes misc_mtx.
chlg_open() is called while misc_mtx is held, and tries to take
chlg_registered_dev_lock.
If these two functions race, a deadlock can occur as each thread will
hold one of the locks while trying to take the other.

chlg_open() does not need to take a lock.  It only uses the
lock to stablize a list while looking for the matching
chlg_registered_dev, and this can be found directly by examining
file-&gt;private_data.

So remove chlg_obd_get(), and use file-&gt;private_Data to find the
obd_device.

Signed-off-by: NeilBrown <a class="moz-txt-link-rfc2396E" href="mailto:neilb@suse.com">&lt;neilb@suse.com&gt;</a>
---

Note that this bug exists in upstream (OpenSFS) lustre as well.</pre>
    </blockquote>
    I think you need to modify mdc_changelog_cdev_init() as well so that
    misc_register() comes after the two list_add_tail() statements.<br>
    <br>
    If someone opens the device a bit a too fast after it becomes
    visible, they might find an empty list at `dev-&gt;ced_obds'.<br>
    Previously, holding `chlg_registered_dev_loc' implicitely prevented
    that.<br>
    <br>
    Is it possible that maybe the kernel shouldn't hold `misc_mtx' while
    calling chlg_open()?<br>
    <br>
    Quentin Bouget<br>
    <blockquote type="cite"
      cite="mid:87bm7a3nue.fsf@notabene.neil.brown.name">
      <pre wrap="">

 drivers/staging/lustre/lustre/mdc/mdc_changelog.c | 35 +++++------------------
 1 file changed, 7 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
index d83507cbf95c..645e7f24a859 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
@@ -444,31 +444,6 @@ static ssize_t chlg_write(struct file *file, const char __user *buff,
 	return rc &lt; 0 ? rc : count;
 }
 
-/**
- * Find the OBD device associated to a changelog character device.
- * @param[in]  cdev  character device instance descriptor
- * @return corresponding OBD device or NULL if none was found.
- */
-static struct obd_device *chlg_obd_get(dev_t cdev)
-{
-	int minor = MINOR(cdev);
-	struct obd_device *obd = NULL;
-	struct chlg_registered_dev *curr;
-
-	mutex_lock(&amp;chlg_registered_dev_lock);
-	list_for_each_entry(curr, &amp;chlg_registered_devices, ced_link) {
-		if (curr-&gt;ced_misc.minor == minor) {
-			/* take the first available OBD device attached */
-			obd = list_first_entry(&amp;curr-&gt;ced_obds,
-					       struct obd_device,
-					       u.cli.cl_chg_dev_linkage);
-			break;
-		}
-	}
-	mutex_unlock(&amp;chlg_registered_dev_lock);
-	return obd;
-}
-
 /**
  * Open handler, initialize internal CRS state and spawn prefetch thread if
  * needed.
@@ -479,12 +454,16 @@ static struct obd_device *chlg_obd_get(dev_t cdev)
 static int chlg_open(struct inode *inode, struct file *file)
 {
 	struct chlg_reader_state *crs;
-	struct obd_device *obd = chlg_obd_get(inode-&gt;i_rdev);
+	struct miscdevice *misc = file-&gt;private_data;
+	struct chlg_registered_dev *dev;
+	struct obd_device *obd;
 	struct task_struct *task;
 	int rc;
 
-	if (!obd)
-		return -ENODEV;
+	dev = container_of(misc, struct chlg_registered_dev, ced_misc);
+	obd = list_first_entry(&amp;dev-&gt;ced_obds,
+			       struct obd_device,
+			       u.cli.cl_chg_dev_linkage);
 
 	crs = kzalloc(sizeof(*crs), GFP_KERNEL);
 	if (!crs)
</pre>
      <!--'"--><br>
      <fieldset class="mimeAttachmentHeader"></fieldset>
      <br>
      <pre wrap="">_______________________________________________
lustre-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:lustre-devel@lists.lustre.org">lustre-devel@lists.lustre.org</a>
<a class="moz-txt-link-freetext" href="http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org">http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org</a>
</pre>
    </blockquote>
    <p><br>
    </p>
  </body>
</html>
NeilBrown Oct. 31, 2018, 8:56 p.m. UTC | #2
On Wed, Oct 31 2018, quentin.bouget@cea.fr wrote:

> Le 31/10/2018 à 02:29, NeilBrown a écrit :
>> Lockdep reports a possible deadlock between chlg_open() and
>> mdc_changelog_cdev_init()
>>
>> mdc_changelog_cdev_init() takes chlg_registered_dev_lock and then
>> calls misc_register() which takes misc_mtx.
>> chlg_open() is called while misc_mtx is held, and tries to take
>> chlg_registered_dev_lock.
>> If these two functions race, a deadlock can occur as each thread will
>> hold one of the locks while trying to take the other.
>>
>> chlg_open() does not need to take a lock.  It only uses the
>> lock to stablize a list while looking for the matching
>> chlg_registered_dev, and this can be found directly by examining
>> file->private_data.
>>
>> So remove chlg_obd_get(), and use file->private_Data to find the
>> obd_device.
>>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>
>> Note that this bug exists in upstream (OpenSFS) lustre as well.
> I think you need to modify mdc_changelog_cdev_init() as well so that 
> misc_register() comes after the two list_add_tail() statements.

Yes, definitely.  Thanks for that.  I'll change the patch.

>
> If someone opens the device a bit a too fast after it becomes visible, 
> they might find an empty list at `dev->ced_obds'.
> Previously, holding `chlg_registered_dev_loc' implicitely prevented that.
>
> Is it possible that maybe the kernel shouldn't hold `misc_mtx' while 
> calling chlg_open()?

No, it really should.  It protects against races between chlg_open and
misc_unregister().

Thanks,
NeilBrown


>
> Quentin Bouget
>>
>>   drivers/staging/lustre/lustre/mdc/mdc_changelog.c | 35 +++++------------------
>>   1 file changed, 7 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
>> index d83507cbf95c..645e7f24a859 100644
>> --- a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
>> +++ b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
>> @@ -444,31 +444,6 @@ static ssize_t chlg_write(struct file *file, const char __user *buff,
>>   	return rc < 0 ? rc : count;
>>   }
>>   
>> -/**
>> - * Find the OBD device associated to a changelog character device.
>> - * @param[in]  cdev  character device instance descriptor
>> - * @return corresponding OBD device or NULL if none was found.
>> - */
>> -static struct obd_device *chlg_obd_get(dev_t cdev)
>> -{
>> -	int minor = MINOR(cdev);
>> -	struct obd_device *obd = NULL;
>> -	struct chlg_registered_dev *curr;
>> -
>> -	mutex_lock(&chlg_registered_dev_lock);
>> -	list_for_each_entry(curr, &chlg_registered_devices, ced_link) {
>> -		if (curr->ced_misc.minor == minor) {
>> -			/* take the first available OBD device attached */
>> -			obd = list_first_entry(&curr->ced_obds,
>> -					       struct obd_device,
>> -					       u.cli.cl_chg_dev_linkage);
>> -			break;
>> -		}
>> -	}
>> -	mutex_unlock(&chlg_registered_dev_lock);
>> -	return obd;
>> -}
>> -
>>   /**
>>    * Open handler, initialize internal CRS state and spawn prefetch thread if
>>    * needed.
>> @@ -479,12 +454,16 @@ static struct obd_device *chlg_obd_get(dev_t cdev)
>>   static int chlg_open(struct inode *inode, struct file *file)
>>   {
>>   	struct chlg_reader_state *crs;
>> -	struct obd_device *obd = chlg_obd_get(inode->i_rdev);
>> +	struct miscdevice *misc = file->private_data;
>> +	struct chlg_registered_dev *dev;
>> +	struct obd_device *obd;
>>   	struct task_struct *task;
>>   	int rc;
>>   
>> -	if (!obd)
>> -		return -ENODEV;
>> +	dev = container_of(misc, struct chlg_registered_dev, ced_misc);
>> +	obd = list_first_entry(&dev->ced_obds,
>> +			       struct obd_device,
>> +			       u.cli.cl_chg_dev_linkage);
>>   
>>   	crs = kzalloc(sizeof(*crs), GFP_KERNEL);
>>   	if (!crs)
>>
>>
>> _______________________________________________
>> lustre-devel mailing list
>> lustre-devel@lists.lustre.org
>> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org

Patch
diff mbox series

diff --git a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
index d83507cbf95c..645e7f24a859 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_changelog.c
@@ -444,31 +444,6 @@  static ssize_t chlg_write(struct file *file, const char __user *buff,
 	return rc < 0 ? rc : count;
 }
 
-/**
- * Find the OBD device associated to a changelog character device.
- * @param[in]  cdev  character device instance descriptor
- * @return corresponding OBD device or NULL if none was found.
- */
-static struct obd_device *chlg_obd_get(dev_t cdev)
-{
-	int minor = MINOR(cdev);
-	struct obd_device *obd = NULL;
-	struct chlg_registered_dev *curr;
-
-	mutex_lock(&chlg_registered_dev_lock);
-	list_for_each_entry(curr, &chlg_registered_devices, ced_link) {
-		if (curr->ced_misc.minor == minor) {
-			/* take the first available OBD device attached */
-			obd = list_first_entry(&curr->ced_obds,
-					       struct obd_device,
-					       u.cli.cl_chg_dev_linkage);
-			break;
-		}
-	}
-	mutex_unlock(&chlg_registered_dev_lock);
-	return obd;
-}
-
 /**
  * Open handler, initialize internal CRS state and spawn prefetch thread if
  * needed.
@@ -479,12 +454,16 @@  static struct obd_device *chlg_obd_get(dev_t cdev)
 static int chlg_open(struct inode *inode, struct file *file)
 {
 	struct chlg_reader_state *crs;
-	struct obd_device *obd = chlg_obd_get(inode->i_rdev);
+	struct miscdevice *misc = file->private_data;
+	struct chlg_registered_dev *dev;
+	struct obd_device *obd;
 	struct task_struct *task;
 	int rc;
 
-	if (!obd)
-		return -ENODEV;
+	dev = container_of(misc, struct chlg_registered_dev, ced_misc);
+	obd = list_first_entry(&dev->ced_obds,
+			       struct obd_device,
+			       u.cli.cl_chg_dev_linkage);
 
 	crs = kzalloc(sizeof(*crs), GFP_KERNEL);
 	if (!crs)