Bug fixes/cleanups for floppy driver (v2)

Bug fixes/cleanups for floppy driver (v2)

[Herton Ronaldo Krzesinski]

Following this are fixes for bugs I noticed on floppy driver, and some
extra cleanups.

v2: separate fixes, and incorporate suggestions by Vivek Goyal.
    also I splitted the cleanups from fixes.
v3: remove dr, also this possibly makes error handling more readable
    (suggested by by Vivek Goyal), incorporate acks.

-- 
[]'s
Herton

[PATCH v3 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop

[Herton Ronaldo Krzesinski]

Since commit 070ad7e ("floppy: convert to delayed work and single-thread
wq"), we end up calling alloc_ordered_workqueue multiple times inside
the loop, which shouldn't be intended. Besides the leak, other side
effect in the current code is if blk_init_queue fails, we would end up
calling unregister_blkdev even if we didn't call yet register_blkdev.

Just moved the allocation of floppy_wq before the loop, and adjusted the
code accordingly.

Cc: stable@vger.kernel.org # 3.5+
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/block/floppy.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index a7d6347..c8d9e68 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4138,6 +4138,10 @@ static int __init do_floppy_init(void)
 
 	raw_cmd = NULL;
 
+	floppy_wq = alloc_ordered_workqueue("floppy", 0);
+	if (!floppy_wq)
+		return -ENOMEM;
+
 	for (dr = 0; dr < N_DRIVE; dr++) {
 		disks[dr] = alloc_disk(1);
 		if (!disks[dr]) {
@@ -4145,16 +4149,10 @@ static int __init do_floppy_init(void)
 			goto out_put_disk;
 		}
 
-		floppy_wq = alloc_ordered_workqueue("floppy", 0);
-		if (!floppy_wq) {
-			err = -ENOMEM;
-			goto out_put_disk;
-		}
-
 		disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);
 		if (!disks[dr]->queue) {
 			err = -ENOMEM;
-			goto out_destroy_workq;
+			goto out_put_disk;
 		}
 
 		blk_queue_max_hw_sectors(disks[dr]->queue, 64);
@@ -4318,8 +4316,6 @@ out_release_dma:
 out_unreg_region:
 	blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
 	platform_driver_unregister(&floppy_driver);
-out_destroy_workq:
-	destroy_workqueue(floppy_wq);
 out_unreg_blkdev:
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 out_put_disk:
@@ -4335,6 +4331,7 @@ out_put_disk:
 		}
 		put_disk(disks[dr]);
 	}
+	destroy_workqueue(floppy_wq);
 	return err;
 }
 
-- 
1.7.9.5

[PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails

[Herton Ronaldo Krzesinski]

If blk_init_queue fails, we do not call put_disk on the current dr
(dr is decremented first in the error handling loop).

Cc: stable@vger.kernel.org
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/block/floppy.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index c8d9e68..1e09e99 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4151,6 +4151,7 @@ static int __init do_floppy_init(void)
 
 		disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);
 		if (!disks[dr]->queue) {
+			put_disk(disks[dr]);
 			err = -ENOMEM;
 			goto out_put_disk;
 		}
-- 
1.7.9.5

[PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling

[Herton Ronaldo Krzesinski]

After commit 3f9a5aa ("floppy: Cleanup disk->queue before caling
put_disk() if add_disk() was never called"), if something fails in the
add_disk loop, we unconditionally set disks[dr]->queue to NULL. But
that's wrong, since we may have succesfully done an add_disk on some of
the drives previously in the loop, and in this case we would end up with
an extra reference to the disks[dr]->queue.

Add a new global array to mark "registered" disks, and use that to check
if we did an add_disk on one of the disks already. Using an array to
track added disks also will help to simplify/cleanup code later, as
suggested by Vivek Goyal.

Cc: stable@vger.kernel.org
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/block/floppy.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 1e09e99..9272203 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -409,6 +409,7 @@ static struct floppy_drive_struct drive_state[N_DRIVE];
 static struct floppy_write_errors write_errors[N_DRIVE];
 static struct timer_list motor_off_timer[N_DRIVE];
 static struct gendisk *disks[N_DRIVE];
+static bool disk_registered[N_DRIVE];
 static struct block_device *opened_bdev[N_DRIVE];
 static DEFINE_MUTEX(open_lock);
 static struct floppy_raw_cmd *raw_cmd, default_raw_cmd;
@@ -4305,6 +4306,7 @@ static int __init do_floppy_init(void)
 		disks[drive]->flags |= GENHD_FL_REMOVABLE;
 		disks[drive]->driverfs_dev = &floppy_device[drive].dev;
 		add_disk(disks[drive]);
+		disk_registered[drive] = true;
 	}
 
 	return 0;
@@ -4328,7 +4330,8 @@ out_put_disk:
 			 * put_disk() is not paired with add_disk() and
 			 * will put queue reference one extra time. fix it.
 			 */
-			disks[dr]->queue = NULL;
+			if (!disk_registered[dr])
+				disks[dr]->queue = NULL;
 		}
 		put_disk(disks[dr]);
 	}
-- 
1.7.9.5

[PATCH v3 4/6] floppy: properly handle failure on add_disk loop

[Herton Ronaldo Krzesinski]

On do_floppy_init, if something failed inside the loop we call add_disk,
there was no cleanup of previous iterations in the error handling.

Cc: stable@vger.kernel.org
Acked-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/block/floppy.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 9272203..3eafe93 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4294,7 +4294,7 @@ static int __init do_floppy_init(void)
 
 		err = platform_device_register(&floppy_device[drive]);
 		if (err)
-			goto out_release_dma;
+			goto out_remove_drives;
 
 		err = device_create_file(&floppy_device[drive].dev,
 					 &dev_attr_cmos);
@@ -4313,6 +4313,14 @@ static int __init do_floppy_init(void)
 
 out_unreg_platform_dev:
 	platform_device_unregister(&floppy_device[drive]);
+out_remove_drives:
+	while (drive--) {
+		if (disk_registered[drive]) {
+			del_gendisk(disks[drive]);
+			device_remove_file(&floppy_device[drive].dev, &dev_attr_cmos);
+			platform_device_unregister(&floppy_device[drive]);
+		}
+	}
 out_release_dma:
 	if (atomic_read(&usage_count))
 		floppy_release_irq_and_dma();
-- 
1.7.9.5

[PATCH v3 5/6] floppy: use disk_registered for checking if a drive is present

[Herton Ronaldo Krzesinski]

Simplify/cleanup code, replacing remaining checks for drives present
using disk_registered array.

Acked-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/block/floppy.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 3eafe93..381b9c1 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4114,9 +4114,7 @@ static struct platform_device floppy_device[N_DRIVE];
 static struct kobject *floppy_find(dev_t dev, int *part, void *data)
 {
 	int drive = (*part & 3) | ((*part & 0x80) >> 5);
-	if (drive >= N_DRIVE ||
-	    !(allowed_drive_mask & (1 << drive)) ||
-	    fdc_state[FDC(drive)].version == FDC_NONE)
+	if (drive >= N_DRIVE || !disk_registered[drive])
 		return NULL;
 	if (((*part >> 2) & 0x1f) >= ARRAY_SIZE(floppy_type))
 		return NULL;
@@ -4561,8 +4559,7 @@ static void __exit floppy_module_exit(void)
 	for (drive = 0; drive < N_DRIVE; drive++) {
 		del_timer_sync(&motor_off_timer[drive]);
 
-		if ((allowed_drive_mask & (1 << drive)) &&
-		    fdc_state[FDC(drive)].version != FDC_NONE) {
+		if (disk_registered[drive]) {
 			del_gendisk(disks[drive]);
 			device_remove_file(&floppy_device[drive].dev, &dev_attr_cmos);
 			platform_device_unregister(&floppy_device[drive]);
@@ -4573,8 +4570,7 @@ static void __exit floppy_module_exit(void)
 		 * These disks have not called add_disk().  Don't put down
 		 * queue reference in put_disk().
 		 */
-		if (!(allowed_drive_mask & (1 << drive)) ||
-		    fdc_state[FDC(drive)].version == FDC_NONE)
+		if (!disk_registered[drive])
 			disks[drive]->queue = NULL;
 
 		put_disk(disks[drive]);
-- 
1.7.9.5

[PATCH v3 6/6] floppy: remove dr, reuse drive on do_floppy_init

[Herton Ronaldo Krzesinski]

This is a small cleanup, that also may turn error handling of
unitialized disks more readable. We don't need a separate variable to
track allocated disks, remove dr and reuse drive variable instead.

Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 drivers/block/floppy.c |   46 +++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 381b9c1..0312ba4 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4124,8 +4124,7 @@ static struct kobject *floppy_find(dev_t dev, int *part, void *data)
 
 static int __init do_floppy_init(void)
 {
-	int i, unit, drive;
-	int err, dr;
+	int i, unit, drive, err;
 
 	set_debugt();
 	interruptjiffies = resultjiffies = jiffies;
@@ -4141,29 +4140,28 @@ static int __init do_floppy_init(void)
 	if (!floppy_wq)
 		return -ENOMEM;
 
-	for (dr = 0; dr < N_DRIVE; dr++) {
-		disks[dr] = alloc_disk(1);
-		if (!disks[dr]) {
+	for (drive = 0; drive < N_DRIVE; drive++) {
+		disks[drive] = alloc_disk(1);
+		if (!disks[drive]) {
 			err = -ENOMEM;
 			goto out_put_disk;
 		}
 
-		disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);
-		if (!disks[dr]->queue) {
-			put_disk(disks[dr]);
+		disks[drive]->queue = blk_init_queue(do_fd_request, &floppy_lock);
+		if (!disks[drive]->queue) {
 			err = -ENOMEM;
 			goto out_put_disk;
 		}
 
-		blk_queue_max_hw_sectors(disks[dr]->queue, 64);
-		disks[dr]->major = FLOPPY_MAJOR;
-		disks[dr]->first_minor = TOMINOR(dr);
-		disks[dr]->fops = &floppy_fops;
-		sprintf(disks[dr]->disk_name, "fd%d", dr);
+		blk_queue_max_hw_sectors(disks[drive]->queue, 64);
+		disks[drive]->major = FLOPPY_MAJOR;
+		disks[drive]->first_minor = TOMINOR(drive);
+		disks[drive]->fops = &floppy_fops;
+		sprintf(disks[drive]->disk_name, "fd%d", drive);
 
-		init_timer(&motor_off_timer[dr]);
-		motor_off_timer[dr].data = dr;
-		motor_off_timer[dr].function = motor_off_callback;
+		init_timer(&motor_off_timer[drive]);
+		motor_off_timer[drive].data = drive;
+		motor_off_timer[drive].function = motor_off_callback;
 	}
 
 	err = register_blkdev(FLOPPY_MAJOR, "fd");
@@ -4328,18 +4326,20 @@ out_unreg_region:
 out_unreg_blkdev:
 	unregister_blkdev(FLOPPY_MAJOR, "fd");
 out_put_disk:
-	while (dr--) {
-		del_timer_sync(&motor_off_timer[dr]);
-		if (disks[dr]->queue) {
-			blk_cleanup_queue(disks[dr]->queue);
+	for (drive = 0; drive < N_DRIVE; drive++) {
+		if (!disks[drive])
+			break;
+		if (disks[drive]->queue) {
+			del_timer_sync(&motor_off_timer[drive]);
+			blk_cleanup_queue(disks[drive]->queue);
 			/*
 			 * put_disk() is not paired with add_disk() and
 			 * will put queue reference one extra time. fix it.
 			 */
-			if (!disk_registered[dr])
-				disks[dr]->queue = NULL;
+			if (!disk_registered[drive])
+				disks[drive]->queue = NULL;
 		}
-		put_disk(disks[dr]);
+		put_disk(disks[drive]);
 	}
 	destroy_workqueue(floppy_wq);
 	return err;
-- 
1.7.9.5

Re: Bug fixes/cleanups for floppy driver (v2)

[Herton Ronaldo Krzesinski]

On Mon, Aug 13, 2012 at 03:16:21PM -0300, Herton Ronaldo Krzesinski wrote:
> Following this are fixes for bugs I noticed on floppy driver, and some
> extra cleanups.
> 
> v2: separate fixes, and incorporate suggestions by Vivek Goyal.
>     also I splitted the cleanups from fixes.
> v3: remove dr, also this possibly makes error handling more readable
>     (suggested by by Vivek Goyal), incorporate acks.

Subject obviously should have been "... (v3)"

-- 
[]'s
Herton

Re: [PATCH v3 1/6] floppy: don't call alloc_ordered_workqueue inside the alloc_disk loop

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2373 bytes --]

On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> Since commit 070ad7e ("floppy: convert to delayed work and single-thread
> wq"), we end up calling alloc_ordered_workqueue multiple times inside
> the loop, which shouldn't be intended. Besides the leak, other side
> effect in the current code is if blk_init_queue fails, we would end up
> calling unregister_blkdev even if we didn't call yet register_blkdev.
> 
> Just moved the allocation of floppy_wq before the loop, and adjusted the
> code accordingly.
> 
> Cc: stable@vger.kernel.org # 3.5+
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>

Reviewed-by: Ben Hutchings <ben@decadent.org.uk>

> ---
>  drivers/block/floppy.c |   15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index a7d6347..c8d9e68 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4138,6 +4138,10 @@ static int __init do_floppy_init(void)
>  
>  	raw_cmd = NULL;
>  
> +	floppy_wq = alloc_ordered_workqueue("floppy", 0);
> +	if (!floppy_wq)
> +		return -ENOMEM;
> +
>  	for (dr = 0; dr < N_DRIVE; dr++) {
>  		disks[dr] = alloc_disk(1);
>  		if (!disks[dr]) {
> @@ -4145,16 +4149,10 @@ static int __init do_floppy_init(void)
>  			goto out_put_disk;
>  		}
>  
> -		floppy_wq = alloc_ordered_workqueue("floppy", 0);
> -		if (!floppy_wq) {
> -			err = -ENOMEM;
> -			goto out_put_disk;
> -		}
> -
>  		disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);
>  		if (!disks[dr]->queue) {
>  			err = -ENOMEM;
> -			goto out_destroy_workq;
> +			goto out_put_disk;
>  		}
>  
>  		blk_queue_max_hw_sectors(disks[dr]->queue, 64);
> @@ -4318,8 +4316,6 @@ out_release_dma:
>  out_unreg_region:
>  	blk_unregister_region(MKDEV(FLOPPY_MAJOR, 0), 256);
>  	platform_driver_unregister(&floppy_driver);
> -out_destroy_workq:
> -	destroy_workqueue(floppy_wq);
>  out_unreg_blkdev:
>  	unregister_blkdev(FLOPPY_MAJOR, "fd");
>  out_put_disk:
> @@ -4335,6 +4331,7 @@ out_put_disk:
>  		}
>  		put_disk(disks[dr]);
>  	}
> +	destroy_workqueue(floppy_wq);
>  	return err;
>  }
>  

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> If blk_init_queue fails, we do not call put_disk on the current dr
> (dr is decremented first in the error handling loop).
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>

Reviewed-by: Ben Hutchings <ben@decadent.org.uk>

> ---
>  drivers/block/floppy.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index c8d9e68..1e09e99 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4151,6 +4151,7 @@ static int __init do_floppy_init(void)
>  
>  		disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);
>  		if (!disks[dr]->queue) {
> +			put_disk(disks[dr]);
>  			err = -ENOMEM;
>  			goto out_put_disk;
>  		}

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 3865 bytes --]

On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> After commit 3f9a5aa ("floppy: Cleanup disk->queue before caling
> put_disk() if add_disk() was never called"), if something fails in the
> add_disk loop, we unconditionally set disks[dr]->queue to NULL. But
> that's wrong, since we may have succesfully done an add_disk on some of
> the drives previously in the loop, and in this case we would end up with
> an extra reference to the disks[dr]->queue.
> 
> Add a new global array to mark "registered" disks, and use that to check
> if we did an add_disk on one of the disks already. Using an array to
> track added disks also will help to simplify/cleanup code later, as
> suggested by Vivek Goyal.
[...]

It's totally ridiculous that a driver should have to do this.  Any
registered disk should have the GENHD_FL_UP flag set... so why can't
genhd check it?  It doesn't look like floppy is the only driver affected
by this problem, either.  So I suggest the following general fix
(untested):

---
Subject: genhd: Make put_disk() safe for disks that have not been registered

Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
condition'), add_disk() adds a reference to disk->queue, which is then
dropped by disk_release().  But if a disk is destroyed without being
registered through add_disk() (or if add_disk() fails at the first
hurdle) then we have a reference imbalance.

Use the GENHD_FL_UP flag to tell whether this extra reference has been
added.  Remove the incomplete workaround from the floppy driver.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Cc: stable@vger.kernel.org
---
 block/genhd.c          |    6 +++---
 drivers/block/floppy.c |   16 +---------------
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index cac7366..6201212 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -587,8 +587,6 @@ void add_disk(struct gendisk *disk)
 	WARN_ON(disk->minors && !(disk->major || disk->first_minor));
 	WARN_ON(!disk->minors && !(disk->flags & GENHD_FL_EXT_DEVT));
 
-	disk->flags |= GENHD_FL_UP;
-
 	retval = blk_alloc_devt(&disk->part0, &devt);
 	if (retval) {
 		WARN_ON(1);
@@ -596,6 +594,8 @@ void add_disk(struct gendisk *disk)
 	}
 	disk_to_dev(disk)->devt = devt;
 
+	disk->flags |= GENHD_FL_UP;
+
 	/* ->major and ->first_minor aren't supposed to be
 	 * dereferenced from here on, but set them just in case.
 	 */
@@ -1105,7 +1105,7 @@ static void disk_release(struct device *dev)
 	disk_replace_part_tbl(disk, NULL);
 	free_part_stats(&disk->part0);
 	free_part_info(&disk->part0);
-	if (disk->queue)
+	if (disk->queue && disk->flags & GENHD_FL_UP)
 		blk_put_queue(disk->queue);
 	kfree(disk);
 }
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index a7d6347..cf27922 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4325,14 +4325,8 @@ out_unreg_blkdev:
 out_put_disk:
 	while (dr--) {
 		del_timer_sync(&motor_off_timer[dr]);
-		if (disks[dr]->queue) {
+		if (disks[dr]->queue)
 			blk_cleanup_queue(disks[dr]->queue);
-			/*
-			 * put_disk() is not paired with add_disk() and
-			 * will put queue reference one extra time. fix it.
-			 */
-			disks[dr]->queue = NULL;
-		}
 		put_disk(disks[dr]);
 	}
 	return err;
@@ -4560,14 +4554,6 @@ static void __exit floppy_module_exit(void)
 		}
 		blk_cleanup_queue(disks[drive]->queue);
 
-		/*
-		 * These disks have not called add_disk().  Don't put down
-		 * queue reference in put_disk().
-		 */
-		if (!(allowed_drive_mask & (1 << drive)) ||
-		    fdc_state[FDC(drive)].version == FDC_NONE)
-			disks[drive]->queue = NULL;
-
 		put_disk(disks[drive]);
 	}
 


-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v3 4/6] floppy: properly handle failure on add_disk loop

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1832 bytes --]

On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> On do_floppy_init, if something failed inside the loop we call add_disk,
> there was no cleanup of previous iterations in the error handling.
> 
> Cc: stable@vger.kernel.org
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>

This depends on 3/6.  If that's replaced by my proposed fix, then:

> ---
>  drivers/block/floppy.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 9272203..3eafe93 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4294,7 +4294,7 @@ static int __init do_floppy_init(void)
>  
>  		err = platform_device_register(&floppy_device[drive]);
>  		if (err)
> -			goto out_release_dma;
> +			goto out_remove_drives;
>  
>  		err = device_create_file(&floppy_device[drive].dev,
>  					 &dev_attr_cmos);
> @@ -4313,6 +4313,14 @@ static int __init do_floppy_init(void)
>  
>  out_unreg_platform_dev:
>  	platform_device_unregister(&floppy_device[drive]);
> +out_remove_drives:
> +	while (drive--) {
> +		if (disk_registered[drive]) {

I think the test of allowed_drive_mask and FDC version before
registering each driver should be factored out into a function that you
can use here.  There would then no need for the disk_registered array.

Ben.

> +			del_gendisk(disks[drive]);
> +			device_remove_file(&floppy_device[drive].dev, &dev_attr_cmos);
> +			platform_device_unregister(&floppy_device[drive]);
> +		}
> +	}
>  out_release_dma:
>  	if (atomic_read(&usage_count))
>  		floppy_release_irq_and_dma();

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v3 5/6] floppy: use disk_registered for checking if a drive is present

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]

On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> Simplify/cleanup code, replacing remaining checks for drives present
> using disk_registered array.
> 
> Acked-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> ---
>  drivers/block/floppy.c |   10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 3eafe93..381b9c1 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4114,9 +4114,7 @@ static struct platform_device floppy_device[N_DRIVE];
>  static struct kobject *floppy_find(dev_t dev, int *part, void *data)
>  {
>  	int drive = (*part & 3) | ((*part & 0x80) >> 5);
> -	if (drive >= N_DRIVE ||
> -	    !(allowed_drive_mask & (1 << drive)) ||
> -	    fdc_state[FDC(drive)].version == FDC_NONE)
> +	if (drive >= N_DRIVE || !disk_registered[drive])
>  		return NULL;
>  	if (((*part >> 2) & 0x1f) >= ARRAY_SIZE(floppy_type))
>  		return NULL;
> @@ -4561,8 +4559,7 @@ static void __exit floppy_module_exit(void)
>  	for (drive = 0; drive < N_DRIVE; drive++) {
>  		del_timer_sync(&motor_off_timer[drive]);
>  
> -		if ((allowed_drive_mask & (1 << drive)) &&
> -		    fdc_state[FDC(drive)].version != FDC_NONE) {
> +		if (disk_registered[drive]) {
>  			del_gendisk(disks[drive]);
>  			device_remove_file(&floppy_device[drive].dev, &dev_attr_cmos);
>  			platform_device_unregister(&floppy_device[drive]);

The same function I suggested for 4/6 would also be used here.  A little
neater than yet another static array, I think.  But I have nothing to do
with this code so you could just ignore my preferences. :-)

Ben.

> @@ -4573,8 +4570,7 @@ static void __exit floppy_module_exit(void)
>  		 * These disks have not called add_disk().  Don't put down
>  		 * queue reference in put_disk().
>  		 */
> -		if (!(allowed_drive_mask & (1 << drive)) ||
> -		    fdc_state[FDC(drive)].version == FDC_NONE)
> +		if (!disk_registered[drive])
>  			disks[drive]->queue = NULL;
>  
>  		put_disk(disks[drive]);

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling

[Stanislaw Gruszka]

On Tue, Aug 14, 2012 at 04:20:39AM +0100, Ben Hutchings wrote:
> On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> > After commit 3f9a5aa ("floppy: Cleanup disk->queue before caling
> > put_disk() if add_disk() was never called"), if something fails in the
> > add_disk loop, we unconditionally set disks[dr]->queue to NULL. But
> > that's wrong, since we may have succesfully done an add_disk on some of
> > the drives previously in the loop, and in this case we would end up with
> > an extra reference to the disks[dr]->queue.
> > 
> > Add a new global array to mark "registered" disks, and use that to check
> > if we did an add_disk on one of the disks already. Using an array to
> > track added disks also will help to simplify/cleanup code later, as
> > suggested by Vivek Goyal.
> [...]
> 
> It's totally ridiculous that a driver should have to do this.  Any
> registered disk should have the GENHD_FL_UP flag set... so why can't
> genhd check it?  It doesn't look like floppy is the only driver affected
> by this problem, either.  So I suggest the following general fix
> (untested):
> 
> ---
> Subject: genhd: Make put_disk() safe for disks that have not been registered
> 
> Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
> condition'), add_disk() adds a reference to disk->queue,

I do not see this? Commit 9f53d2fe insert disk_alloc_events() to add_disk(),
but disk_alloc_events() function does not get any reference to disk->queue,
I missed something?

Stanislaw

Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling

[Herton Ronaldo Krzesinski]

On Tue, Aug 14, 2012 at 11:03:30AM +0200, Stanislaw Gruszka wrote:
> On Tue, Aug 14, 2012 at 04:20:39AM +0100, Ben Hutchings wrote:
> > It's totally ridiculous that a driver should have to do this.  Any
> > registered disk should have the GENHD_FL_UP flag set... so why can't
> > genhd check it?  It doesn't look like floppy is the only driver affected
> > by this problem, either.  So I suggest the following general fix
> > (untested):
> > 
> > ---
> > Subject: genhd: Make put_disk() safe for disks that have not been registered
> > 
> > Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
> > condition'), add_disk() adds a reference to disk->queue,
> 
> I do not see this? Commit 9f53d2fe insert disk_alloc_events() to add_disk(),
> but disk_alloc_events() function does not get any reference to disk->queue,
> I missed something?

I think he meant commit 523e1d3 ("block: make gendisk hold a reference
to its queue") instead.

> 
> Stanislaw
> 

-- 
[]'s
Herton

Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]

On Tue, 2012-08-14 at 11:03 +0200, Stanislaw Gruszka wrote:
> On Tue, Aug 14, 2012 at 04:20:39AM +0100, Ben Hutchings wrote:
> > On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> > > After commit 3f9a5aa ("floppy: Cleanup disk->queue before caling
> > > put_disk() if add_disk() was never called"), if something fails in the
> > > add_disk loop, we unconditionally set disks[dr]->queue to NULL. But
> > > that's wrong, since we may have succesfully done an add_disk on some of
> > > the drives previously in the loop, and in this case we would end up with
> > > an extra reference to the disks[dr]->queue.
> > > 
> > > Add a new global array to mark "registered" disks, and use that to check
> > > if we did an add_disk on one of the disks already. Using an array to
> > > track added disks also will help to simplify/cleanup code later, as
> > > suggested by Vivek Goyal.
> > [...]
> > 
> > It's totally ridiculous that a driver should have to do this.  Any
> > registered disk should have the GENHD_FL_UP flag set... so why can't
> > genhd check it?  It doesn't look like floppy is the only driver affected
> > by this problem, either.  So I suggest the following general fix
> > (untested):
> > 
> > ---
> > Subject: genhd: Make put_disk() safe for disks that have not been registered
> > 
> > Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
> > condition'), add_disk() adds a reference to disk->queue,
> 
> I do not see this? Commit 9f53d2fe insert disk_alloc_events() to add_disk(),
> but disk_alloc_events() function does not get any reference to disk->queue,
> I missed something?

Sorry, not sure why I pointed to that one.  The reference should of
course be to:

commit 523e1d399ce0e23bec562abe2b2f8d297af81161
Author: Tejun Heo <tj@kernel.org>
Date:   Wed Oct 19 14:31:07 2011 +0200

    block: make gendisk hold a reference to its queue

Ben.

-- 
Ben Hutchings
I say we take off; nuke the site from orbit.  It's the only way to be sure.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling

[Herton Ronaldo Krzesinski]

On Tue, Aug 14, 2012 at 04:20:39AM +0100, Ben Hutchings wrote:
> It's totally ridiculous that a driver should have to do this.  Any
> registered disk should have the GENHD_FL_UP flag set... so why can't
> genhd check it?  It doesn't look like floppy is the only driver affected
> by this problem, either.  So I suggest the following general fix
> (untested):
> 
> ---
> Subject: genhd: Make put_disk() safe for disks that have not been registered
> 
> Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
> condition'), add_disk() adds a reference to disk->queue, which is then
> dropped by disk_release().  But if a disk is destroyed without being
> registered through add_disk() (or if add_disk() fails at the first
> hurdle) then we have a reference imbalance.
> 
> Use the GENHD_FL_UP flag to tell whether this extra reference has been
> added.  Remove the incomplete workaround from the floppy driver.

Indeed this is a more sane/right approach.

Acked-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>

Just the changelog is pointing to the wrong commit as already noted by
Stanislaw.

> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Cc: stable@vger.kernel.org

-- 
[]'s
Herton

Re: [PATCH v3 4/6] floppy: properly handle failure on add_disk loop

[Herton Ronaldo Krzesinski]

On Tue, Aug 14, 2012 at 04:31:23AM +0100, Ben Hutchings wrote:
> On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> > On do_floppy_init, if something failed inside the loop we call add_disk,
> > there was no cleanup of previous iterations in the error handling.
> > 
> > Cc: stable@vger.kernel.org
> > Acked-by: Vivek Goyal <vgoyal@redhat.com>
> > Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> 
> This depends on 3/6.  If that's replaced by my proposed fix, then:
> 
> > ---
> >  drivers/block/floppy.c |   10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> > index 9272203..3eafe93 100644
> > --- a/drivers/block/floppy.c
> > +++ b/drivers/block/floppy.c
> > @@ -4294,7 +4294,7 @@ static int __init do_floppy_init(void)
> >  
> >  		err = platform_device_register(&floppy_device[drive]);
> >  		if (err)
> > -			goto out_release_dma;
> > +			goto out_remove_drives;
> >  
> >  		err = device_create_file(&floppy_device[drive].dev,
> >  					 &dev_attr_cmos);
> > @@ -4313,6 +4313,14 @@ static int __init do_floppy_init(void)
> >  
> >  out_unreg_platform_dev:
> >  	platform_device_unregister(&floppy_device[drive]);
> > +out_remove_drives:
> > +	while (drive--) {
> > +		if (disk_registered[drive]) {
> 
> I think the test of allowed_drive_mask and FDC version before
> registering each driver should be factored out into a function that you
> can use here.  There would then no need for the disk_registered array.

Seems a better approach. I'll redo the patches, but I'm not sure how to
proceed now, since it depends on your change. Patches 4-6 that I did
needs to be dropped/redone. At first my patches 0001/0002 could be
applied with yours, then I would submit another series for the rest. Or
I could take your change and submit your patch along with my series.

> 
> Ben.
> 

-- 
[]'s
Herton

Re: [PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails

[Vivek Goyal]

On Mon, Aug 13, 2012 at 03:16:23PM -0300, Herton Ronaldo Krzesinski wrote:
> If blk_init_queue fails, we do not call put_disk on the current dr
> (dr is decremented first in the error handling loop).
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> ---
>  drivers/block/floppy.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index c8d9e68..1e09e99 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -4151,6 +4151,7 @@ static int __init do_floppy_init(void)
>  
>  		disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);
>  		if (!disks[dr]->queue) {
> +			put_disk(disks[dr]);
>  			err = -ENOMEM;
>  			goto out_put_disk;
>  		}

I think it will create conflict with patch 6. Will it not call put_disk()
twice on disks[dr].

Why are we retaining this patch given the fact that we will loop through
all the drives and queues in out_put_disk.

Thanks
Vivek

Re: [PATCH v3 3/6] floppy: avoid leaking extra reference to queue on do_floppy_init error handling

[Vivek Goyal]

On Tue, Aug 14, 2012 at 04:20:39AM +0100, Ben Hutchings wrote:
> On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> > After commit 3f9a5aa ("floppy: Cleanup disk->queue before caling
> > put_disk() if add_disk() was never called"), if something fails in the
> > add_disk loop, we unconditionally set disks[dr]->queue to NULL. But
> > that's wrong, since we may have succesfully done an add_disk on some of
> > the drives previously in the loop, and in this case we would end up with
> > an extra reference to the disks[dr]->queue.
> > 
> > Add a new global array to mark "registered" disks, and use that to check
> > if we did an add_disk on one of the disks already. Using an array to
> > track added disks also will help to simplify/cleanup code later, as
> > suggested by Vivek Goyal.
> [...]
> 
> It's totally ridiculous that a driver should have to do this.  Any
> registered disk should have the GENHD_FL_UP flag set... so why can't
> genhd check it?  It doesn't look like floppy is the only driver affected
> by this problem, either.  So I suggest the following general fix
> (untested):
> 
> ---
> Subject: genhd: Make put_disk() safe for disks that have not been registered
> 
> Since commit 9f53d2f ('block: fix __blkdev_get and add_disk race
> condition'), add_disk() adds a reference to disk->queue, which is then
> dropped by disk_release().  But if a disk is destroyed without being
> registered through add_disk() (or if add_disk() fails at the first
> hurdle) then we have a reference imbalance.
> 
> Use the GENHD_FL_UP flag to tell whether this extra reference has been
> added.  Remove the incomplete workaround from the floppy driver.
> 

Checking for GENHD_FL_UP to represent whether we took a reference on the
disk->queue or not sounds reasonable. 

Thanks
Vivek

Re: [PATCH v3 2/6] floppy: do put_disk on current dr if blk_init_queue fails

[Herton Ronaldo Krzesinski]

On Tue, Aug 14, 2012 at 03:48:35PM -0400, Vivek Goyal wrote:
> On Mon, Aug 13, 2012 at 03:16:23PM -0300, Herton Ronaldo Krzesinski wrote:
> > If blk_init_queue fails, we do not call put_disk on the current dr
> > (dr is decremented first in the error handling loop).
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> > ---
> >  drivers/block/floppy.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> > index c8d9e68..1e09e99 100644
> > --- a/drivers/block/floppy.c
> > +++ b/drivers/block/floppy.c
> > @@ -4151,6 +4151,7 @@ static int __init do_floppy_init(void)
> >  
> >  		disks[dr]->queue = blk_init_queue(do_fd_request, &floppy_lock);
> >  		if (!disks[dr]->queue) {
> > +			put_disk(disks[dr]);
> >  			err = -ENOMEM;
> >  			goto out_put_disk;
> >  		}
> 
> I think it will create conflict with patch 6. Will it not call put_disk()
> twice on disks[dr].

It'll not conflict, because I took care of removing it on patch 6.

> Why are we retaining this patch given the fact that we will loop through
> all the drives and queues in out_put_disk.

Because I wanted a minimal bug fix for stable, didn't want to bring
entire code shuffling/cleanup in patch 6 for stable, and thus didn't
mark that patch for stable.

> 
> Thanks
> Vivek
> 

-- 
[]'s
Herton

Re: [PATCH v3 4/6] floppy: properly handle failure on add_disk loop

[Herton Ronaldo Krzesinski]

On Tue, Aug 14, 2012 at 11:43:11AM -0300, Herton Ronaldo Krzesinski wrote:
> On Tue, Aug 14, 2012 at 04:31:23AM +0100, Ben Hutchings wrote:
> > On Mon, 2012-08-13 at 15:16 -0300, Herton Ronaldo Krzesinski wrote:
> > > On do_floppy_init, if something failed inside the loop we call add_disk,
> > > there was no cleanup of previous iterations in the error handling.
> > > 
> > > Cc: stable@vger.kernel.org
> > > Acked-by: Vivek Goyal <vgoyal@redhat.com>
> > > Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> > 
> > This depends on 3/6.  If that's replaced by my proposed fix, then:

I redid the patch series with your proposed fix, and I'm going to submit it
again in some minutes, so I hope finally it goes forward.

> > 
> > > ---
> > >  drivers/block/floppy.c |   10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> > > index 9272203..3eafe93 100644
> > > --- a/drivers/block/floppy.c
> > > +++ b/drivers/block/floppy.c
> > > @@ -4294,7 +4294,7 @@ static int __init do_floppy_init(void)
> > >  
> > >  		err = platform_device_register(&floppy_device[drive]);
> > >  		if (err)
> > > -			goto out_release_dma;
> > > +			goto out_remove_drives;
> > >  
> > >  		err = device_create_file(&floppy_device[drive].dev,
> > >  					 &dev_attr_cmos);
> > > @@ -4313,6 +4313,14 @@ static int __init do_floppy_init(void)
> > >  
> > >  out_unreg_platform_dev:
> > >  	platform_device_unregister(&floppy_device[drive]);
> > > +out_remove_drives:
> > > +	while (drive--) {
> > > +		if (disk_registered[drive]) {
> > 
> > I think the test of allowed_drive_mask and FDC version before
> > registering each driver should be factored out into a function that you
> > can use here.  There would then no need for the disk_registered array.
> 
> Seems a better approach. I'll redo the patches, but I'm not sure how to
> proceed now, since it depends on your change. Patches 4-6 that I did
> needs to be dropped/redone. At first my patches 0001/0002 could be
> applied with yours, then I would submit another series for the rest. Or
> I could take your change and submit your patch along with my series.
> 
> > 
> > Ben.
> > 

-- 
[]'s
Herton