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
Cal Peake <[email protected]> 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.
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.
On 11/16/05, Andrew Morton <[email protected]> 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.
On Wed, Nov 16, 2005 at 12:59:58AM -0800, Andrew Morton wrote:
> Cal Peake <[email protected]> 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:
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
On 11/21/05, Cal Peake <[email protected]> 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.
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
On 11/22/05, Linus Torvalds <[email protected]> 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.
Jon Masters <[email protected]> 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.
On 11/22/05, Andrew Morton <[email protected]> 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.
Jon Masters <[email protected]> wrote:
>
> On 11/22/05, Andrew Morton <[email protected]> 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.
On 11/22/05, Andrew Morton <[email protected]> wrote:
> Jon Masters <[email protected]> wrote:
> > On 11/22/05, Andrew Morton <[email protected]> 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.
Jon Masters <[email protected]> wrote:
> On 11/22/05, Andrew Morton <[email protected]> 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.
On 11/23/05, Bodo Eggert <[email protected]> wrote:
> Jon Masters <[email protected]> wrote:
> > On 11/22/05, Andrew Morton <[email protected]> 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.
Andrew Morton wrote:
> Jon Masters <[email protected]> wrote:
>
>>On 11/22/05, Andrew Morton <[email protected]> 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 ([email protected])
"The secret to procrastination is to put things off until the
last possible moment - but no longer" -me
On 11/28/05, Bill Davidsen <[email protected]> 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.
Jon Masters wrote:
> On 11/28/05, Bill Davidsen <[email protected]> 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 ([email protected])
"The secret to procrastination is to put things off until the
last possible moment - but no longer" -me
On 11/29/05, Bill Davidsen <[email protected]> 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.