Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1420763 > unrolled thread

Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7 Jiri Kosina <jikos@kernel.org> - 2016-06-13 14:20 +0200
    Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7 Wim Osterholt <wim@djo.tudelft.nl> - 2016-06-14 20:50 +0200
      Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7 Jiri Kosina <jikos@kernel.org> - 2016-06-15 09:10 +0200
        Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7 Wim Osterholt <wim@djo.tudelft.nl> - 2016-06-15 13:50 +0200
        Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7 Al Viro <viro@ZenIV.linux.org.uk> - 2016-06-15 15:30 +0200
          Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7 Jiri Kosina <jikos@kernel.org> - 2016-06-15 16:20 +0200
            Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7 Wim Osterholt <wim@djo.tudelft.nl> - 2016-06-16 00:50 +0200
              [PATCH] floppy: fix open(O_ACCMODE) for ioctl-only open Jiri Kosina <jikos@kernel.org> - 2016-06-16 10:00 +0200
            Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7 Wim Osterholt <wim@djo.tudelft.nl> - 2016-06-16 01:10 +0200
              Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7 Jiri Kosina <jikos@kernel.org> - 2016-06-16 01:20 +0200
              Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7 Joe Perches <joe@perches.com> - 2016-06-16 01:20 +0200
    Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7 Al Viro <viro@ZenIV.linux.org.uk> - 2016-06-14 21:10 +0200

#1420763 — Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7

On Sat, 11 Jun 2016, Wim Osterholt wrote:

> 
> up to vanilla kernel 4.4.13 floppy functionality performs like it should.
> (On an x86 PC that is. With a 1.44MB diskette drive.)
> >From kernel 4.5* and up it changed to barely usable.
> 
> After a virgin start (cold or warm boot) with an empty diskette drive and
> then loaded with a standard 720K diskette you may run 'mdir' (from mtools)
> and it shows the directory fine.
> The first time you load a standard 1.44MB diskette (wether it is a virgin
> start or after a 720K disktette) and you run mdir, it says literally:
> 
>  plain_io: Input/output error
>  init A: could not read boot sector
>  Cannot initialize 'A:'
> 
> After this, all subsequent runs of mdir will do fine on both floppies.
> However, most of my floppies are in a different format. (1.6MB)
> I rely on 'setfdprm' (from fdutils) to set the correct parameters.
> 
> setfdprm /dev/fd0 1600/1440
> /dev/fd0: Invalid argument
> 
> Strace shows me:
> ...
> open(/dev/fd0, O_ACCMODE) = -1
> 
> So this actually means that 'invalid argument' refers to O_ACCMODE.

Hmm, could you please test with 09954bad448 reverted? (although I don't 
really have a good explanation currently how it'd be causing what you are 
observing).

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [next] | [standalone]

#1422220

On Mon, Jun 13, 2016 at 02:15:15PM +0200, Jiri Kosina wrote:
> > up to vanilla kernel 4.4.13 floppy functionality performs like it should.
> > (On an x86 PC that is. With a 1.44MB diskette drive.)
> > >From kernel 4.5* and up it changed to barely usable.
> > 
> > After a virgin start (cold or warm boot) with an empty diskette drive and
> > then loaded with a standard 720K diskette you may run 'mdir' (from mtools)
> > and it shows the directory fine.
> > The first time you load a standard 1.44MB diskette (wether it is a virgin
> > start or after a 720K disktette) and you run mdir, it says literally:
> > 
> >  plain_io: Input/output error
> >  init A: could not read boot sector
> >  Cannot initialize 'A:'
> > 
> > After this, all subsequent runs of mdir will do fine on both floppies.
> > However, most of my floppies are in a different format. (1.6MB)
> > I rely on 'setfdprm' (from fdutils) to set the correct parameters.
> > 
> > setfdprm /dev/fd0 1600/1440
> > /dev/fd0: Invalid argument
> > 
> > Strace shows me:
> > ...
> > open(/dev/fd0, O_ACCMODE) = -1
> > 
> > So this actually means that 'invalid argument' refers to O_ACCMODE.
> 
> Hmm, could you please test with 09954bad448 reverted? (although I don't 
> really have a good explanation currently how it'd be causing what you are 
> observing).
> 
> Thanks,
> 
> -- 
> Jiri Kosina

Hmm. Now I need a crash course git.
After reading at www.kernel.org/pub/software/scm/git/docs/user-manual.html
I now tried:
git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
git checkout -b new v4.5  (assuming that the first 'wrong' kernel would be best)
git revert 09954bad448    (that did something, which I assume te be good)
copied the .config file from 4.5 I had lying around  and ran make.

Surprising or not, the thusly compiled kernel ran fine and I could handle
floppies like before!
(open(/dev/fd0,O_ACCMODE) succeeds.)


Regards, Wim.


----- wim@djo.tudelft.nl -----

[toc] | [prev] | [next] | [standalone]

#1422678

On Tue, 14 Jun 2016, Wim Osterholt wrote:

> Surprising or not, the thusly compiled kernel ran fine and I could 
> handle floppies like before! (open(/dev/fd0,O_ACCMODE) succeeds.)

Thanks for testing.

Now next question -- what do you actually want to achieve with passing 
O_ACCMODE to open()?

O_ACCMODE should primarily be used as a mask to use when extracting access 
mode bits from fcntl(F_GETFL) call.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1422931

On Wed, Jun 15, 2016 at 09:09:13AM +0200, Jiri Kosina wrote:
> > Surprising or not, the thusly compiled kernel ran fine and I could 
> > handle floppies like before! (open(/dev/fd0,O_ACCMODE) succeeds.)
> 
> Thanks for testing.
> 
> Now next question -- what do you actually want to achieve with passing 
> O_ACCMODE to open()?
> 
> O_ACCMODE should primarily be used as a mask to use when extracting access 
> mode bits from fcntl(F_GETFL) call.

It happens in setfdprm from fdutils. What they wanted to achieve with it
I don't know. Setting parameters or some such.
Problem is that fdutils is probably unmaintained for ten years or so.

Regards, Wim.


----- wim@djo.tudelft.nl -----

[toc] | [prev] | [next] | [standalone]

#1423014

On Wed, Jun 15, 2016 at 09:09:13AM +0200, Jiri Kosina wrote:
> On Tue, 14 Jun 2016, Wim Osterholt wrote:
> 
> > Surprising or not, the thusly compiled kernel ran fine and I could 
> > handle floppies like before! (open(/dev/fd0,O_ACCMODE) succeeds.)
> 
> Thanks for testing.
> 
> Now next question -- what do you actually want to achieve with passing 
> O_ACCMODE to open()?
> 
> O_ACCMODE should primarily be used as a mask to use when extracting access 
> mode bits from fcntl(F_GETFL) call.

ioctl-only open.  It's an old weird part of /dev/fd0 ABI and if you are
playing with that driver, you'd better bother to check the actual userland
talking to it.

Rationale, IIRC, is that unlike the normal open() this one does *not* depend
on formatted disk being there.  Regularizing it ot of existence is not
a good idea.

[toc] | [prev] | [next] | [standalone]

#1423068

On Wed, 15 Jun 2016, Al Viro wrote:

> ioctl-only open.  It's an old weird part of /dev/fd0 ABI 

Ah, right you are, I completely forgot about this gem.

> and if you are playing with that driver, 

I am merely trying to keep it in a state that doesn't crash the system.

> you'd better bother to check the actual userland talking to it.

Sure, this needs to be fixed.

Wim, could you please test whether the patch below, applied on top of 
vanilla kernel (i.e. drop the revert), everything you are using still 
works as expected?




From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] floppy: fix open(O_ACCMODE) for ioctl-only open

Commit 09954bad4 ("floppy: refactor open() flags handling"), as a side-effect,
causes open(/dev/fdX, O_ACCMODE) to fail. It turns out that this is being used
setfdprm userspace for ioctl-only open().

Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) 
modes, while still keeping the original O_NDELAY bug fixed.

Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/block/floppy.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 84708a5..a1dcf12 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3663,11 +3663,6 @@ 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) {
@@ -3711,13 +3706,15 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
 	if (UFDCS->rawcmd == 1)
 		UFDCS->rawcmd = 2;
 
-	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;
+	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;
 
-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1423565

On Wed, Jun 15, 2016 at 04:13:53PM +0200, Jiri Kosina wrote:
> 
> Wim, could you please test whether the patch below, applied on top of 
> vanilla kernel (i.e. drop the revert), everything you are using still 
> works as expected?
> 

Applied on kernel-4.7-rc3 it looks like it's working. (Strace setfdprm looks
good.) That is on a remote machine. An actual test on floppies must wait
until tomorrow when I get there.

Regards, Wim.


----- wim@djo.tudelft.nl -----

[toc] | [prev] | [next] | [standalone]

#1423761 — [PATCH] floppy: fix open(O_ACCMODE) for ioctl-only open

From: Jiri Kosina <jkosina@suse.cz>

Commit 09954bad4 ("floppy: refactor open() flags handling"), as a 
side-effect, causes open(/dev/fdX, O_ACCMODE) to fail. It turns out that 
this is being used setfdprm userspace for ioctl-only open().

Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) 
modes, while still keeping the original O_NDELAY bug fixed.

Cc: stable@vger.kernel.org # v4.5+
Reported-by: Wim Osterholt <wim@djo.tudelft.nl>
Tested-by: Wim Osterholt <wim@djo.tudelft.nl>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---

Jens, this should preferably go into 4.7-rcX and to -stable as well.

 drivers/block/floppy.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 84708a5..a1dcf12 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3663,11 +3663,6 @@ 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) {
@@ -3711,13 +3706,15 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)
 	if (UFDCS->rawcmd == 1)
 		UFDCS->rawcmd = 2;
 
-	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;
+	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;
 
-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1423570

On my first message I stated:

 It looks to me that the code in floppy.c is quite old; no changes here.
 So the bug is elsewhere in the kernel.
 
That was because the changelog at the beginning of floppy.c ended in 2003.
Wouln't it be wise to keep these items updated?

Groeten, Wim.


----- wim@djo.tudelft.nl -----

[toc] | [prev] | [next] | [standalone]

#1423576

On Thu, 16 Jun 2016, Wim Osterholt wrote:

> That was because the changelog at the beginning of floppy.c ended in 2003.
> Wouln't it be wise to keep these items updated?

Those things have only historical value these days. The real changelog has 
been kept in git (formerly bitkeeper) changelogs for past ~15 years.

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1423577

On Thu, 2016-06-16 at 01:07 +0200, Wim Osterholt wrote:
> On my first message I stated:
> 
>  It looks to me that the code in floppy.c is quite old; no changes here.
>  So the bug is elsewhere in the kernel.
>  
> That was because the changelog at the beginning of floppy.c ended in 2003.
> Wouln't it be wise to keep these items updated?

Not really.

git commit log entries are a better place because they
can be relatively free-form, detailed and verbose.

[toc] | [prev] | [next] | [standalone]

#1422237

On Mon, Jun 13, 2016 at 02:15:15PM +0200, Jiri Kosina wrote:

> Hmm, could you please test with 09954bad448 reverted? (although I don't 
> really have a good explanation currently how it'd be causing what you are 
> observing).

I do, actually - ->f_mode on open(..., 3) contains neither FMODE_READ nor
FMODE_WRITE.  So this
    While at it, clean up a bit handling of !(mode & (FMODE_READ|FMODE_WRITE))
    case and return EINVAL instead of succeeding as well.
is working as promised in commit message.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web