There is a possible race in md_probe. If two threads call md_probe
for the same device, then one could exit (having checked that
->gendisk exists) before the other has called kobject_init_and_add,
thus returning an incomplete kobj which will cause problems when
we try to add children to it.
So extend the range of protection of disks_mutex slightly to
avoid this possibility.
Signed-off-by: Neil Brown <[email protected]>
### Diffstat output
./drivers/md/md.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff .prev/drivers/md/md.c ./drivers/md/md.c
--- .prev/drivers/md/md.c 2008-06-27 15:31:27.000000000 +1000
+++ ./drivers/md/md.c 2008-06-27 15:31:35.000000000 +1000
@@ -3359,9 +3359,9 @@ static struct kobject *md_probe(dev_t de
disk->queue = mddev->queue;
add_disk(disk);
mddev->gendisk = disk;
- mutex_unlock(&disks_mutex);
error = kobject_init_and_add(&mddev->kobj, &md_ktype, &disk->dev.kobj,
"%s", "md");
+ mutex_unlock(&disks_mutex);
if (error)
printk(KERN_WARNING "md: cannot register %s/md - name in use\n",
disk->disk_name);
On 16:50, NeilBrown wrote:
>
> There is a possible race in md_probe. If two threads call md_probe
> for the same device, then one could exit (having checked that
> ->gendisk exists) before the other has called kobject_init_and_add,
> thus returning an incomplete kobj which will cause problems when
> we try to add children to it.
>
> So extend the range of protection of disks_mutex slightly to
> avoid this possibility.
>
> Signed-off-by: Neil Brown <[email protected]>
>
> ### Diffstat output
> ./drivers/md/md.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff .prev/drivers/md/md.c ./drivers/md/md.c
> --- .prev/drivers/md/md.c 2008-06-27 15:31:27.000000000 +1000
> +++ ./drivers/md/md.c 2008-06-27 15:31:35.000000000 +1000
> @@ -3359,9 +3359,9 @@ static struct kobject *md_probe(dev_t de
> disk->queue = mddev->queue;
> add_disk(disk);
> mddev->gendisk = disk;
> - mutex_unlock(&disks_mutex);
> error = kobject_init_and_add(&mddev->kobj, &md_ktype, &disk->dev.kobj,
> "%s", "md");
> + mutex_unlock(&disks_mutex);
> if (error)
> printk(KERN_WARNING "md: cannot register %s/md - name in use\n",
> disk->disk_name);
Even with this patch, md_probe() calls mddev_find() without holding
the disks_mutex. Is this OK? If it isn't, something like the patch
below might be necessary.
Andre
---
From: Andre Noll <[email protected]>
Fix possible race in md_probe().
The current code calls mddev_find() without any locks held. It might
happen that mddev_find() succeeds but the returned mddev pointer
becomes stale just before the disks_mutex is aquired.
So close the race by calling mddev_find() with the disks mutex held.
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 647395b..6cb8773 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -3184,17 +3184,20 @@ static int mdp_major;
static struct kobject *md_probe(dev_t dev, int *part, void *data)
{
static DEFINE_MUTEX(disks_mutex);
- mddev_t *mddev = mddev_find(dev);
+ mddev_t *mddev;
struct gendisk *disk;
int partitioned = (MAJOR(dev) != MD_MAJOR);
int shift = partitioned ? MdpMinorShift : 0;
int unit = MINOR(dev) >> shift;
int error;
- if (!mddev)
- return NULL;
mutex_lock(&disks_mutex);
+ mddev = mddev_find(dev);
+ if (!mddev) {
+ mutex_unlock(&disks_mutex);
+ return NULL;
+ }
if (mddev->gendisk) {
mutex_unlock(&disks_mutex);
mddev_put(mddev);
--
The only person who always got his work done by Friday was Robinson Crusoe
On Friday June 27, [email protected] wrote:
> On 16:50, NeilBrown wrote:
> >
> > There is a possible race in md_probe. If two threads call md_probe
> > for the same device, then one could exit (having checked that
> > ->gendisk exists) before the other has called kobject_init_and_add,
> > thus returning an incomplete kobj which will cause problems when
> > we try to add children to it.
...
>
> Even with this patch, md_probe() calls mddev_find() without holding
> the disks_mutex. Is this OK? If it isn't, something like the patch
> below might be necessary.
Thanks for looking at this and asking.
No, the below patch is not necessary.
mddev_find gets a reference on the mddev, so it cannot become stale.
md_probe does not return what it gets from mddev_find until getting
the disks_mutex lock and checking the contents, so it is sure to
return a good mddev.
Thanks,
NeilBrown
>
> Andre
> ---
>
> From: Andre Noll <[email protected]>
>
> Fix possible race in md_probe().
>
> The current code calls mddev_find() without any locks held. It might
> happen that mddev_find() succeeds but the returned mddev pointer
> becomes stale just before the disks_mutex is aquired.
>
> So close the race by calling mddev_find() with the disks mutex held.
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 647395b..6cb8773 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -3184,17 +3184,20 @@ static int mdp_major;
> static struct kobject *md_probe(dev_t dev, int *part, void *data)
> {
> static DEFINE_MUTEX(disks_mutex);
> - mddev_t *mddev = mddev_find(dev);
> + mddev_t *mddev;
> struct gendisk *disk;
> int partitioned = (MAJOR(dev) != MD_MAJOR);
> int shift = partitioned ? MdpMinorShift : 0;
> int unit = MINOR(dev) >> shift;
> int error;
>
> - if (!mddev)
> - return NULL;
>
> mutex_lock(&disks_mutex);
> + mddev = mddev_find(dev);
> + if (!mddev) {
> + mutex_unlock(&disks_mutex);
> + return NULL;
> + }
> if (mddev->gendisk) {
> mutex_unlock(&disks_mutex);
> mddev_put(mddev);
>
> --
> The only person who always got his work done by Friday was Robinson Crusoe
On 09:38, Neil Brown wrote:
> mddev_find gets a reference on the mddev, so it cannot become stale.
> md_probe does not return what it gets from mddev_find until getting
> the disks_mutex lock and checking the contents, so it is sure to
> return a good mddev.
I see. Thanks for the clarification.
Andre
--
The only person who always got his work done by Friday was Robinson Crusoe