| Message ID | 87bm7a3nue.fsf@notabene.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lustre: mdc: fix possible deadlock in chlg_open() | expand |
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->private_data. So remove chlg_obd_get(), and use file->private_Data to find the obd_device. Signed-off-by: NeilBrown <a class="moz-txt-link-rfc2396E" href="mailto:neilb@suse.com"><neilb@suse.com></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->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 < 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) </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>
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
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)
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(-)