On Thu, 21 Jan 2021, Denis Efremov wrote:
> > 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.
> >
> > 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.
You are right; I'll drop the '4.5+' indicator and will backport it once/if
it hits Linus' tree.
> > + 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,
--
Jiri Kosina
SUSE Labs
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.
Cc: [email protected]
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]>
---
drivers/block/floppy.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index dfe1dfc901cc..0b71292d9d5a 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -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;
--
Jiri Kosina
SUSE Labs
On 1/26/21 12:31 PM, Kurt Garloff wrote:
> Hi Denis, Jiri, Jens,
>
> Am 26.01.21 um 09:21 schrieb Denis Efremov:
>> On 1/22/21 2:13 PM, Jiri Kosina wrote:
>>> 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.
>>>
>>> Cc: [email protected]
>>> 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
>
> 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.
Yes, it will be backported to all stable trees including v4.4
Thanks,
Denis
Hi Denis, Jiri, Jens,
Am 26.01.21 um 09:21 schrieb Denis Efremov:
> On 1/22/21 2:13 PM, Jiri Kosina wrote:
>> 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.
>>
>> Cc: [email protected]
>> 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
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,
--
Kurt Garloff <[email protected]>, Cologne, Germany
On 1/22/21 2:13 PM, Jiri Kosina wrote:
> 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.
>
> Cc: [email protected]
> 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
> ---
> drivers/block/floppy.c | 30 +++++++++++++++---------------
> 1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index dfe1dfc901cc..0b71292d9d5a 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -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;
>
On 2/4/21 12:24 PM, Jiri Kosina wrote:
> On Tue, 26 Jan 2021, Denis Efremov wrote:
>
>> 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
On Tue, 26 Jan 2021, Denis Efremov wrote:
> 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,
--
Jiri Kosina
SUSE Labs