[BUG] FLOPPY DRIVER since 5.10.20

[BUG] FLOPPY DRIVER since 5.10.20

[Mark Hounschell]

These 2 incremental patches, patch-5.10.19-20 and patch-5.11.2-3 have 
broken the user land fd = open("/dev/fd0", (O_RDWR | O_NDELAY)); 
functionality.

Since FOREVER before the patch, when using O_NDELAY, one could open the 
floppy device with no media inserted or even with write protected media 
without error. "Read-only file system" status is returned only when we 
actually tried to write to it. We have software still in use today that 
relies on this functionality.

After the patch, if no media is in the drive the open fails with "no 
such device or address". If the floppy media is write protected the open 
fails with "Read-only file system".

This is certainly drastically changing user land usage incorrectly IMHO.

I think the commit is commit 8a0c014cd20516ade9654fc13b51345ec58e7be8
It is the only reference to the floppy driver in ChangeLog-5.10.20. 
However this change log entry actually sounds like it fixes the very 
issue I am reporting.

Here is the patch from the 5.10.19-20 change.

diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 7df79ae6b0a1e..295da442329f3 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4120,23 +4120,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;

Regards
Mark

Re: [BUG] FLOPPY DRIVER since 5.10.20

[Denis Efremov]

Hi,

On 7/23/21 9:47 PM, Mark Hounschell wrote:
> 
> These 2 incremental patches, patch-5.10.19-20 and patch-5.11.2-3 have broken the user land fd = open("/dev/fd0", (O_RDWR | O_NDELAY)); functionality.

Thank you for the report, I'm looking into this.

> Since FOREVER before the patch, when using O_NDELAY, one could open the floppy device with no media inserted or even with write protected media without error. "Read-only file system" status is returned only when we actually tried to write to it. We have software still in use today that relies on this functionality.

If it's a project with open sources could you please give a link?

Regards,
Denis

Re: [BUG] FLOPPY DRIVER since 5.10.20

[Mark Hounschell]

On 7/26/21 3:57 AM, Denis Efremov wrote:
> Hi,
> 
> On 7/23/21 9:47 PM, Mark Hounschell wrote:
>>
>> These 2 incremental patches, patch-5.10.19-20 and patch-5.11.2-3 have broken the user land fd = open("/dev/fd0", (O_RDWR | O_NDELAY)); functionality.
> 
> Thank you for the report, I'm looking into this.
> 
>> Since FOREVER before the patch, when using O_NDELAY, one could open the floppy device with no media inserted or even with write protected media without error. "Read-only file system" status is returned only when we actually tried to write to it. We have software still in use today that relies on this functionality.
> 
> If it's a project with open sources could you please give a link?
> 

This is imatterial

Re: [BUG] FLOPPY DRIVER since 5.10.20

[Mark Hounschell]

On 7/26/21 3:57 AM, Denis Efremov wrote:
> Hi,
> 
> On 7/23/21 9:47 PM, Mark Hounschell wrote:
>>
>> These 2 incremental patches, patch-5.10.19-20 and patch-5.11.2-3 have broken the user land fd = open("/dev/fd0", (O_RDWR | O_NDELAY)); functionality.
> 
> Thank you for the report, I'm looking into this.
> 
>> Since FOREVER before the patch, when using O_NDELAY, one could open the floppy device with no media inserted or even with write protected media without error. "Read-only file system" status is returned only when we actually tried to write to it. We have software still in use today that relies on this functionality.
> 
> If it's a project with open sources could you please give a link?
> 
> Regards,
> Denis
> 
This is immaterial but fdutils and libdsk both use rely on this flag. Who 
can know who else does. The point is it should NOT have been changed.

Mark

Re: [BUG] FLOPPY DRIVER since 5.10.20

[Denis Efremov]

On 7/26/21 2:17 PM, Mark Hounschell wrote:
> On 7/26/21 3:57 AM, Denis Efremov wrote:
>> Hi,
>>
>> On 7/23/21 9:47 PM, Mark Hounschell wrote:
>>>
>>> These 2 incremental patches, patch-5.10.19-20 and patch-5.11.2-3 have broken the user land fd = open("/dev/fd0", (O_RDWR | O_NDELAY)); functionality.
>>
>> Thank you for the report, I'm looking into this.
>>
>>> Since FOREVER before the patch, when using O_NDELAY, one could open the floppy device with no media inserted or even with write protected media without error. "Read-only file system" status is returned only when we actually tried to write to it. We have software still in use today that relies on this functionality.
>>
>> If it's a project with open sources could you please give a link?
>>
>> Regards,
>> Denis
>>
> This is immaterial but fdutils and libdsk both use rely on this flag. Who can know who else does. The point is it should NOT have been changed.

Yes, I asked this only to add utils and this behavior to the tests.
And be more specific about why we should preserve this behavior in
next commit messages.

Denis

Re: [BUG] FLOPPY DRIVER since 5.10.20

[Mark Hounschell]

On 7/26/21 7:37 AM, Denis Efremov wrote:
> 
> 
> On 7/26/21 2:17 PM, Mark Hounschell wrote:
>> On 7/26/21 3:57 AM, Denis Efremov wrote:
>>> Hi,
>>>
>>> On 7/23/21 9:47 PM, Mark Hounschell wrote:
>>>>
>>>> These 2 incremental patches, patch-5.10.19-20 and patch-5.11.2-3 have broken the user land fd = open("/dev/fd0", (O_RDWR | O_NDELAY)); functionality.
>>>
>>> Thank you for the report, I'm looking into this.
>>>
>>>> Since FOREVER before the patch, when using O_NDELAY, one could open the floppy device with no media inserted or even with write protected media without error. "Read-only file system" status is returned only when we actually tried to write to it. We have software still in use today that relies on this functionality.
>>>
>>> If it's a project with open sources could you please give a link?
>>>
>>> Regards,
>>> Denis
>>>
>> This is immaterial but fdutils and libdsk both use rely on this flag. Who can know who else does. The point is it should NOT have been changed.
> 
> Yes, I asked this only to add utils and this behavior to the tests.
> And be more specific about why we should preserve this behavior in
> next commit messages.
> 

Well, first thing is now you can't open a floppy with a write protected 
floppy installed. I don't think that was intended but that is now how it is.

Next there are commands that can be sent to the floppy via "ioctl(fd, 
FDRAWCMD,  &raw_cmd);" that do NOT require a floppy diskette to be 
installed.

All commands issued to the device that require a floppy diskette without 
a diskette installed fail with the proper status letting you know the 
device is not ready / no diskette installed. That goes for write 
protected floppies too.

There is no reason to force a user to only be able to operate on Linux 
fdformat formatted floppies.

Regards
Mark

Re: [BUG] FLOPPY DRIVER since 5.10.20

[Denis Efremov]

On 7/26/21 3:23 PM, Mark Hounschell wrote:
> On 7/26/21 7:37 AM, Denis Efremov wrote:
>>
>>
>> On 7/26/21 2:17 PM, Mark Hounschell wrote:
>>> On 7/26/21 3:57 AM, Denis Efremov wrote:
>>>> Hi,
>>>>
>>>> On 7/23/21 9:47 PM, Mark Hounschell wrote:
>>>>>
>>>>> These 2 incremental patches, patch-5.10.19-20 and patch-5.11.2-3 have broken the user land fd = open("/dev/fd0", (O_RDWR | O_NDELAY)); functionality.
>>>>
>>>> Thank you for the report, I'm looking into this.
>>>>
>>>>> Since FOREVER before the patch, when using O_NDELAY, one could open the floppy device with no media inserted or even with write protected media without error. "Read-only file system" status is returned only when we actually tried to write to it. We have software still in use today that relies on this functionality.
>>>>
>>>> If it's a project with open sources could you please give a link?
>>>>
>>>> Regards,
>>>> Denis
>>>>
>>> This is immaterial but fdutils and libdsk both use rely on this flag. Who can know who else does. The point is it should NOT have been changed.
>>
>> Yes, I asked this only to add utils and this behavior to the tests.
>> And be more specific about why we should preserve this behavior in
>> next commit messages.
>>
> 
> Well, first thing is now you can't open a floppy with a write protected floppy installed. I don't think that was intended but that is now how it is.
> 
> Next there are commands that can be sent to the floppy via "ioctl(fd, FDRAWCMD,  &raw_cmd);" that do NOT require a floppy diskette to be installed.
> 
> All commands issued to the device that require a floppy diskette without a diskette installed fail with the proper status letting you know the device is not ready / no diskette installed. That goes for write protected floppies too.
> 
> There is no reason to force a user to only be able to operate on Linux fdformat formatted floppies.
> 

It appears that the story behind the issue is long enough.
I'll try to sum up the things:
[1] 09954bad4487 floppy: refactor open() flags handling
[2] ff06db1efb2a floppy: fix open(O_ACCMODE) for ioctl-only open
[3] 468c298ad3ed Revert "floppy: fix open(O_ACCMODE) for ioctl-only open"
[4] f2791e7eadf4 Revert "floppy: refactor open() flags handling"
[5] 8a0c014cd205 floppy: reintroduce O_NDELAY fix

In [1] we tried to fix O_NDELAY behavior because it's hard to define
proper non-blocking behavior for floppies. We also added
"!(mode & (FMODE_READ|FMODE_WRITE))" sanity check for open in that patch.
Motivation for the changes was that it's easy to livelock the system with
floppy's O_NDELAY and syzkaller spotted it. Just for the record, /dev/fd0
is only accessible by the root user in recent distros. 

Patch [1] broke ioctl-only opens in fdutils because:
$ grep -nre open ./setfdprm.c 
60:     if ((fd = open(argv[0],3)) < 0) { /* 3 == no access at all */
Patch [2] reverted "!(mode & (FMODE_READ|FMODE_WRITE))" to fix ioctls.
I guess [2] was not enough and Jens completely reverted [1] with [3] [4].

The last [5] patch restores the open function to the [2] state (it's possible
to use ioctl with open O_ACCMODE). [5] was added because libblkid use O_NONBLOCK
for probing devices, and floppy driver prints many I/O errors to the kernel log.
There are also problems with mounts after. I'm afraid simple revert for [5] is
not enough, otherwise we will face libblkid issues once again.I'll try to test the things and find a more elegant solution.

Denis

Re: [BUG] FLOPPY DRIVER since 5.10.20

[Denis Efremov]

Hi,

On 7/26/21 7:34 PM, Denis Efremov wrote:
> 
> 
> On 7/26/21 3:23 PM, Mark Hounschell wrote:
>> On 7/26/21 7:37 AM, Denis Efremov wrote:
>>>
>>>
>>> On 7/26/21 2:17 PM, Mark Hounschell wrote:
>>>> On 7/26/21 3:57 AM, Denis Efremov wrote:
>>>>> Hi,
>>>>>
>>>>> On 7/23/21 9:47 PM, Mark Hounschell wrote:
>>>>>>
>>>>>> These 2 incremental patches, patch-5.10.19-20 and patch-5.11.2-3 have broken the user land fd = open("/dev/fd0", (O_RDWR | O_NDELAY)); functionality.
>>>>>
>>>>> Thank you for the report, I'm looking into this.
>>>>>
>>>>>> Since FOREVER before the patch, when using O_NDELAY, one could open the floppy device with no media inserted or even with write protected media without error. "Read-only file system" status is returned only when we actually tried to write to it. We have software still in use today that relies on this functionality.
>>>>>
>>>>> If it's a project with open sources could you please give a link?
>>>>>
>>>>> Regards,
>>>>> Denis
>>>>>
>>>> This is immaterial but fdutils and libdsk both use rely on this flag. Who can know who else does. The point is it should NOT have been changed.
>>>
>>> Yes, I asked this only to add utils and this behavior to the tests.
>>> And be more specific about why we should preserve this behavior in
>>> next commit messages.
>>>
>>
>> Well, first thing is now you can't open a floppy with a write protected floppy installed. I don't think that was intended but that is now how it is.
>>
>> Next there are commands that can be sent to the floppy via "ioctl(fd, FDRAWCMD,  &raw_cmd);" that do NOT require a floppy diskette to be installed.
>>
>> All commands issued to the device that require a floppy diskette without a diskette installed fail with the proper status letting you know the device is not ready / no diskette installed. That goes for write protected floppies too.
>>
>> There is no reason to force a user to only be able to operate on Linux fdformat formatted floppies.
>>
> 
> It appears that the story behind the issue is long enough.
> I'll try to sum up the things:
> [1] 09954bad4487 floppy: refactor open() flags handling
> [2] ff06db1efb2a floppy: fix open(O_ACCMODE) for ioctl-only open
> [3] 468c298ad3ed Revert "floppy: fix open(O_ACCMODE) for ioctl-only open"
> [4] f2791e7eadf4 Revert "floppy: refactor open() flags handling"
> [5] 8a0c014cd205 floppy: reintroduce O_NDELAY fix
> 
> In [1] we tried to fix O_NDELAY behavior because it's hard to define
> proper non-blocking behavior for floppies. We also added
> "!(mode & (FMODE_READ|FMODE_WRITE))" sanity check for open in that patch.
> Motivation for the changes was that it's easy to livelock the system with
> floppy's O_NDELAY and syzkaller spotted it. Just for the record, /dev/fd0
> is only accessible by the root user in recent distros. 
> 
> Patch [1] broke ioctl-only opens in fdutils because:
> $ grep -nre open ./setfdprm.c 
> 60:     if ((fd = open(argv[0],3)) < 0) { /* 3 == no access at all */
> Patch [2] reverted "!(mode & (FMODE_READ|FMODE_WRITE))" to fix ioctls.
> I guess [2] was not enough and Jens completely reverted [1] with [3] [4].
> 
> The last [5] patch restores the open function to the [2] state (it's possible
> to use ioctl with open O_ACCMODE). [5] was added because libblkid use O_NONBLOCK
> for probing devices, and floppy driver prints many I/O errors to the kernel log.
> There are also problems with mounts after. I'm afraid simple revert for [5] is
> not enough, otherwise we will face libblkid issues once again.I'll try to test the things and find a more elegant solution.
> 

I performed some tests and here is a small example that can be reproduced
even with qemu.
With O_NDELAY fix:
$ fdlist # no floppy inserted
fdlist (): drive fd0 does not exist

Without O_NDELAY fix:
$ fdlist # no floppy inserted
NAME   TYPE  STATUS
 fd0  2880K  not mounted

That's because of O_RDONLY|O_NDELAY open in probe_drive:
https://sources.debian.org/src/fdutils/5.6-2/src/fdmount.c/#L390

I guess that's why the original patch was reverted
f2791e7eadf4 Revert "floppy: refactor open() flags handling"
We still have software that depends on O_NDELAY in floppies
and this patch will be reverted again.

Meanwhile I can't fully reproduce the issues with libblkid.
I know that systemd-udevd tries to open /dev/fd0 during boot
with O_RDONLY|O_NDELAY. With O_NDELAY implemented we don't call
floppy_revalidate() which result in an attempt to read block 0
https://elixir.bootlin.com/linux/v5.10.20/source/drivers/block/floppy.c#L4127
However, *with* O_NDELAY fix we try to read block 0 and get kernel
log errors if no floppy inserted:
[    1.732360] blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
[    1.732822] floppy: error 10 while reading block 0
If floppy inserted this results in a boot delay on a
real system.

Jiri, Kurt, can you give more details about test conditions
for O_NDELAY problem or maybe even provide some examples?

Maybe it will cheaper to implement a special probing for
floppies in new software than drop O_NDELAY for all already
written software. Of course, if there is no cheap and obvious
in-kernel fix.

Denis

Re: [BUG] FLOPPY DRIVER since 5.10.20

[Kurt Garloff]

Hi Denis,

Here's what I did to uncover and reproduce the bug:
* Building a VM image with openSUSE-15.2
* VM image includes cloud-init which reads from a config-drive (with fallback to network) to customize the generic image on (first) boot
* Started the image with an attached floppy disk in KVM (qemu-5.x) that contained valid cloud-config (cidata) information
(-drive file=/tmp/ci-disk.17138.3Jf2/seed-17138.img,format=raw,if=floppy,id=cidata)

This worked fine until openSUSE updated libblkid to include a backport that opens cdroms and floppies with O_NONBLOCK (to avoid spurious CD tray closes on CDRoms).

History is at
https://bugzilla.suse.com/show_bug.cgi?id=1181018 <https://bugzilla.suse.com/show_bug.cgi?id=1181018>

My understanding is that
* qemu reports media changed once on the attached floppy (it's a removable device after all) -- I have no idea whether or not that behavior from qemu is reasonable or not.
* old libblkid used to probe it without O_NONBLOCK, finding out that there is a medium inserted and clearing the media change flag
* a subsequent mount attempt (by cloud-init) would succeed

With new libblkid, using O_NONBLOCK, the media change was not cleared, and the mount would not succeed, DESPITE a valid floppy being attached before booting.

With the kernel update, the access would work again, despite blkkid using O_NONBLOCK.

Jiri should be able to understand this in more detail than I am -- I am no expert in handling of removable block devices ...

Let me know if you need a VM image to reproduce this issue -- I can find it in my archives and push it to some place for downloading.

-- 
Kurt Garloff <kurt@garloff.de>
Cologne, Germany

On 30/07/2021 07:15, Denis Efremov wrote:
> Hi,
>
> On 7/26/21 7:34 PM, Denis Efremov wrote:
>>
>> On 7/26/21 3:23 PM, Mark Hounschell wrote:
>>> On 7/26/21 7:37 AM, Denis Efremov wrote:
>>>>
>>>> On 7/26/21 2:17 PM, Mark Hounschell wrote:
>>>>> On 7/26/21 3:57 AM, Denis Efremov wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 7/23/21 9:47 PM, Mark Hounschell wrote:
>>>>>>> These 2 incremental patches, patch-5.10.19-20 and patch-5.11.2-3 have broken the user land fd = open("/dev/fd0", (O_RDWR | O_NDELAY)); functionality.
>>>>>> Thank you for the report, I'm looking into this.
>>>>>>
>>>>>>> Since FOREVER before the patch, when using O_NDELAY, one could open the floppy device with no media inserted or even with write protected media without error. "Read-only file system" status is returned only when we actually tried to write to it. We have software still in use today that relies on this functionality.
>>>>>> If it's a project with open sources could you please give a link?
>>>>>>
>>>>>> Regards,
>>>>>> Denis
>>>>>>
>>>>> This is immaterial but fdutils and libdsk both use rely on this flag. Who can know who else does. The point is it should NOT have been changed.
>>>> Yes, I asked this only to add utils and this behavior to the tests.
>>>> And be more specific about why we should preserve this behavior in
>>>> next commit messages.
>>>>
>>> Well, first thing is now you can't open a floppy with a write protected floppy installed. I don't think that was intended but that is now how it is.
>>>
>>> Next there are commands that can be sent to the floppy via "ioctl(fd, FDRAWCMD,  &raw_cmd);" that do NOT require a floppy diskette to be installed.
>>>
>>> All commands issued to the device that require a floppy diskette without a diskette installed fail with the proper status letting you know the device is not ready / no diskette installed. That goes for write protected floppies too.
>>>
>>> There is no reason to force a user to only be able to operate on Linux fdformat formatted floppies.
>>>
>> It appears that the story behind the issue is long enough.
>> I'll try to sum up the things:
>> [1] 09954bad4487 floppy: refactor open() flags handling
>> [2] ff06db1efb2a floppy: fix open(O_ACCMODE) for ioctl-only open
>> [3] 468c298ad3ed Revert "floppy: fix open(O_ACCMODE) for ioctl-only open"
>> [4] f2791e7eadf4 Revert "floppy: refactor open() flags handling"
>> [5] 8a0c014cd205 floppy: reintroduce O_NDELAY fix
>>
>> In [1] we tried to fix O_NDELAY behavior because it's hard to define
>> proper non-blocking behavior for floppies. We also added
>> "!(mode & (FMODE_READ|FMODE_WRITE))" sanity check for open in that patch.
>> Motivation for the changes was that it's easy to livelock the system with
>> floppy's O_NDELAY and syzkaller spotted it. Just for the record, /dev/fd0
>> is only accessible by the root user in recent distros. 
>>
>> Patch [1] broke ioctl-only opens in fdutils because:
>> $ grep -nre open ./setfdprm.c 
>> 60:     if ((fd = open(argv[0],3)) < 0) { /* 3 == no access at all */
>> Patch [2] reverted "!(mode & (FMODE_READ|FMODE_WRITE))" to fix ioctls.
>> I guess [2] was not enough and Jens completely reverted [1] with [3] [4].
>>
>> The last [5] patch restores the open function to the [2] state (it's possible
>> to use ioctl with open O_ACCMODE). [5] was added because libblkid use O_NONBLOCK
>> for probing devices, and floppy driver prints many I/O errors to the kernel log.
>> There are also problems with mounts after. I'm afraid simple revert for [5] is
>> not enough, otherwise we will face libblkid issues once again.I'll try to test the things and find a more elegant solution.
>>
> I performed some tests and here is a small example that can be reproduced
> even with qemu.
> With O_NDELAY fix:
> $ fdlist # no floppy inserted
> fdlist (): drive fd0 does not exist
>
> Without O_NDELAY fix:
> $ fdlist # no floppy inserted
> NAME   TYPE  STATUS
>  fd0  2880K  not mounted
>
> That's because of O_RDONLY|O_NDELAY open in probe_drive:
> https://sources.debian.org/src/fdutils/5.6-2/src/fdmount.c/#L390
>
> I guess that's why the original patch was reverted
> f2791e7eadf4 Revert "floppy: refactor open() flags handling"
> We still have software that depends on O_NDELAY in floppies
> and this patch will be reverted again.
>
> Meanwhile I can't fully reproduce the issues with libblkid.
> I know that systemd-udevd tries to open /dev/fd0 during boot
> with O_RDONLY|O_NDELAY. With O_NDELAY implemented we don't call
> floppy_revalidate() which result in an attempt to read block 0
> https://elixir.bootlin.com/linux/v5.10.20/source/drivers/block/floppy.c#L4127
> However, *with* O_NDELAY fix we try to read block 0 and get kernel
> log errors if no floppy inserted:
> [    1.732360] blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
> [    1.732822] floppy: error 10 while reading block 0
> If floppy inserted this results in a boot delay on a
> real system.
>
> Jiri, Kurt, can you give more details about test conditions
> for O_NDELAY problem or maybe even provide some examples?
>
> Maybe it will cheaper to implement a special probing for
> floppies in new software than drop O_NDELAY for all already
> written software. Of course, if there is no cheap and obvious
> in-kernel fix.
>
> Denis
>

[PATCH] Revert "floppy: reintroduce O_NDELAY fix"

[Denis Efremov]

The patch breaks userspace implementations (e.g. fdutils) and introduces
regressions in behaviour. Previously, it was possible to O_NDELAY open a
floppy device with no media inserted or with write protected media without
an error. Some userspace tools use this particular behavior for probing.

It's not the first time when we revert this patch. Previous revert is in
commit f2791e7eadf4 (Revert "floppy: refactor open() flags handling").

This reverts commit 8a0c014cd20516ade9654fc13b51345ec58e7be8.

Link: https://lore.kernel.org/linux-block/de10cb47-34d1-5a88-7751-225ca380f735@compro.net/
Reported-by: Mark Hounschell <markh@compro.net>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Wim Osterholt <wim@djo.tudelft.nl>
Cc: Kurt Garloff <kurt@garloff.de>
Cc: <stable@vger.kernel.org>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
 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 87460e0e5c72..fef79ea52e3e 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -4029,23 +4029,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_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))
+	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))
 			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;
-- 
2.31.1

Re: [PATCH] Revert "floppy: reintroduce O_NDELAY fix"

[Jiri Kosina]

On Sun, 8 Aug 2021, Denis Efremov wrote:

> The patch breaks userspace implementations (e.g. fdutils) and introduces
> regressions in behaviour. Previously, it was possible to O_NDELAY open a
> floppy device with no media inserted or with write protected media without
> an error. Some userspace tools use this particular behavior for probing.
> 
> It's not the first time when we revert this patch. Previous revert is in
> commit f2791e7eadf4 (Revert "floppy: refactor open() flags handling").
> 
> This reverts commit 8a0c014cd20516ade9654fc13b51345ec58e7be8.

By reverting it you bring back the bugs that were fixed by it -- e.g. the 
possibility to livelock mmap() on the returned fd to keep waiting on the 
page unlock bit forever, or the functionality bug reported at [1], and 
likely others.

[1] https://bugzilla.suse.com/show_bug.cgi?id=1181018

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] Revert "floppy: reintroduce O_NDELAY fix"

[Jiri Kosina]

On Sun, 8 Aug 2021, Denis Efremov wrote:

> The patch breaks userspace implementations (e.g. fdutils) and introduces
> regressions in behaviour. Previously, it was possible to O_NDELAY open a
> floppy device with no media inserted or with write protected media without
> an error. Some userspace tools use this particular behavior for probing.
> 
> It's not the first time when we revert this patch. Previous revert is in
> commit f2791e7eadf4 (Revert "floppy: refactor open() flags handling").
> 
> This reverts commit 8a0c014cd20516ade9654fc13b51345ec58e7be8.

By reverting it you bring back the bugs that were fixed by it -- e.g. the 
possibility to livelock mmap() on the returned fd to keep waiting on the 
page unlock bit forever, or the functionality bug reported at [1], and 
likely others.

[1] https://bugzilla.suse.com/show_bug.cgi?id=1181018

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] Revert "floppy: reintroduce O_NDELAY fix"

[Denis Efremov]

Hi,

On 8/16/21 10:17 AM, Jiri Kosina wrote:
> On Sun, 8 Aug 2021, Denis Efremov wrote:
> 
>> The patch breaks userspace implementations (e.g. fdutils) and introduces
>> regressions in behaviour. Previously, it was possible to O_NDELAY open a
>> floppy device with no media inserted or with write protected media without
>> an error. Some userspace tools use this particular behavior for probing.
>>
>> It's not the first time when we revert this patch. Previous revert is in
>> commit f2791e7eadf4 (Revert "floppy: refactor open() flags handling").
>>
>> This reverts commit 8a0c014cd20516ade9654fc13b51345ec58e7be8.
> 
> By reverting it you bring back the bugs that were fixed by it

I agree with you, that O_NDELAY is broken for floppies (and always been).
However, just by removing O_NDELAY we break many existing tools that use
it for probing and ioctl-only opens. With the patch tools fail to open the
device without a diskette and try to read a diskette if there is one (this is
not as fast on a real hardware as in QEMU).
I think that there should be a better fix that doesn't break existing tools.
It appears that people still use software that depends on O_NDELAY in floppies.
Same patch was already reverted in 2016 (presumably) by the same reason.

>  -- e.g. the 
> possibility to livelock mmap() on the returned fd to keep waiting on the 
> page unlock bit forever

As far as I understand this is a problem only for syzkaller.
And this is not a security issue nowadays since most distributions
(I don't know exceptions) require at least "disk" group to access floppies.
Do you know a link for the syzkaller reproducer?

> or the functionality bug reported at [1], and 
> likely others.
> 
> [1] https://bugzilla.suse.com/show_bug.cgi?id=1181018

The patch starts to return -ENXIO for O_NDELAY|O_RDONLY opens and devices
without a diskette. I don't think this is an expected behavior during 
libblkid probing.

Probably there is a better fix for [1], maybe even an additional workaround
for floppies in libblkid. They already have workarounds for cdroms
https://github.com/karelzak/util-linux/commit/dc30fd4383e57a0440cdb0e16ba5c4336a43b290

I started to add simple tests https://lkml.org/lkml/2021/8/18/845
However, I failed to reproduce mount bug [1], probably
because I don't know how to configure cloudinit properly. I tried to reproduce
a mount fail bug with open("/dev/fd0", O_NDELAY|O_RDONLY) and mount("/dev/fd0", ...)
but it works. Looks like there should be something else in between...

Regards,
Denis

Re: [PATCH] Revert "floppy: reintroduce O_NDELAY fix"

[Thorsten Leemhuis]

On 08.08.21 09:42, Denis Efremov wrote:
> The patch breaks userspace implementations (e.g. fdutils) and introduces
> regressions in behaviour. Previously, it was possible to O_NDELAY open a
> floppy device with no media inserted or with write protected media without
> an error. Some userspace tools use this particular behavior for probing.
> 
> It's not the first time when we revert this patch. Previous revert is in
> commit f2791e7eadf4 (Revert "floppy: refactor open() flags handling").
> 
> This reverts commit 8a0c014cd20516ade9654fc13b51345ec58e7be8.
> 
> Link: https://lore.kernel.org/linux-block/de10cb47-34d1-5a88-7751-225ca380f735@compro.net/

FYI, I'm just starting to act as regression tracker again and will add
this to the list of tracked regressions.

Feel free to ignore the rest of this message, it's intended for the
regression tracking bot I'm writing. This "regzbot" in fact is running
now and this is the first regression that gets added to the database
(I'm sure despite a lot of testing something will go wrong, but don't
worry about it, I'll deal with it on my side). See also:
https://linux-regtracking.leemhuis.info/post/inital-regzbot-running/
https://linux-regtracking.leemhuis.info/post/regzbot-approach/

#regzbot ^introduced 8a0c014cd20516ade9654fc13b51345ec58e7be8
#regzbot monitor
https://lore.kernel.org/lkml/20210818154646.925351-1-efremov@linux.com/
#regzbot monitor
https://lore.kernel.org/lkml/388418f4-2b9a-6fed-836c-a004369dc7c0@linux.com/

Ciao, Thorsten

Re: [PATCH] Revert "floppy: reintroduce O_NDELAY fix"

[Thorsten Leemhuis]

On 30.08.21 11:20, Thorsten Leemhuis wrote:
> On 08.08.21 09:42, Denis Efremov wrote:
>> The patch breaks userspace implementations (e.g. fdutils) and introduces
>> regressions in behaviour. Previously, it was possible to O_NDELAY open a
>> floppy device with no media inserted or with write protected media without
>> an error. Some userspace tools use this particular behavior for probing.
>>
>> It's not the first time when we revert this patch. Previous revert is in
>> commit f2791e7eadf4 (Revert "floppy: refactor open() flags handling").
>>
>> This reverts commit 8a0c014cd20516ade9654fc13b51345ec58e7be8.
>>
>> Link: https://lore.kernel.org/linux-block/de10cb47-34d1-5a88-7751-225ca380f735@compro.net/
> 
> FYI, I'm just starting to act as regression tracker again and will add
> this to the list of tracked regressions.
> 
> Feel free to ignore the rest of this message, it's intended for the
> regression tracking bot I'm writing. This "regzbot" in fact is running
> now and this is the first regression that gets added to the database
> (I'm sure despite a lot of testing something will go wrong, but don't
> worry about it, I'll deal with it on my side). See also:
> https://linux-regtracking.leemhuis.info/post/inital-regzbot-running/
> https://linux-regtracking.leemhuis.info/post/regzbot-approach/
> 
> #regzbot ^introduced 8a0c014cd20516ade9654fc13b51345ec58e7be8
> #regzbot monitor
> https://lore.kernel.org/lkml/20210818154646.925351-1-efremov@linux.com/
> #regzbot monitor
> https://lore.kernel.org/lkml/388418f4-2b9a-6fed-836c-a004369dc7c0@linux.com/

TWIMC: the above made me find and fix a few bugs and other problems in
regzbot. But regzbot didn't automatically close the record when the
revert was merged, which was my fault, as above commands didn't point to
the initial report (the one referenced in the revert). So I'm adding it
now with below line (with only regressions list in To: to avoid spamming
people) to make regzbot handle such situations retroactively:

#regzbot monitor
https://lore.kernel.org/linux-block/de10cb47-34d1-5a88-7751-225ca380f735@compro.net/