disfunctional floppy driver in kernels 4.5, 4.6 and 4.7
Subsystems (2)
L.S.
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.)
Quoted 1 line
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.
At all kernels I see no changed rights in:
ls -al /dev/fd0
brw-rw---- 1 root disk 2, 0 May 13 16:43 /dev/fd0
It looks to me that the code in floppy.c is quite old; no changes here. So the bug is elsewhere in the kernel.
Could someone please explain and repair the magic that is happening here?
Thanks in advance, Wim Osterholt.
----- [email protected] -----
VFS regression ? Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7
Quoted 20 lines · On Sat, 11 Jun 2016 01:02:55 +0200 Wim Osterholt <[email protected]> wrote:
L.S.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.)
Quoted 1 line
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.
Quoted 8 lines
setfdprm
/dev/fd01600/1440/dev/fd0: Invalid argumentStrace shows me:
...open(/dev/fd0, O_ACCMODE)= -1So this actually means that 'invalid argument' refers to
O_ACCMODE.
That looks like a bug introduced in the VFS layer rather than in the floppy driver. A couple of Linux drivers have a weird un-Unixy hack where you open them with a mode value of 3 to mean "control only" and /dev/fd is the main user of this
If you do
touch foo
then compile and run the following program does it error on the newer kernel ?
#include <fcntl.h>
#include <stdio.h>
int main(int argc, char *argv[])
{
if (open("foo", 3) == -1)
perror("foo"); return 0;
}
Alan
Re: VFS regression ? Re: disfunctional floppy driver in kernels 4.5, 4.6 and 4.7
Quoted 12 lines · One Thousand Gnomes
Quoted 1 line
open(/dev/fd0, O_ACCMODE)= -1If you do
touch foo
then compile and run the following program does it error on the newer kernel ?
#include <
fcntl.h>
#include <stdio.h>
Quoted 6 lines
int
main(int argc, char *argv[])
{
if (open("foo", 3)== -1)perror("foo");return 0;
}
No errors get printed for kernel 4.4 to 4.7 .
Regards, Wim.
----- [email protected] -----
Quoted 19 lines · Wim Osterholt
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.)
Quoted 1 line
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.
Quoted 8 lines
setfdprm
/dev/fd01600/1440/dev/fd0: Invalid argumentStrace shows me:
...open(/dev/fd0, O_ACCMODE)= -1So 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,
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 25 lines · Jiri Kosina
Quoted 25 lines
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.)
Quoted 1 line
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/fd01600/1440/dev/fd0: Invalid argumentStrace shows me:
...open(/dev/fd0, O_ACCMODE)= -1
Quoted 10 lines
Quoted 1 line
So this actually means that 'invalid argument' refers to
O_ACCMODE.Hmm, could you please test with
09954bad448reverted? (although I don't really have a good explanation currently how it'd be causing what you are observing).Thanks,
Signature · 1 line
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.
----- [email protected] -----
Quoted 2 lines · Wim Osterholt
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.
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 10 lines · Jiri Kosina
Quoted 2 lines
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_ACCMODEtoopen()?
O_ACCMODEshould primarily be used as a mask to use when extracting access mode bits fromfcntl(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.
----- [email protected] -----
Quoted 5 lines · Jiri Kosina
Quoted 2 lines · Wim Osterholt
Surprising or not, the thusly compiled kernel ran fine and I could handle floppies like before! (
open(/dev/fd0,O_ACCMODE)succeeds.)
Quoted 7 lines
Thanks for testing.
Now next question -- what do you actually want to achieve with passing
O_ACCMODEtoopen()?
O_ACCMODEshould primarily be used as a mask to use when extracting access mode bits fromfcntl(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.
Quoted 1 line · Al Viro
ioctl-only open. It's an old weird part of
/dev/fd0ABI
Ah, right you are, I completely forgot about this gem.
Quoted 1 line
and if you are playing with that driver,
I am merely trying to keep it in a state that doesn't crash the system.
Quoted 1 line
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 <[email protected]>
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.
--- drivers/block/floppy.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
drivers/block/floppy.c +9 -12
@@ -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 ;
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 5 lines · Jiri Kosina
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.
----- [email protected] -----
PATCH 1/1 floppy: fix open(O_ACCMODE) for ioctl-only open
From: Jiri Kosina <[email protected]>
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.
---
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(-)
drivers/block/floppy.c +9 -12
@@ -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 ;
Signature · 2 lines
Jiri Kosina SUSE Labs
PATCH RESEND 1/1 floppy: fix open(O_ACCMODE) for ioctl-only open
From: Jiri Kosina <[email protected]>
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.
---
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(-)
drivers/block/floppy.c +9 -12
@@ -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 ;
Signature · 2 lines
Jiri Kosina SUSE Labs
Re: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
Quoted 9 lines · Jiri Kosina
From: Jiri Kosina <[email protected]>
Commit
09954bad4("floppy: refactoropen()flags handling"), as a side-effect, causesopen(/dev/fdX, O_ACCMODE)to fail. It turns out that this is being used setfdprm userspace for ioctl-onlyopen().Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) modes, while still keeping the original
O_NDELAYbug fixed.
Quoted 8 lines
Cc [email protected] # v4.5+Reported-by Wim Osterholt <[email protected]>Tested-by Wim Osterholt <[email protected]>Signed-off-by Jiri Kosina <[email protected]>---
Jens, this should preferably go into 4.7-rcX and to -stable as well.
How come this patch is still not in the recent release of 4.7 ?
Regards, Wim.
Jens Axboe
maintainerRe: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
In reply to Jiri KosinaQuoted 6 lines · Jiri Kosina
From: Jiri Kosina <[email protected]>
Commit
09954bad4("floppy: refactoropen()flags handling"), as a side-effect, causesopen(/dev/fdX, O_ACCMODE)to fail. It turns out that this is being used setfdprm userspace for ioctl-onlyopen().
Quoted 7 lines
Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) modes, while still keeping the original
O_NDELAYbug fixed.Cc [email protected] # v4.5+Reported-by Wim Osterholt <[email protected]>Tested-by Wim Osterholt <[email protected]>Signed-off-by Jiri Kosina <[email protected]>
Added for this series, thanks.
Signature · 1 line
Jens Axboe
Re: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
Quoted 9 lines · Jens Axboe
Quoted 9 lines
From: Jiri Kosina <[email protected]>
Commit
09954bad4("floppy: refactoropen()flags handling"), as a side-effect, causesopen(/dev/fdX, O_ACCMODE)to fail. It turns out that this is being used setfdprm userspace for ioctl-onlyopen().Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) modes, while still keeping the original
O_NDELAYbug fixed.
Quoted 6 lines
Quoted 4 lines
Cc [email protected] # v4.5+Reported-by Wim Osterholt <[email protected]>Tested-by Wim Osterholt <[email protected]>Signed-off-by Jiri Kosina <[email protected]>Added for this series, thanks.
[ CCing Denis too ]
Let me revive this 4 years old thread.
I've just now noticed that instead of my patch above being merged, what happened instead was
commit f2791e7eadf437633f30faa51b30878cf15650be Author: Jens Axboe <[email protected]> Date: Thu Aug 25 08:56:51 2016 -0600 Revert "floppy: refactor open() flags handling" This reverts commit 09954bad448791ef01202351d437abdd9497a804.
which was plain revert of 09954bad4 (without any further explanation), which in turn reintroduced the O_NDELAY issue, and I've just been hit by it again.
I am not able to find any e-mail thread that'd indicate why ultimately revert happened, instead of mergin my fix.
Jens, do you have any idea?
Thanks,
Signature · 2 lines
Jiri Kosina SUSE Labs
Denis Efremov
maintainerRe: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
Hi,
Quoted 34 lines · Jiri Kosina
Quoted 9 lines · Jens Axboe
Quoted 9 lines
From: Jiri Kosina <[email protected]>
Commit
09954bad4("floppy: refactoropen()flags handling"), as a side-effect, causesopen(/dev/fdX, O_ACCMODE)to fail. It turns out that this is being used setfdprm userspace for ioctl-onlyopen().Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) modes, while still keeping the original
O_NDELAYbug fixed.Quoted 6 lines
Quoted 4 lines
Cc [email protected] # v4.5+Reported-by Wim Osterholt <[email protected]>Tested-by Wim Osterholt <[email protected]>Signed-off-by Jiri Kosina <[email protected]>Added for this series, thanks.
[ CCing Denis too ]
Let me revive this 4 years old thread.
I've just now noticed that instead of my patch above being merged, what happened instead was
commit f2791e7eadf437633f30faa51b30878cf15650be Author: Jens Axboe <[email protected]> Date: Thu Aug 25 08:56:51 2016 -0600 Revert "floppy: refactor open() flags handling" This reverts commit 09954bad448791ef01202351d437abdd9497a804.
Quoted 6 lines
which was plain revert of
09954bad4(without any further explanation), which in turn reintroduced theO_NDELAYissue, and I've just been hit by it again.I am not able to find any e-mail thread that'd indicate why ultimately revert happened, instead of mergin my fix.
I think it's hard to recall the exact reasons after so many years. I'll send a patch today based on this one.
Best Regards,
Denis
Jiri Kosina
Tested-by Wim OsterholtRe: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
Quoted 1 line · Denis Efremov
I think it's hard to recall the exact reasons after so many years.
Yeah, I guess so :)
Quoted 1 line
I'll send a patch today based on this one.
I am currently waiting for confirmation by the original reporter that the patch below fixes the issue.
From: Jiri Kosina <[email protected]>
Subject: [PATCH] floppy: reintroduce O_NDELAY fix
Originally fixed in 09954bad4 ("floppy: refactor open() flags handling") then reverted for unknown reason in f2791e7eadf437 instead of taking the open(O_ACCMODE) for ioctl-only open fix, which had the changelog below
====
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.
=====
--- drivers/block/floppy.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)
drivers/block/floppy.c +14 -15
@@ -4121,23 +4121,22 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)if (fdc_state[FDC (drive)].rawcmd ==1 ) fdc_state[FDC (drive)].rawcmd =2 ; -if (!(mode &FMODE_NDELAY )) { -if (mode & (FMODE_READ |FMODE_WRITE )) { - drive_state[drive].last_checked =0 ; -clear_bit (FD_OPEN_SHOULD_FAIL_BIT , - &drive_state[drive].flags); -if (bdev_check_media_change (bdev)) -floppy_revalidate (bdev->bd_disk); -if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) - goto out; -if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) - goto out; - } - res = -EROFS ; -if ((mode &FMODE_WRITE ) && - !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) +if (mode & (FMODE_READ |FMODE_WRITE )) { +UDRS ->last_checked =0 ; +clear_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags); +check_disk_change (bdev); +if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) + goto out; +if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) goto out; } + + res = -EROFS ; + +if ((mode &FMODE_WRITE ) && + !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) + goto out; +mutex_unlock (&open_lock);mutex_unlock (&floppy_mutex);return 0 ;
Signature · 2 lines
Jiri Kosina SUSE Labs
kernel test robot
build botRe: [PATCH] floppy: reintroduce O_NDELAY fix
I love your patch! Yet something to improve:
Hi Jiri,
I love your patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on linux/master linus/master v5.11-rc4 next-20210121] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Jiri-Kosina/floppy-reintroduce-O_NDELAY-fix/20210121-182951 base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next config: sparc64-randconfig-r033-20210121 (attached as .config) compiler: sparc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/605da67173ab7c362845b2f74c2914bfcec6db2e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Jiri-Kosina/floppy-reintroduce-O_NDELAY-fix/20210121-182951 git checkout 605da67173ab7c362845b2f74c2914bfcec6db2e
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc64
If you fix the issue, kindly add following tag as appropriate
All errors (new ones prefixed by >>):
In file included from arch/sparc/include/asm/floppy.h:5,
from drivers/block/floppy.c:251:
arch/sparc/include/asm/floppy_64.h:200:13: warning: no previous prototype for 'sparc_floppy_irq' [-Wmissing-prototypes]
200 | irqreturn_t sparc_floppy_irq(int irq, void *dev_cookie)
| ^~~~~~~~~~~~~~~~
In file included from arch/sparc/include/asm/floppy.h:5,
from drivers/block/floppy.c:251:
arch/sparc/include/asm/floppy_64.h:437:6: warning: no previous prototype for 'sun_pci_fd_dma_callback' [-Wmissing-prototypes]
437 | void sun_pci_fd_dma_callback(struct ebus_dma_info *p, int event, void *cookie)
| ^~~~~~~~~~~~~~~~~~~~~~~
drivers/block/floppy.c: In function 'floppy_open':
>> drivers/block/floppy.c:4125:3: error: 'UDRS' undeclared (first use in this function)
4125 | UDRS->last_checked = 0;
| ^~~~
drivers/block/floppy.c:4125:3: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/block/floppy.c:4127:3: error: implicit declaration of function 'check_disk_change'; did you mean 'bdev_disk_changed'? [-Werror=implicit-function-declaration]
4127 | check_disk_change(bdev);
| ^~~~~~~~~~~~~~~~~
| bdev_disk_changedcc1: some warnings being treated as errors
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for COMPAT_BINFMT_ELF
Depends on COMPAT && BINFMT_ELF
Selected by
- COMPAT && SPARC64
WARNING: unmet direct dependencies detected for FRAME_POINTER
Depends on DEBUG_KERNEL && (M68K || UML || SUPERH) || ARCH_WANT_FRAME_POINTERS || MCOUNT
Selected by
- LOCKDEP && DEBUG_KERNEL && LOCK_DEBUGGING_SUPPORT && !MIPS && !PPC && !ARM && !S390 && !MICROBLAZE && !ARC && !X86
vim +/UDRS +4125 drivers/block/floppy.c
4052
4053 /*
4054 * floppy_open check for aliasing (/dev/fd0 can be the same as
4055 * /dev/PS0 etc), and disallows simultaneous access to the same
4056 * drive with different device numbers.
4057 */
4058 static int floppy_open(struct block_device *bdev, fmode_t mode)
4059 {
4060 int drive = (long)bdev->bd_disk->private_data;
4061 int old_dev, new_dev;
4062 int try;
4063 int res = -EBUSY;
4064 char *tmp;
4065
4066 mutex_lock(&floppy_mutex);
4067 mutex_lock(&open_lock);
4068 old_dev = drive_state[drive].fd_device;
4069 if (opened_bdev[drive] && opened_bdev[drive] != bdev)
4070 goto out2;
4071
4072 if (!drive_state[drive].fd_ref && (drive_params[drive].flags & FD_BROKEN_DCL)) {
4073 set_bit(FD_DISK_CHANGED_BIT, &drive_state[drive].flags);
4074 set_bit(FD_VERIFY_BIT, &drive_state[drive].flags);
4075 }
4076
4077 drive_state[drive].fd_ref++;
4078
4079 opened_bdev[drive] = bdev;
4080
4081 res = -ENXIO;
4082
4083 if (!floppy_track_buffer) {
4084 /* if opening an ED drive, reserve a big buffer,
4085 * else reserve a small one */
4086 if ((drive_params[drive].cmos == 6) || (drive_params[drive].cmos == 5))
4087 try = 64; /* Only 48 actually useful */
4088 else
4089 try = 32; /* Only 24 actually useful */
4090
4091 tmp = (char *)fd_dma_mem_alloc(1024 * try);
4092 if (!tmp && !floppy_track_buffer) {
4093 try >>= 1; /* buffer only one side */
4094 INFBOUND(try, 16);
4095 tmp = (char *)fd_dma_mem_alloc(1024 * try);
4096 }
4097 if (!tmp && !floppy_track_buffer)
4098 fallback_on_nodma_alloc(&tmp, 2048 * try);
4099 if (!tmp && !floppy_track_buffer) {
4100 DPRINT("Unable to allocate DMA memory\n");
4101 goto out;
4102 }
4103 if (floppy_track_buffer) {
4104 if (tmp)
4105 fd_dma_mem_free((unsigned long)tmp, try * 1024);
4106 } else {
4107 buffer_min = buffer_max = -1;
4108 floppy_track_buffer = tmp;
4109 max_buffer_sectors = try;
4110 }
4111 }
4112
4113 new_dev = MINOR(bdev->bd_dev);
4114 drive_state[drive].fd_device = new_dev;
4115 set_capacity(disks[drive][ITYPE(new_dev)], floppy_sizes[new_dev]);
4116 if (old_dev != -1 && old_dev != new_dev) {
4117 if (buffer_drive == drive)
4118 buffer_track = -1;
4119 }
4120
4121 if (fdc_state[FDC(drive)].rawcmd == 1)
4122 fdc_state[FDC(drive)].rawcmd = 2;
4123
4124 if (mode & (FMODE_READ|FMODE_WRITE)) {
> 4125 UDRS->last_checked = 0; 4126 clear_bit(FD_OPEN_SHOULD_FAIL_BIT, &drive_state[drive].flags); > 4127 check_disk_change(bdev);
4128 if (test_bit(FD_DISK_CHANGED_BIT, &drive_state[drive].flags))
4129 goto out;
4130 if (test_bit(FD_OPEN_SHOULD_FAIL_BIT, &drive_state[drive].flags))
4131 goto out;
4132 }
4133
4134 res = -EROFS;
4135
4136 if ((mode & FMODE_WRITE) &&
4137 !test_bit(FD_DISK_WRITABLE_BIT, &drive_state[drive].flags))
4138 goto out;
4139
4140 mutex_unlock(&open_lock);
4141 mutex_unlock(&floppy_mutex);
4142 return 0;
4143 out:
4144 drive_state[drive].fd_ref--;
4145
4146 if (!drive_state[drive].fd_ref)
4147 opened_bdev[drive] = NULL;
4148 out2:
4149 mutex_unlock(&open_lock);
4150 mutex_unlock(&floppy_mutex);
4151 return res;
4152 }
4153
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
Jiri Kosina
Tested-by Wim OsterholtRe: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
In reply to Jiri KosinaQuoted 2 lines · Jiri Kosina
I am currently waiting for confirmation by the original reporter that the patch below fixes the issue.
... a now a patch that actually compiles :) (made a mistake when forward-porting from the older kernel on which this has been reported).
From: Jiri Kosina <[email protected]>
Subject: [PATCH v2] floppy: reintroduce O_NDELAY fix
Originally fixed in 09954bad4 ("floppy: refactor open() flags handling") then reverted for unknown reason in f2791e7eadf437 instead of taking the open(O_ACCMODE) for ioctl-only open fix, which had the changelog below
====
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.
=====
---
v1 -> v2: fix build issue due to bad forward-port
drivers/block/floppy.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)
drivers/block/floppy.c +15 -15
@@ -4121,23 +4121,23 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)if (fdc_state[FDC (drive)].rawcmd ==1 ) fdc_state[FDC (drive)].rawcmd =2 ; -if (!(mode &FMODE_NDELAY )) { -if (mode & (FMODE_READ |FMODE_WRITE )) { - drive_state[drive].last_checked =0 ; -clear_bit (FD_OPEN_SHOULD_FAIL_BIT , - &drive_state[drive].flags); -if (bdev_check_media_change (bdev)) -floppy_revalidate (bdev->bd_disk); -if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) - goto out; -if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) - goto out; - } - res = -EROFS ; -if ((mode &FMODE_WRITE ) && - !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) +if (mode & (FMODE_READ |FMODE_WRITE )) { +UDRS ->last_checked =0 ; +clear_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags); +if (bdev_check_media_change (bdev)) +floppy_revalidate (bdev->bd_disk); +if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) + goto out; +if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) goto out; } + + res = -EROFS ; + +if ((mode &FMODE_WRITE ) && + !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) + goto out; +mutex_unlock (&open_lock);mutex_unlock (&floppy_mutex);return 0 ;
Signature · 2 lines
Jiri Kosina SUSE Labs
Denis Efremov
maintainerRe: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
Quoted 7 lines · Jiri Kosina
Quoted 2 lines · Jiri Kosina
I am currently waiting for confirmation by the original reporter that the patch below fixes the issue.
... a now a patch that actually compiles :) (made a mistake when forward-porting from the older kernel on which this has been reported).
Oh, sorry for the last message (forgot to check the inbox before hitting the send button). I'll test the patch. A couple of nitpicks below.
Quoted 13 lines
From: Jiri Kosina <[email protected]>
Subject: [PATCH v2] floppy: reintroduceO_NDELAYfixOriginally fixed in
09954bad4("floppy: refactoropen()flags handling") then reverted for unknown reason inf2791e7eadf437instead of taking theopen(O_ACCMODE)for ioctl-only open fix, which had the changelog below====
Commit09954bad4("floppy: refactoropen()flags handling"), as a side-effect, causesopen(/dev/fdX, O_ACCMODE)to fail. It turns out that this is being used setfdprm userspace for ioctl-onlyopen().
Quoted 4 lines
Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) modes, while still keeping the original
O_NDELAYbug fixed.Cc [email protected] # v4.5+
Are you sure that it's not worth to backport it to LTS v4.4? Because f2791e7ead is just a revert and 09954bad4 is not presented in v4.4 I'm not sure what fixes tag is better to use in this case.
Quoted 30 lines
Reported-by Wim Osterholt <[email protected]>Tested-by Wim Osterholt <[email protected]>Signed-off-by Jiri Kosina <[email protected]>=====
Fixes 09954bad4 ("floppy: refactor open() flags handling")Fixes f2791e7ead ("Revert "floppy: refactor open() flags handling"")Signed-off-by Jiri Kosina <[email protected]>---
v1 -> v2: fix build issue due to bad forward-port
drivers/block/floppy.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)drivers/block/floppy.c +0
-7@@ -4121,23 +4121,23 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)if (fdc_state[FDC (drive)].rawcmd ==1 ) fdc_state[FDC (drive)].rawcmd =2 ; -if (!(mode &FMODE_NDELAY )) { -if (mode & (FMODE_READ |FMODE_WRITE )) { - drive_state[drive].last_checked =0 ; -clear_bit (FD_OPEN_SHOULD_FAIL_BIT , - &drive_state[drive].flags); -if (bdev_check_media_change (bdev)) -floppy_revalidate (bdev->bd_disk);
drivers/block/floppy.c static int floppy_open(struct block_device *bdev, fmode_t mode) · line 4125
Quoted 10 lines
drivers/block/floppy.c +2
-8-if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) - goto out; -if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) - goto out; - } - res = -EROFS ; -if ((mode &FMODE_WRITE ) && - !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) +if (mode & (FMODE_READ |FMODE_WRITE )) { +UDRS ->last_checked =0 ;
UDRS will still break the compilation here.
Quoted 19 lines
drivers/block/floppy.c +13
-0+clear_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags); +if (bdev_check_media_change (bdev)) +floppy_revalidate (bdev->bd_disk); +if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) + goto out; +if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) goto out; } + + res = -EROFS ; + +if ((mode &FMODE_WRITE ) && + !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) + goto out; +mutex_unlock (&open_lock);mutex_unlock (&floppy_mutex);return 0 ;
Re: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
Quoted 12 lines · Denis Efremov
Quoted 12 lines
From: Jiri Kosina <[email protected]>
Subject: [PATCH v2] floppy: reintroduceO_NDELAYfixOriginally fixed in
09954bad4("floppy: refactoropen()flags handling") then reverted for unknown reason inf2791e7eadf437instead of taking theopen(O_ACCMODE)for ioctl-only open fix, which had the changelog below====
Commit09954bad4("floppy: refactoropen()flags handling"), as a side-effect, causesopen(/dev/fdX, O_ACCMODE)to fail. It turns out that this is being used setfdprm userspace for ioctl-onlyopen().
Quoted 8 lines
Quoted 4 lines
Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) modes, while still keeping the original
O_NDELAYbug fixed.Cc [email protected] # v4.5+Are you sure that it's not worth to backport it to LTS v4.4? Because
f2791e7eadis just a revert and09954bad4is not presented in v4.4 I'm not sure what fixes tag is better to use in this case.
You are right; I'll drop the '4.5+' indicator and will backport it once/if it hits Linus' tree.
Quoted 4 lines
drivers/block/floppy.c static int floppy_open(struct block_device *bdev, fmode_t mode) · line 4125
Quoted 2 lines
drivers/block/floppy.c +2
-0+if (mode & (FMODE_READ |FMODE_WRITE )) { +UDRS ->last_checked =0 ;UDRS will still break the compilation here.
Doh, forgot to refresh before sending, sorry for the noise. I'll send the final version once I get confirmation from the reporter that it's fixing the issue properly, add his Reported-by: etc.
Thanks,
Signature · 2 lines
Jiri Kosina SUSE Labs
PATCH 1/1 floppy: reintroduce O_NDELAY fix
From: Jiri Kosina <[email protected]>
This issue was originally fixed in 09954bad4 ("floppy: refactor open() flags handling").
The fix as a side-effect, however, introduce issue for open(O_ACCMODE) that is being used for ioctl-only open. I wrote a fix for that, but instead of it being merged, full revert of 09954bad4 was performed, re-introducing the O_NDELAY / O_NONBLOCK issue, and it strikes again.
This is a forward-port of the original fix to current codebase; the original submission had the changelog below:
====
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.
--- drivers/block/floppy.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)
drivers/block/floppy.c +15 -15
@@ -4121,23 +4121,23 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)if (fdc_state[FDC (drive)].rawcmd ==1 ) fdc_state[FDC (drive)].rawcmd =2 ; -if (!(mode &FMODE_NDELAY )) { -if (mode & (FMODE_READ |FMODE_WRITE )) { - drive_state[drive].last_checked =0 ; -clear_bit (FD_OPEN_SHOULD_FAIL_BIT , - &drive_state[drive].flags); -if (bdev_check_media_change (bdev)) -floppy_revalidate (bdev->bd_disk); -if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) - goto out; -if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) - goto out; - } - res = -EROFS ; -if ((mode &FMODE_WRITE ) && - !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) +if (mode & (FMODE_READ |FMODE_WRITE )) { + drive_state[drive].last_checked =0 ; +clear_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags); +if (bdev_check_media_change (bdev)) +floppy_revalidate (bdev->bd_disk); +if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) + goto out; +if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) goto out; } + + res = -EROFS ; + +if ((mode &FMODE_WRITE ) && + !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) + goto out; +mutex_unlock (&open_lock);mutex_unlock (&floppy_mutex);return 0 ;
Signature · 2 lines
Jiri Kosina SUSE Labs
Re: [PATCH] floppy: reintroduce O_NDELAY fix
Quoted 21 lines · Jiri Kosina
From: Jiri Kosina <[email protected]>
This issue was originally fixed in
09954bad4("floppy: refactoropen()flags handling").The fix as a side-effect, however, introduce issue for
open(O_ACCMODE)that is being used for ioctl-only open. I wrote a fix for that, but instead of it being merged, full revert of09954bad4was performed, re-introducing theO_NDELAY/O_NONBLOCKissue, and it strikes again.This is a forward-port of the original fix to current codebase; the original submission had the changelog below:
====
Commit09954bad4("floppy: refactoropen()flags handling"), as a side-effect, causesopen(/dev/fdX, O_ACCMODE)to fail. It turns out that this is being used setfdprm userspace for ioctl-onlyopen().Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) modes, while still keeping the original
O_NDELAYbug fixed.
Quoted 7 lines
Reported-by Wim Osterholt <[email protected]>Tested-by Wim Osterholt <[email protected]>Reported-and-tested-by Kurt Garloff <[email protected]>Fixes 09954bad4 ("floppy: refactor open() flags handling")Fixes f2791e7ead ("Revert "floppy: refactor open() flags handling"")Signed-off-by Jiri Kosina <[email protected]>
Applied. I'll send it to Jens soon with a couple of cleanup patches.
https://github.com/evdenis/linux-floppy/commit/e32f6163c47efbdbad06258560aa00d1c7e5b699
Thanks,
Denis
Quoted 48 lines
--- drivers/block/floppy.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)drivers/block/floppy.c +15
-15@@ -4121,23 +4121,23 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)if (fdc_state[FDC (drive)].rawcmd ==1 ) fdc_state[FDC (drive)].rawcmd =2 ; -if (!(mode &FMODE_NDELAY )) { -if (mode & (FMODE_READ |FMODE_WRITE )) { - drive_state[drive].last_checked =0 ; -clear_bit (FD_OPEN_SHOULD_FAIL_BIT , - &drive_state[drive].flags); -if (bdev_check_media_change (bdev)) -floppy_revalidate (bdev->bd_disk); -if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) - goto out; -if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) - goto out; - } - res = -EROFS ; -if ((mode &FMODE_WRITE ) && - !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) +if (mode & (FMODE_READ |FMODE_WRITE )) { + drive_state[drive].last_checked =0 ; +clear_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags); +if (bdev_check_media_change (bdev)) +floppy_revalidate (bdev->bd_disk); +if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) + goto out; +if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) goto out; } + + res = -EROFS ; + +if ((mode &FMODE_WRITE ) && + !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) + goto out; +mutex_unlock (&open_lock);mutex_unlock (&floppy_mutex);return 0 ;
Re: [PATCH] floppy: reintroduce O_NDELAY fix
Hi Denis, Jiri, Jens,
Am 26.01.21 um 09:21 schrieb Denis Efremov:
Quoted 29 lines · Jiri Kosina
Quoted 21 lines
From: Jiri Kosina <[email protected]>
This issue was originally fixed in
09954bad4("floppy: refactoropen()flags handling").The fix as a side-effect, however, introduce issue for
open(O_ACCMODE)that is being used for ioctl-only open. I wrote a fix for that, but instead of it being merged, full revert of09954bad4was performed, re-introducing theO_NDELAY/O_NONBLOCKissue, and it strikes again.This is a forward-port of the original fix to current codebase; the original submission had the changelog below:
====
Commit09954bad4("floppy: refactoropen()flags handling"), as a side-effect, causesopen(/dev/fdX, O_ACCMODE)to fail. It turns out that this is being used setfdprm userspace for ioctl-onlyopen().Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) modes, while still keeping the original
O_NDELAYbug fixed.Quoted 7 lines
Reported-by Wim Osterholt <[email protected]>Tested-by Wim Osterholt <[email protected]>Reported-and-tested-by Kurt Garloff <[email protected]>Fixes 09954bad4 ("floppy: refactor open() flags handling")Fixes f2791e7ead ("Revert "floppy: refactor open() flags handling"")Signed-off-by Jiri Kosina <[email protected]>
Quoted 3 lines
Applied. I'll send it to Jens soon with a couple of cleanup patches.
https://github.com/evdenis/linux-floppy/commit/e32f6163c47efbdbad06258560aa00d1c7e5b699
Great, thanks.
Due to libblkid (rightfully) using O_NONBLOCK these days when probing
devices, the floppy driver does spit loads of
[ 9.533513] floppy0: disk absent or changed during operation
[ 9.534989] blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0 [ 9.537206] Buffer I/O error on dev fd0, logical block 0, async page read
[ 9.546837] floppy0: disk absent or changed during operation
[ 9.548389] blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 and fails a mount prior to being opened without O_NONBLOCK at least once.
(Reproduction is easy with qemu-kvm.)
The patch addresses it and I would suggest it to also be backported and applied to the active stable kernel trees.
Thanks,
Signature · 1 line
Kurt Garloff <[email protected]>, Cologne, Germany
Denis Efremov
maintainerRe: [PATCH] floppy: reintroduce O_NDELAY fix
Quoted 48 lines · Kurt Garloff
Hi Denis, Jiri, Jens,
Am 26.01.21 um 09:21 schrieb Denis Efremov:
Quoted 29 lines · Jiri Kosina
Quoted 21 lines
From: Jiri Kosina <[email protected]>
This issue was originally fixed in
09954bad4("floppy: refactoropen()flags handling").The fix as a side-effect, however, introduce issue for
open(O_ACCMODE)that is being used for ioctl-only open. I wrote a fix for that, but instead of it being merged, full revert of09954bad4was performed, re-introducing theO_NDELAY/O_NONBLOCKissue, and it strikes again.This is a forward-port of the original fix to current codebase; the original submission had the changelog below:
====
Commit09954bad4("floppy: refactoropen()flags handling"), as a side-effect, causesopen(/dev/fdX, O_ACCMODE)to fail. It turns out that this is being used setfdprm userspace for ioctl-onlyopen().Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) modes, while still keeping the original
O_NDELAYbug fixed.Quoted 7 lines
Reported-by Wim Osterholt <[email protected]>Tested-by Wim Osterholt <[email protected]>Reported-and-tested-by Kurt Garloff <[email protected]>Fixes 09954bad4 ("floppy: refactor open() flags handling")Fixes f2791e7ead ("Revert "floppy: refactor open() flags handling"")Signed-off-by Jiri Kosina <[email protected]>Quoted 3 lines
Applied. I'll send it to Jens soon with a couple of cleanup patches.
https://github.com/evdenis/linux-floppy/commit/e32f6163c47efbdbad06258560aa00d1c7e5b699
Great, thanks.
Due to libblkid (rightfully) using
O_NONBLOCKthese days when probing
devices, the floppy driver does spit loads of
[ 9.533513] floppy0: disk absent or changed during operation
[ 9.534989]blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags0x0phys_seg1 prio class 0 [ 9.537206] Buffer I/O error on dev fd0, logical block 0, async page read
[ 9.546837] floppy0: disk absent or changed during operation
[ 9.548389]blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags0x80700phys_seg1 and fails a mount prior to being opened withoutO_NONBLOCKat least once.
(Reproduction is easy with qemu-kvm.)
Quoted 2 lines
The patch addresses it and I would suggest it to also be backported and applied to the active stable kernel trees.
Yes, it will be backported to all stable trees including v4.4
Thanks,
Denis
Re: [PATCH] floppy: reintroduce O_NDELAY fix
In reply to Denis EfremovQuoted 3 lines · Denis Efremov
Applied. I'll send it to Jens soon with a couple of cleanup patches.
https://github.com/evdenis/linux-floppy/commit/e32f6163c47efbdbad06258560aa00d1c7e5b699
Denis,
I don't see this fix in Jens' tree yet. Is there any problem with the patch?
Thanks,
Signature · 2 lines
Jiri Kosina SUSE Labs
Denis Efremov
maintainerRe: [PATCH] floppy: reintroduce O_NDELAY fix
Quoted 10 lines · Jiri Kosina
Quoted 3 lines · Denis Efremov
Applied. I'll send it to Jens soon with a couple of cleanup patches.
https://github.com/evdenis/linux-floppy/commit/e32f6163c47efbdbad06258560aa00d1c7e5b699
Denis,
I don't see this fix in Jens' tree yet. Is there any problem with the patch?
Hi, sorry for the delay. I've just send the pull request to Jens. I tested the patch and stressed the driver with syzkaller. Everything look good with the patch to me. Thanks!
Denis
Denis Efremov
maintainerRe: [PATCH RESEND] floppy: fix open(O_ACCMODE) for ioctl-only open
In reply to Jiri KosinaQuoted 55 lines · Jiri Kosina
Quoted 1 line · Denis Efremov
I think it's hard to recall the exact reasons after so many years.
Yeah, I guess so :)
Quoted 1 line
I'll send a patch today based on this one.
I am currently waiting for confirmation by the original reporter that the patch below fixes the issue.
From: Jiri Kosina <[email protected]>
Subject: [PATCH] floppy: reintroduceO_NDELAYfixOriginally fixed in
09954bad4("floppy: refactoropen()flags handling") then reverted for unknown reason inf2791e7eadf437instead of taking theopen(O_ACCMODE)for ioctl-only open fix, which had the changelog below====
Commit09954bad4("floppy: refactoropen()flags handling"), as a side-effect, causesopen(/dev/fdX, O_ACCMODE)to fail. It turns out that this is being used setfdprm userspace for ioctl-onlyopen().Reintroduce back the original behavior wrt !(FMODE_READ|FMODE_WRITE) modes, while still keeping the original
O_NDELAYbug fixed.Cc [email protected] # v4.5+Reported-by Wim Osterholt <[email protected]>Tested-by Wim Osterholt <[email protected]>Signed-off-by Jiri Kosina <[email protected]>=====
Fixes 09954bad4 ("floppy: refactor open() flags handling")Fixes f2791e7ead ("Revert "floppy: refactor open() flags handling"")Signed-off-by Jiri Kosina <[email protected]>--- drivers/block/floppy.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)drivers/block/floppy.c +0
-6@@ -4121,23 +4121,22 @@ static int floppy_open(struct block_device *bdev, fmode_t mode)if (fdc_state[FDC (drive)].rawcmd ==1 ) fdc_state[FDC (drive)].rawcmd =2 ; -if (!(mode &FMODE_NDELAY )) { -if (mode & (FMODE_READ |FMODE_WRITE )) { - drive_state[drive].last_checked =0 ; -clear_bit (FD_OPEN_SHOULD_FAIL_BIT , - &drive_state[drive].flags); -if (bdev_check_media_change (bdev))
drivers/block/floppy.c static int floppy_open(struct block_device *bdev, fmode_t mode) · line 4124
Quoted 10 lines
drivers/block/floppy.c +1
-9-floppy_revalidate (bdev->bd_disk); -if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) - goto out; -if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) - goto out; - } - res = -EROFS ; -if ((mode &FMODE_WRITE ) && - !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) +if (mode & (FMODE_READ |FMODE_WRITE )) {
As the bot points out this was refactored a bit in:8d9d34e25a37 ("floppy: cleanup: expand macro UDRS") 4a6f3d480edc ("floppy: use bdev_check_media_change")
Should be something like:
+ drive_state[drive].last_checked = 0;
+ clear_bit(FD_OPEN_SHOULD_FAIL_BIT,
+ &drive_state[drive].flags);
+ if (bdev_check_media_change(bdev))
+ floppy_revalidate(bdev->bd_disk);
Quoted 16 lines
drivers/block/floppy.c +10
-0+if (test_bit (FD_DISK_CHANGED_BIT , &drive_state[drive].flags)) + goto out; +if (test_bit (FD_OPEN_SHOULD_FAIL_BIT , &drive_state[drive].flags)) goto out; } + + res = -EROFS ; + +if ((mode &FMODE_WRITE ) && + !test_bit (FD_DISK_WRITABLE_BIT , &drive_state[drive].flags)) + goto out; +mutex_unlock (&open_lock);mutex_unlock (&floppy_mutex);return 0 ;
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.
----- [email protected] -----
Quoted 2 lines · Wim Osterholt
That was because the changelog at the beginning of
floppy.cended 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.
Signature · 2 lines
Jiri Kosina SUSE Labs
Quoted 7 lines · Wim Osterholt
On my first message I stated:
It looks to me that the code in
floppy.cis quite old; no changes here. So the bug is elsewhere in the kernel.
That was because the changelog at the beginning offloppy.cended 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.
Quoted 3 lines · Jiri Kosina
Hmm, could you please test with
09954bad448reverted? (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.