floppy regression from "[PATCH] fix floppy.c to store correct ..."

floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Cal Peake]

Hi,

Commit 88baf3e85af72f606363a85e9a60e9e61cc64a6c:

 "[PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk"

causes an annoying side-effect. Upon first write attempt to a floppy I get 
this:

$ dd if=bootdisk.img of=/dev/fd0 bs=1440k
dd: writing `/dev/fd0': Operation not permitted
1+0 records in
0+0 records out

Any successive attempts succeed without problem. Confirmed that backing 
out the patch fixes it.

-cp

-- 
Phishing, pharming; n.: Ways to obtain phood. -- The Devil's Infosec Dictionary

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Andrew Morton]

Cal Peake <cp@absolutedigital.net> wrote:
>
> Hi,
> 
> Commit 88baf3e85af72f606363a85e9a60e9e61cc64a6c:
> 
>  "[PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk"
> 
> causes an annoying side-effect. Upon first write attempt to a floppy I get 
> this:
> 
> $ dd if=bootdisk.img of=/dev/fd0 bs=1440k
> dd: writing `/dev/fd0': Operation not permitted
> 1+0 records in
> 0+0 records out
> 
> Any successive attempts succeed without problem. Confirmed that backing 
> out the patch fixes it.
> 

hmm, yes, when floppy_open() does its test we haven't yet gone and
determined the state of FD_DISK_WRITABLE.  On later opens, we have done, so
things work OK.

We may be able to do the test at the end of floppy_open(), after
check_disk_change() has called floppy_revalidate().  But for O_NDELAY opens
we appear to be screwed.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Jon Masters]

On 11/16/05, Andrew Morton <akpm@osdl.org> wrote:

> hmm, yes, when floppy_open() does its test we haven't yet gone and
> determined the state of FD_DISK_WRITABLE.  On later opens, we
> have done, so things work OK.

I'll fix this later on today.

> We may be able to do the test at the end of floppy_open(), after
> check_disk_change() has called floppy_revalidate().  But for
> O_NDELAY opens we appear to be screwed.

Give me a few hours to take a look at it. I'll move the test and look
at the latter case and get back to you.

Jon.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Jon Masters]

On Wed, Nov 16, 2005 at 12:59:58AM -0800, Andrew Morton wrote:
> Cal Peake <cp@absolutedigital.net> wrote:

> >  "[PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk"

> > causes an annoying side-effect. Upon first write attempt to a floppy I get 
> > this:

> > $ dd if=bootdisk.img of=/dev/fd0 bs=1440k
> > dd: writing `/dev/fd0': Operation not permitted
> > 1+0 records in
> > 0+0 records out

Yes indeed. I reproduced that behaviour.

> hmm, yes, when floppy_open() does its test we haven't yet gone and
> determined the state of FD_DISK_WRITABLE.  On later opens, we have done, so
> things work OK.

I stuck a test in for first use and had floppy_release free up policy
too. But there are a bunch of problems in the floppy driver I've noticed
in going through it tonight (and there's only so much of that I can take
at 03:43 on a Saturday morning). I'll hopefully followup again.

Jon.

diff -urN linux-2.6.14/drivers/block/floppy.c linux-2.6.14_new/drivers/block/floppy.c
--- linux-2.6.14/drivers/block/floppy.c	2005-10-28 01:02:08.000000000 +0100
+++ linux-2.6.14_new/drivers/block/floppy.c	2005-11-19 03:17:08.000000000 +0000
@@ -1610,10 +1610,11 @@
 			DPRINT("wp=%x\n", ST3 & 0x40);
 		}
 #endif
-		if (!(ST3 & 0x40))
+		if (!(ST3 & 0x40)) {
 			SETF(FD_DISK_WRITABLE);
-		else
+		} else {
 			CLEARF(FD_DISK_WRITABLE);
+		}
 	}
 }
 
@@ -3677,15 +3678,19 @@
 	int drive = (long)inode->i_bdev->bd_disk->private_data;
 
 	down(&open_lock);
+
 	if (UDRS->fd_ref < 0)
 		UDRS->fd_ref = 0;
 	else if (!UDRS->fd_ref--) {
 		DPRINT("floppy_release with fd_ref == 0");
 		UDRS->fd_ref = 0;
 	}
-	if (!UDRS->fd_ref)
+	if (!UDRS->fd_ref) {
+		opened_bdev[drive]->bd_disk->policy = 0;
 		opened_bdev[drive] = NULL;
+	}
 	floppy_release_irq_and_dma();
+
 	up(&open_lock);
 	return 0;
 }
@@ -3714,6 +3719,13 @@
 		USETF(FD_VERIFY);
 	}
 
+	/* set underlying gendisk policy to reflect device ro/rw status */
+	if (UDRS->first_read_date && !(UTESTF(FD_DISK_WRITABLE))) {
+		inode->i_bdev->bd_disk->policy = 1;
+	} else {
+		inode->i_bdev->bd_disk->policy = 0;
+	}
+	
 	if (UDRS->fd_ref == -1 || (UDRS->fd_ref && (filp->f_flags & O_EXCL)))
 		goto out2;
 
@@ -3788,6 +3800,7 @@
 		if ((filp->f_mode & 2) && !(UTESTF(FD_DISK_WRITABLE)))
 			goto out;
 	}
+
 	up(&open_lock);
 	return 0;
 out:

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Cal Peake]

On Sat, 19 Nov 2005, Jon Masters wrote:

> On Wed, Nov 16, 2005 at 12:59:58AM -0800, Andrew Morton wrote:
> 
> > hmm, yes, when floppy_open() does its test we haven't yet gone and
> > determined the state of FD_DISK_WRITABLE.  On later opens, we have done, so
> > things work OK.
> 
> I stuck a test in for first use and had floppy_release free up policy
> too. But there are a bunch of problems in the floppy driver I've noticed
> in going through it tonight (and there's only so much of that I can take
> at 03:43 on a Saturday morning). I'll hopefully followup again.

This patch fixes it for me. If you cook up anything else I'll give it a go 
as well. Thanks Jon.

> diff -urN linux-2.6.14/drivers/block/floppy.c linux-2.6.14_new/drivers/block/floppy.c
> --- linux-2.6.14/drivers/block/floppy.c	2005-10-28 01:02:08.000000000 +0100
> +++ linux-2.6.14_new/drivers/block/floppy.c	2005-11-19 03:17:08.000000000 +0000
> @@ -1610,10 +1610,11 @@
>  			DPRINT("wp=%x\n", ST3 & 0x40);
>  		}
>  #endif
> -		if (!(ST3 & 0x40))
> +		if (!(ST3 & 0x40)) {
>  			SETF(FD_DISK_WRITABLE);
> -		else
> +		} else {
>  			CLEARF(FD_DISK_WRITABLE);
> +		}
>  	}
>  }
>  
> @@ -3677,15 +3678,19 @@
>  	int drive = (long)inode->i_bdev->bd_disk->private_data;
>  
>  	down(&open_lock);
> +
>  	if (UDRS->fd_ref < 0)
>  		UDRS->fd_ref = 0;
>  	else if (!UDRS->fd_ref--) {
>  		DPRINT("floppy_release with fd_ref == 0");
>  		UDRS->fd_ref = 0;
>  	}
> -	if (!UDRS->fd_ref)
> +	if (!UDRS->fd_ref) {
> +		opened_bdev[drive]->bd_disk->policy = 0;
>  		opened_bdev[drive] = NULL;
> +	}
>  	floppy_release_irq_and_dma();
> +
>  	up(&open_lock);
>  	return 0;
>  }
> @@ -3714,6 +3719,13 @@
>  		USETF(FD_VERIFY);
>  	}
>  
> +	/* set underlying gendisk policy to reflect device ro/rw status */
> +	if (UDRS->first_read_date && !(UTESTF(FD_DISK_WRITABLE))) {
> +		inode->i_bdev->bd_disk->policy = 1;
> +	} else {
> +		inode->i_bdev->bd_disk->policy = 0;
> +	}
> +	
>  	if (UDRS->fd_ref == -1 || (UDRS->fd_ref && (filp->f_flags & O_EXCL)))
>  		goto out2;
>  
> @@ -3788,6 +3800,7 @@
>  		if ((filp->f_mode & 2) && !(UTESTF(FD_DISK_WRITABLE)))
>  			goto out;
>  	}
> +
>  	up(&open_lock);
>  	return 0;
>  out:
> 

-- 
Phishing, pharming; n.: Ways to obtain phood. -- The Devil's Infosec Dictionary

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Jon Masters]

On 11/21/05, Cal Peake <cp@absolutedigital.net> wrote:

> This patch fixes it for me. If you cook up anything
> else I'll give it a go as well. Thanks Jon.

I think we'll end up going with that for the time being. I am itching
to break the rest of floppy.c but don't have a lot of time this week -
will followup with Andrew later.

Also, I'm going to resend those gendisk bits I was proposing earlier,
but without the offensive parts.

Jon.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Linus Torvalds]

On Sat, 19 Nov 2005, Jon Masters wrote:
> 
> I stuck a test in for first use and had floppy_release free up policy
> too. But there are a bunch of problems in the floppy driver I've noticed
> in going through it tonight (and there's only so much of that I can take
> at 03:43 on a Saturday morning). I'll hopefully followup again.

Can you do one that is against your previous patch that already got 
merged through Andrew? And sign off on it?

		Linus

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Jon Masters]

On 11/22/05, Linus Torvalds <torvalds@osdl.org> wrote:

> On Sat, 19 Nov 2005, Jon Masters wrote:
> >
> > I stuck a test in for first use and had floppy_release free up policy
> > too. But there are a bunch of problems in the floppy driver I've noticed
> > in going through it tonight (and there's only so much of that I can take
> > at 03:43 on a Saturday morning). I'll hopefully followup again.

> Can you do one that is against your previous patch that already got
> merged through Andrew? And sign off on it?

Yeah, since I hate sleep. It's just test compiling, will mail in (my) morning.

Jon.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Andrew Morton]

Jon Masters <jonathan@jonmasters.org> wrote:
>
>  -	if (!UDRS->fd_ref)
>  +	if (!UDRS->fd_ref) {
>  +		opened_bdev[drive]->bd_disk->policy = 0;
>   		opened_bdev[drive] = NULL;
>  +	}
>   	floppy_release_irq_and_dma();
>  +
>   	up(&open_lock);
>   	return 0;
>   }
>  @@ -3714,6 +3719,13 @@
>   		USETF(FD_VERIFY);
>   	}
>   
>  +	/* set underlying gendisk policy to reflect device ro/rw status */
>  +	if (UDRS->first_read_date && !(UTESTF(FD_DISK_WRITABLE))) {
>  +		inode->i_bdev->bd_disk->policy = 1;
>  +	} else {
>  +		inode->i_bdev->bd_disk->policy = 0;
>  +	}
>  +	

That still does the wrong thing.  Put in a write-protected floppy, try to
write to it and it says -EROFS.  Then pop the WP switch and try to write to
it again and it wrongly claims EPERM.  A second attempt to write will
succeed.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Jon Masters]

On 11/22/05, Andrew Morton <akpm@osdl.org> wrote:

> That still does the wrong thing.  Put in a write-protected floppy, try to
> write to it and it says -EROFS.  Then pop the WP switch and try to
> write to it again and it wrongly claims EPERM.  A second attempt to
> write will succeed.

The problem is that we need to wait until the floppy driver next
checks the read status on the drive. I think to get it completely
right will take moving bits of the floppy driver around, unless I'm
being stupid. I'm planning to do that too though.

Jon.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Andrew Morton]

Jon Masters <jonmasters@gmail.com> wrote:
>
> On 11/22/05, Andrew Morton <akpm@osdl.org> wrote:
> 
> > That still does the wrong thing.  Put in a write-protected floppy, try to
> > write to it and it says -EROFS.  Then pop the WP switch and try to
> > write to it again and it wrongly claims EPERM.  A second attempt to
> > write will succeed.
> 
> The problem is that we need to wait until the floppy driver next
> checks the read status on the drive. I think to get it completely
> right will take moving bits of the floppy driver around, unless I'm
> being stupid. I'm planning to do that too though.
> 

In the meanwhile I think we should revert back to the 2.6.14 version of
floppy.c - the present problem is probably worse than the one which it
kinda-fixes.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Jon Masters]

On 11/22/05, Andrew Morton <akpm@osdl.org> wrote:
> Jon Masters <jonmasters@gmail.com> wrote:

> > On 11/22/05, Andrew Morton <akpm@osdl.org> wrote:

> > > That still does the wrong thing.  Put in a write-protected floppy, try to
> > > write to it and it says -EROFS.  Then pop the WP switch and try to
> > > write to it again and it wrongly claims EPERM.  A second attempt to
> > > write will succeed.

> > The problem is that we need to wait until the floppy driver next
> > checks the read status on the drive. I think to get it completely
> > right will take moving bits of the floppy driver around, unless I'm
> > being stupid. I'm planning to do that too though.

> In the meanwhile I think we should revert back to the 2.6.14 version of
> floppy.c - the present problem is probably worse than the one which it
> kinda-fixes.

Ok, as you please. It's probably going to take something much more
ugly to make this work with things as they stand - I'll get something
out at the weekend.

Jon.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Bill Davidsen]

Andrew Morton wrote:
> Jon Masters <jonmasters@gmail.com> wrote:
> 
>>On 11/22/05, Andrew Morton <akpm@osdl.org> wrote:
>>
>>
>>>That still does the wrong thing.  Put in a write-protected floppy, try to
>>>write to it and it says -EROFS.  Then pop the WP switch and try to
>>>write to it again and it wrongly claims EPERM.  A second attempt to
>>>write will succeed.
>>
>>The problem is that we need to wait until the floppy driver next
>>checks the read status on the drive. I think to get it completely
>>right will take moving bits of the floppy driver around, unless I'm
>>being stupid. I'm planning to do that too though.
>>
> 
> 
> In the meanwhile I think we should revert back to the 2.6.14 version of
> floppy.c - the present problem is probably worse than the one which it
> kinda-fixes.

I think that's best, because there are few people (relatively) using 
floppy, and those who are probably are used to old behaviour.
-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Jon Masters]

On 11/28/05, Bill Davidsen <davidsen@tmr.com> wrote:

> I think that's best, because there are few people (relatively) using
> floppy, and those who are probably are used to old behaviour.

The point of the thread is more that this exposes behaviour which
might be present in other drivers too - assuming the block device
state matches the underlying media.

I was out of commission over the weekend after too much pumpkin pie,
but I'll sort this out this week and send out some patches.

Jon.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Bill Davidsen]

Jon Masters wrote:
> On 11/28/05, Bill Davidsen <davidsen@tmr.com> wrote:
> 
> 
>>I think that's best, because there are few people (relatively) using
>>floppy, and those who are probably are used to old behaviour.
> 
> 
> The point of the thread is more that this exposes behaviour which
> might be present in other drivers too - assuming the block device
> state matches the underlying media.

You missed my point... Andrew suggested that since the new behaviour is
not fully functional that a revert was in order until a new version is
available. I agreed, because the old broken behaviour is at least
expected, while waiting for the floppy driver to check is not, and old
problems are less likely to cause a problem until a fixed fix is in place.

-- 
    -bill davidsen (davidsen@tmr.com)
"The secret to procrastination is to put things off until the
  last possible moment - but no longer"  -me

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Jon Masters]

On 11/29/05, Bill Davidsen <davidsen@tmr.com> wrote:

> You missed my point... Andrew suggested that since the new behaviour is
> not fully functional that a revert was in order until a new version is
> available. I agreed, because the old broken behaviour is at least
> expected, while waiting for the floppy driver to check is not, and old
> problems are less likely to cause a problem until a fixed fix is in place.

Nope. I got your point perfectly and I agree with you. I'll resubmit a
longer term fix later.

Jon.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Mikael Pettersson]

Cal Peake writes:
 > Hi,
 > 
 > Commit 88baf3e85af72f606363a85e9a60e9e61cc64a6c:
 > 
 >  "[PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk"
 > 
 > causes an annoying side-effect. Upon first write attempt to a floppy I get 
 > this:
 > 
 > $ dd if=bootdisk.img of=/dev/fd0 bs=1440k
 > dd: writing `/dev/fd0': Operation not permitted
 > 1+0 records in
 > 0+0 records out
 > 
 > Any successive attempts succeed without problem. Confirmed that backing 
 > out the patch fixes it.

I can confirm that I got the exact same mis-behaviour in -rc1.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Bodo Eggert]

Jon Masters <jonmasters@gmail.com> wrote:
> On 11/22/05, Andrew Morton <akpm@osdl.org> wrote:

>> In the meanwhile I think we should revert back to the 2.6.14 version of
>> floppy.c - the present problem is probably worse than the one which it
>> kinda-fixes.
> 
> Ok, as you please. It's probably going to take something much more
> ugly to make this work with things as they stand - I'll get something
> out at the weekend.

I think it should be a general solution to flipped CP switches on floppies
and USB sticks as well as network block devices on a fs that got remounted
ro or hdparm -W.

The device needs a WP status that gets updated on open or mount (must
complete before open()/mount() completes), on failed write()s iff the write
failed because of write protection error and whenever checking is cheap.
If the check can't be done sanely on open() calls (as in the case of NBD),
asume it to be RW enabled unless we know otherwise (e.g. the user told us).
(re)mounts should allways enforce the check as long as it's possible.
The filesystems will need to get updated to use this status as well and
remount themselves ro (or do a panic/reboot, if desired).

This will still misbehave, but I think it will misbehave in a sane way. You
may get a rw mount on ro devices in corner cases, but you can't keep it. If
you erroneously got your device ro, you can update the status by remounting,
so you won't get stuck with a busy ro filesystem. "true>/dev/node" will
update the state, too, but I doubt it's usefullness unless the application
using the device is designed to take use of this feature.

-- 
Ich danke GMX dafür, die Verwendung meiner Adressen mittels per SPF
verbreiteten Lügen zu sabotieren.

Re: floppy regression from "[PATCH] fix floppy.c to store correct ..."

[Jon Masters]

On 11/23/05, Bodo Eggert <harvested.in.lkml@posting.7eggert.dyndns.org> wrote:
> Jon Masters <jonmasters@gmail.com> wrote:
> > On 11/22/05, Andrew Morton <akpm@osdl.org> wrote:
>
> >> In the meanwhile I think we should revert back to the 2.6.14 version of
> >> floppy.c - the present problem is probably worse than the one which it
> >> kinda-fixes.
> >
> > Ok, as you please. It's probably going to take something much more
> > ugly to make this work with things as they stand - I'll get something
> > out at the weekend.
>
> I think it should be a general solution to flipped CP switches on floppies
> and USB sticks as well as network block devices on a fs that got remounted
> ro or hdparm -W.

Yes. We talked about this before and I even started proposing ideas,
but nobody was interested in more disruptive changes to the block
layer - I'm quite happy to do it.

> The device needs a WP status that gets updated on open or mount (must
> complete before open()/mount() completes),

Yes. In fact I thought about the floppy case some more yesterday and
realised that we can have it do the expensive check only in the case
that we want to open to write, read paths are then still cheap but we
check the media in the write case - this isn't as generic as I want.
It seems to me that this is a whole class of problem that needs a
higher solution and I did propose a callback approach so we can tell
Linux that the media WP state changed.

> on failed write()s iff the write failed because of write protection error and whenever
> checking is cheap.

Which is basically what I said before. But then, I also sent a patch
to rename policy sanely and would like to introduce wrapper macros for
all of this too.

> If the check can't be done sanely on open() calls (as in the case of NBD),
> asume it to be RW enabled unless we know otherwise (e.g. the user told us).

No. That's dangerous and gets us back into the same position. We want
to do the checks according to what kind of open we are trying to do,
in the worse case. Still, we need to handle non-blocking open but
that's the user's problem - they asked for it.

> (re)mounts should allways enforce the check as long as it's possible.
> The filesystems will need to get updated to use this status as well and
> remount themselves ro (or do a panic/reboot, if desired).

As viro said, just randomly changing the media status isn't easy. But
in the case that we changed the media to RO then the higher layers
aren't going to be able to write anyway so we /should/ do an remount
ro anyway. The whole problem comes because Linux blindly relies on the
read/write state of the block device and not of the underlying media.

> This will still misbehave, but I think it will misbehave in a sane way. You
> may get a rw mount on ro devices in corner cases, but you can't keep it.

Yes, but right not it still fails "silently" and possibly results in
data loss. The way to fix this is to have a nice shiney way to inform
Linux of this.

Ok. I'm too busy to spend much time during the week on this right now
but I will set aside some time over the weekend to split out what I
proposed before and send a few patches which implement what we've been
talking about.

Cheers,

Jon.