[PATCH v2] floppy: refactor open() flags handling (was Re: mm: uninterruptable tasks hanged on mmap_sem)

[Jiri Kosina]

On Fri, 5 Feb 2016, Dmitry Vyukov wrote:

> > could you please feed the patch below (on top of the previous floppy fix)
> > to your syzkaller machinery and test whether you are still able to
> > reproduce the problem? It passess my local testing here.
> 
> Now that open exits early with EWOULDBLOCK, I guess the reproduced is
> not doing anything particularly interesting. 

Yeah. But as I explained in the changelog, I think it's a valid thing to 
do (opinions welcome).

I don't think having a huge discussion about what nonblocking really means 
for floppy and then try to refactor the whole driver to support that would 
make sense.

Alternatively we can take more conservative aproach, accept the 
nonblocking flag, but do the regular business of the driver.

Actually, let's try that, to make sure that we don't introduce userspace 
breakage.

Could you please retest with the patch below?

Thanks a lot.



From: Jiri Kosina <jkosina@xxxxxxx>
Subject: [PATCH v2] floppy: refactor open() flags handling

In case /dev/fdX is open with O_NDELAY / O_NONBLOCK, floppy_open() immediately
succeeds, without performing any further media / controller preparations.
That's "correct" wrt. the NODELAY flag, but is hardly correct wrt. the rest
of the floppy driver, that is not really O_NONBLOCK ready, at all. Therefore
it's not too surprising, that subsequent attempts to work with the
filedescriptor produce bad results. Namely, syzkaller tool has been able
to livelock mmap() on the returned fd to keep waiting on the page unlock
bit forever.

Quite frankly, I have trouble defining what non-blocking behavior would be for
floppies. Is waiting ages for the driver to actually succeed reading a sector
blocking operation? Is waiting for drive motor to start blocking operation? How
about in case of virtualized floppies?

One option would be returning EWOULDBLOCK in case O_NDLEAY / O_NONBLOCK is
being passed to open(). That has a theoretical potential of breaking some
arcane and archaic userspace though.

Let's take a more conservative aproach, and accept the O_NDLEAY flag, and let
the driver behave as usual.

While at it, clean up a bit handling of !(mode & (FMODE_READ|FMODE_WRITE))
case and return EINVAL instead of succeeding as well.

Spotted by syzkaller tool.

Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
NOT-YET-Signed-off-by: Jiri Kosina <jkosina@xxxxxxx>
---
 drivers/block/floppy.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index d15d415..f7d4d7b 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3662,6 +3662,11 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
 
 	opened_bdev[drive] = bdev;
 
+	if (!(mode & (FMODE_READ|FMODE_WRITE))) {
+		res = -EINVAL;
+		goto out;
+	}
+
 	res = -ENXIO;
 
 	if (!floppy_track_buffer) {
@@ -3705,21 +3710,20 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
 	if (UFDCS->rawcmd == 1)
 		UFDCS->rawcmd = 2;
 
-	if (!(mode & FMODE_NDELAY)) {
-		if (mode & (FMODE_READ|FMODE_WRITE)) {
-			UDRS->last_checked = 0;
-			clear_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags);
-			check_disk_change(bdev);
-			if (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags))
-				goto out;
-			if (test_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags))
-				goto out;
-		}
-		res = -EROFS;
-		if ((mode & FMODE_WRITE) &&
-		    !test_bit(FD_DISK_WRITABLE_BIT, &UDRS->flags))
-			goto out;
-	}
+	UDRS->last_checked = 0;
+	clear_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags);
+	check_disk_change(bdev);
+	if (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags))
+		goto out;
+	if (test_bit(FD_OPEN_SHOULD_FAIL_BIT, &UDRS->flags))
+		goto out;
+
+	res = -EROFS;
+
+	if ((mode & FMODE_WRITE) &&
+			!test_bit(FD_DISK_WRITABLE_BIT, &UDRS->flags))
+		goto out;
+
 	mutex_unlock(&open_lock);
 	mutex_unlock(&floppy_mutex);
 	return 0;