2005-10-29 17:25:53

by Jon Masters

[permalink] [raw]
Subject: [PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Evgeny Stambulchik found that doing the following always worked:

# mount /dev/fd0 /mnt/floppy/
mount: block device /dev/fd0 is write-protected, mounting read-only
# mount -o remount,rw /mnt/floppy
# echo $?
0

This is the case because the block device /dev/fd0 is writeable but the
floppy disk is marked protected. A fix is to simply have floppy_open
mark the underlying gendisk policy according to reality (since the VFS
doesn't provide a way for do_remount_sb to inquire as to the current
device status).

Signed-off-by: Jon Masters <[email protected]>

- --- 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-10-29
18:14:47.000000000 +0100
@@ -3714,6 +3714,13 @@
USETF(FD_VERIFY);
}

+ /* set underlying gendisk policy to reflect real ro/rw status */
+ if (UTESTF(FD_DISK_WRITABLE)) {
+ inode->i_bdev->bd_disk->policy = 0;
+ } else {
+ inode->i_bdev->bd_disk->policy = 1;
+ }
+
if (UDRS->fd_ref == -1 || (UDRS->fd_ref && (filp->f_flags &
O_EXCL)))
goto out2;
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFDY7CAeTyyexZHHxERAvwzAJ0cnuIhiufkEwRK/Kyj0p8URvLAEgCdF38+
k8hBPhPYvtIt3XGKDfkQbeY=
=P0sF
-----END PGP SIGNATURE-----


2005-10-31 11:57:25

by Evgeny Stambulchik

[permalink] [raw]
Subject: Re: [PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk

Jon Masters wrote:

> Let me know if this fixes it for you - should bomb out now if you try.
> The error isn't the cleanest (blame mount), but it does fail.

This works fine, thanks! For what it worth, though, mount -o remount,rw
says remounting read-only yet still returns success. (Opposite to
busybox, which now says "Permission denied" - rather misleading, but at
least it fails).

My question is, shouldn't it be implemented at a more generic level?
Floppy is just one example. Others are all kind of USB storage, ZIP/Jazz
drives, and even normal SCSI disks (which have a RO jumper - at least
some of them do).

I got an ancient USB disk on key with a write-protection switch. When I
plug it in in the RO mode, everything goes exactly as it was (before
your patch) with floppy. Now something interesting:
1. The thingy is plugged in RW and mounted
2. While connected, I switch it to RO
dmesg says:
-> SCSI device sda: 129024 512-byte hdwr sectors (66 MB)
-> sda: Write Protect is on
-> sda: Mode Sense: 03 00 80 00
-> sda: assuming drive cache: write through
3.
# mount -o remount,rw /mnt
mount: block device /dev/sda1 is write-protected, mounting read-only
# echo $?
0

So it seems there is some layer in bd which does know about RO status
(and furthermore it's set by hot events)?

Regards,

Evgeny

2005-10-31 15:59:03

by Jon Masters

[permalink] [raw]
Subject: Re: [PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk

On 10/31/05, Evgeny Stambulchik <[email protected]> wrote:
> Jon Masters wrote:

> > Let me know if this fixes it for you - should bomb out now if you try.
> > The error isn't the cleanest (blame mount), but it does fail.

> This works fine, thanks!

Good. This raises the general problem of devices knowing about their
underlying media status.

> For what it worth, though, mount -o remount,rw
> says remounting read-only yet still returns success. (Opposite to
> busybox, which now says "Permission denied" - rather misleading, but at
> least it fails).

Mount needs some fixing :-) It just tries calling sys_mount and
retries ro if that fails, then returns success because the last call
worked ok. Or so it would seem from the strace output.

> My question is, shouldn't it be implemented at a more generic level?

Yes. Right now, I think the gendisk policy is the best place to store
this - but having some generic "check the media status" VFS callback
might be a good thing.

> Floppy is just one example. Others are all kind of USB storage, ZIP/Jazz
> drives, and even normal SCSI disks (which have a RO jumper - at least
> some of them do).

I'll check, but they probably do something like this in their implementation.

> I got an ancient USB disk on key with a write-protection switch. When I
> plug it in in the RO mode, everything goes exactly as it was (before
> your patch) with floppy. Now something interesting:
> 1. The thingy is plugged in RW and mounted
> 2. While connected, I switch it to RO
> dmesg says:
> -> SCSI device sda: 129024 512-byte hdwr sectors (66 MB)
> -> sda: Write Protect is on
> -> sda: Mode Sense: 03 00 80 00
> -> sda: assuming drive cache: write through
> 3.
> # mount -o remount,rw /mnt
> mount: block device /dev/sda1 is write-protected, mounting read-only
> # echo $?
> 0
>
> So it seems there is some layer in bd which does know about RO status
> (and furthermore it's set by hot events)?

There's a callback for remount requests, perhaps they use that. Bear
in mind that USB devices tend to be more communicative than ancient
floppy disk drives. I'll have a look into this though - anyone else
reading who thinks we need a generic way of checking media read/write
status in addition to media change detect?

Jon.

2005-10-31 23:17:27

by Rob Landley

[permalink] [raw]
Subject: Re: [PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk

On Monday 31 October 2005 05:57, Evgeny Stambulchik wrote:
> Jon Masters wrote:
> > Let me know if this fixes it for you - should bomb out now if you try.
> > The error isn't the cleanest (blame mount), but it does fail.
>
> This works fine, thanks! For what it worth, though, mount -o remount,rw
> says remounting read-only yet still returns success. (Opposite to
> busybox, which now says "Permission denied" - rather misleading, but at
> least it fails).

That sounds like the string translation of EPERM returned by libc's
strerror(). (At busybox we're frugal bastards; we don't include text
messages when we can get the C library to provide them for us. :)

But yeah, we're sticklers for correct behavior, and only attempt to remount
readonly if we get EACCES or EROFS, not _just_ because we attempted a
read/write mount and it failed. (And yes, I personally tested this corner
case. We haven't started on an automated regression test script for mount
yet because running it would require root access, but it's on the todo list
as we upgrade the test suite in our Copious Free Time...)

Rob

2005-11-01 02:36:44

by Jon Masters

[permalink] [raw]
Subject: Re: [PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk

On 10/31/05, Rob Landley <[email protected]> wrote:
> On Monday 31 October 2005 05:57, Evgeny Stambulchik wrote:
> > Jon Masters wrote:
> > > Let me know if this fixes it for you - should bomb out now if you try.
> > > The error isn't the cleanest (blame mount), but it does fail.
> >
> > This works fine, thanks! For what it worth, though, mount -o remount,rw
> > says remounting read-only yet still returns success. (Opposite to
> > busybox, which now says "Permission denied" - rather misleading, but at
> > least it fails).
>
> That sounds like the string translation of EPERM returned by libc's
> strerror(). (At busybox we're frugal bastards; we don't include text
> messages when we can get the C library to provide them for us. :)

Indeed. Reminds me that I should clean up and send a form parser I
wrote for busybox to handle multi-part mime form posts in its
webserver while I'm at it. That's something it could do with even if
it's being frugal.

> But yeah, we're sticklers for correct behavior, and only attempt to remount
> readonly if we get EACCES or EROFS, not _just_ because we attempted a
> read/write mount and it failed. (And yes, I personally tested this corner
> case. We haven't started on an automated regression test script for mount
> yet because running it would require root access, but it's on the todo list
> as we upgrade the test suite in our Copious Free Time...)

You looked at running this inside a qemu environment (scratchbox), huh?

Jon.

2005-11-01 08:40:34

by Rob Landley

[permalink] [raw]
Subject: Re: [PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk

On Monday 31 October 2005 20:36, Jon Masters wrote:
> On 10/31/05, Rob Landley <[email protected]> wrote:
> > On Monday 31 October 2005 05:57, Evgeny Stambulchik wrote:
> > > Jon Masters wrote:
> > > > Let me know if this fixes it for you - should bomb out now if you
> > > > try. The error isn't the cleanest (blame mount), but it does fail.
> > >
> > > This works fine, thanks! For what it worth, though, mount -o remount,rw
> > > says remounting read-only yet still returns success. (Opposite to
> > > busybox, which now says "Permission denied" - rather misleading, but at
> > > least it fails).
> >
> > That sounds like the string translation of EPERM returned by libc's
> > strerror(). (At busybox we're frugal bastards; we don't include text
> > messages when we can get the C library to provide them for us. :)
>
> Indeed. Reminds me that I should clean up and send a form parser I
> wrote for busybox to handle multi-part mime form posts in its
> webserver while I'm at it. That's something it could do with even if
> it's being frugal.

Oh we'll accept a suprising amount of functionality as long as it's the
smallest possible implementation and you can easily configure it out if you
don't want it.

We're starting to draw the line at servers, though. I personally think our
existing httpd/dhcpd/telnetd servers should probably be broken off into a
separate package. (We've wandered a touch beyond the mandate of standard
command line utilities, in places. An agenda item for version 1.2...)

> > But yeah, we're sticklers for correct behavior, and only attempt to
> > remount readonly if we get EACCES or EROFS, not _just_ because we
> > attempted a read/write mount and it failed. (And yes, I personally
> > tested this corner case. We haven't started on an automated regression
> > test script for mount yet because running it would require root access,
> > but it's on the todo list as we upgrade the test suite in our Copious
> > Free Time...)
>
> You looked at running this inside a qemu environment (scratchbox), huh?

I use User Mode Linux, actually. (I finally downloaded qemu a couple days
ago, to test non-x86 builds. But I could only build a crippled version at
the time, because ubuntu doesn't have the SDL headers installed either. It's
sort of development whack-a-mole...)

No, I'm building my own distro from source code, based on busybox and uClibc.
(And when I mean "based on busybox" I mean I'm using it to completely replace
bzip2, coreutils, e2fsprogs, file, findutils, gawk, grep, gzip, inetutils,
less, modutils, net-tools, patch, procps, sed, shadow, sysklogd, sysvinit,
tar, util-linux, and vim.) And yes, this is a complete self-hosting
development environment. (By self-hosting I mean capable of rebuilding
itself from source.)

You can download the build script here:
http://www.landley.net/code/firmware

Untar it and run build.sh (as a normal user, not as root), and it'll download
all the software packages it needs, build User Mode Linux, and run the rest
of the build under UML. The result is a self-contained "firmware-uml" you
can run to try it out. It's a User Mode Linux kernel with appended squashfs
root partion, which automatically loopback mounts itself during initramfs. A
bootable version is in the works. (I already have a patched lilo that'll
boot it, the hard part is making an installer.)

I've only got two gnu packages left other than development tools: I still need
to untangle busybox's command shell mess so I can stop using bash, and I need
to add "diff -u" so I can drop diffutils. (And while I'm at it, I need to
find another compiler to replace gcc and binutils, and find another make
implementation, at least as an _option_. Then I can ask Richard Stallman if
the resulting system is still "GNU/Linux", despite not using any GNU packages
or libraries even to build it. Ooh, software development to advance a
political agenda, a _totally_ unfair thing to pull on RMS...)

And yes, I've found (and fixed) eight gazillion bugs over the past two years
working on this system. You want to shake out all the bugs and weird corner
cases an implementation of sed/awk/sort? I mean after you've read through
the relevant Open Group base standard, line by line taking copious notes, and
implemented everything you can see? Try to get it to handle the ./configure
of binutils and gcc. Those suckers are _evil_.

And by evil I mean I had to add this to busybox sed to get the darn thing to
actually use it:

#define LIE_TO_AUTOCONF
#ifdef LIE_TO_AUTOCONF
if(argc==2 && !strcmp(argv[1],"--version")) {
printf("This is not GNU sed version 4.0\n");
exit(0);
}
#endif

You guessed it: the configure regexp checks for _GNU_ sed. Grrr...

> Jon.

Rob

2005-11-01 13:53:34

by Evgeny Stambulchik

[permalink] [raw]
Subject: Re: [PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk

Rob Landley wrote:

> But yeah, we're sticklers for correct behavior, and only attempt to remount
> readonly if we get EACCES or EROFS, not _just_ because we attempted a
> read/write mount and it failed.

BTW, busybox doesn't agree to do just mount -o remount,... /mountpoint,
it wants also either the device name passed or the entry be present in
fstab.

Regards,

Evgeny

2005-11-01 17:32:55

by Rob Landley

[permalink] [raw]
Subject: Re: [PATCH] fix floppy.c to store correct ro/rw status in underlying gendisk

On Tuesday 01 November 2005 07:53, Evgeny Stambulchik wrote:
> Rob Landley wrote:
> > But yeah, we're sticklers for correct behavior, and only attempt to
> > remount readonly if we get EACCES or EROFS, not _just_ because we
> > attempted a read/write mount and it failed.
>
> BTW, busybox doesn't agree to do just mount -o remount,... /mountpoint,
> it wants also either the device name passed or the entry be present in
> fstab.

Did you try the -devel version or 1.01? (I did about an 80% rewrite of this
sucker after 1.0, but it hasn't shipped yet. I bounced a 1.1.0-pre1 tarball
off of Erik and he said he'd put it up today, meanwhile grab the subversion
snapshot.)

Busybox -devel is way ahead of 1.01. Hopefully we'll have a new -stable
release by the end of the year...

Rob