* [BUG] FLOPPY DRIVER since 5.10.20 @ 2021-07-23 18:47 Mark Hounschell 2021-07-26 7:57 ` Denis Efremov 2021-08-08 7:42 ` [PATCH] Revert "floppy: reintroduce O_NDELAY fix" Denis Efremov 0 siblings, 2 replies; 13+ messages in thread From: Mark Hounschell @ 2021-07-23 18:47 UTC (permalink / raw) To: linux-block, Linux-kernel Cc: Denis Efremov, Jiri Kosina, Mark Hounschell, Greg Kroah-Hartman 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [BUG] FLOPPY DRIVER since 5.10.20 2021-07-23 18:47 [BUG] FLOPPY DRIVER since 5.10.20 Mark Hounschell @ 2021-07-26 7:57 ` Denis Efremov 2021-07-26 11:15 ` Mark Hounschell 2021-07-26 11:17 ` Mark Hounschell 2021-08-08 7:42 ` [PATCH] Revert "floppy: reintroduce O_NDELAY fix" Denis Efremov 1 sibling, 2 replies; 13+ messages in thread From: Denis Efremov @ 2021-07-26 7:57 UTC (permalink / raw) To: markh, linux-block, Linux-kernel Cc: Jiri Kosina, Mark Hounschell, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] FLOPPY DRIVER since 5.10.20 2021-07-26 7:57 ` Denis Efremov @ 2021-07-26 11:15 ` Mark Hounschell 2021-07-26 11:17 ` Mark Hounschell 1 sibling, 0 replies; 13+ messages in thread From: Mark Hounschell @ 2021-07-26 11:15 UTC (permalink / raw) To: Denis Efremov, markh, linux-block, Linux-kernel Cc: Jiri Kosina, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] FLOPPY DRIVER since 5.10.20 2021-07-26 7:57 ` Denis Efremov 2021-07-26 11:15 ` Mark Hounschell @ 2021-07-26 11:17 ` Mark Hounschell 2021-07-26 11:37 ` Denis Efremov 1 sibling, 1 reply; 13+ messages in thread From: Mark Hounschell @ 2021-07-26 11:17 UTC (permalink / raw) To: Denis Efremov, markh, linux-block, Linux-kernel Cc: Jiri Kosina, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] FLOPPY DRIVER since 5.10.20 2021-07-26 11:17 ` Mark Hounschell @ 2021-07-26 11:37 ` Denis Efremov 2021-07-26 12:23 ` Mark Hounschell 0 siblings, 1 reply; 13+ messages in thread From: Denis Efremov @ 2021-07-26 11:37 UTC (permalink / raw) To: dmarkh, markh, linux-block, Linux-kernel; +Cc: Jiri Kosina, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] FLOPPY DRIVER since 5.10.20 2021-07-26 11:37 ` Denis Efremov @ 2021-07-26 12:23 ` Mark Hounschell 2021-07-26 16:34 ` Denis Efremov 0 siblings, 1 reply; 13+ messages in thread From: Mark Hounschell @ 2021-07-26 12:23 UTC (permalink / raw) To: Denis Efremov, dmarkh, linux-block, Linux-kernel Cc: Jiri Kosina, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] FLOPPY DRIVER since 5.10.20 2021-07-26 12:23 ` Mark Hounschell @ 2021-07-26 16:34 ` Denis Efremov 2021-07-30 5:15 ` Denis Efremov 0 siblings, 1 reply; 13+ messages in thread From: Denis Efremov @ 2021-07-26 16:34 UTC (permalink / raw) To: markh, dmarkh, linux-block, Linux-kernel, Wim Osterholt, Jiri Kosina Cc: Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] FLOPPY DRIVER since 5.10.20 2021-07-26 16:34 ` Denis Efremov @ 2021-07-30 5:15 ` Denis Efremov 2021-08-01 11:14 ` Kurt Garloff 0 siblings, 1 reply; 13+ messages in thread From: Denis Efremov @ 2021-07-30 5:15 UTC (permalink / raw) To: linux-block, Linux-kernel, Jiri Kosina, Kurt Garloff Cc: Greg Kroah-Hartman, Wim Osterholt, dmarkh, markh 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [BUG] FLOPPY DRIVER since 5.10.20 2021-07-30 5:15 ` Denis Efremov @ 2021-08-01 11:14 ` Kurt Garloff 0 siblings, 0 replies; 13+ messages in thread From: Kurt Garloff @ 2021-08-01 11:14 UTC (permalink / raw) To: Denis Efremov, linux-block, Linux-kernel, Jiri Kosina Cc: Greg Kroah-Hartman, Wim Osterholt, dmarkh, markh 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 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] Revert "floppy: reintroduce O_NDELAY fix" 2021-07-23 18:47 [BUG] FLOPPY DRIVER since 5.10.20 Mark Hounschell 2021-07-26 7:57 ` Denis Efremov @ 2021-08-08 7:42 ` Denis Efremov 2021-08-16 7:17 ` Jiri Kosina 2021-08-30 9:20 ` Thorsten Leemhuis 1 sibling, 2 replies; 13+ messages in thread From: Denis Efremov @ 2021-08-08 7:42 UTC (permalink / raw) To: linux-block Cc: linux-kernel, regressions, Denis Efremov, Mark Hounschell, Jiri Kosina, Wim Osterholt, Kurt Garloff, stable 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Revert "floppy: reintroduce O_NDELAY fix" 2021-08-08 7:42 ` [PATCH] Revert "floppy: reintroduce O_NDELAY fix" Denis Efremov @ 2021-08-16 7:17 ` Jiri Kosina 2021-08-18 15:53 ` Denis Efremov 2021-08-30 9:20 ` Thorsten Leemhuis 1 sibling, 1 reply; 13+ messages in thread From: Jiri Kosina @ 2021-08-16 7:17 UTC (permalink / raw) To: Denis Efremov Cc: linux-block, linux-kernel, regressions, Mark Hounschell, Wim Osterholt, Kurt Garloff, stable 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Revert "floppy: reintroduce O_NDELAY fix" 2021-08-16 7:17 ` Jiri Kosina @ 2021-08-18 15:53 ` Denis Efremov 0 siblings, 0 replies; 13+ messages in thread From: Denis Efremov @ 2021-08-18 15:53 UTC (permalink / raw) To: Jiri Kosina Cc: linux-block, linux-kernel, regressions, Mark Hounschell, Wim Osterholt, Kurt Garloff, stable 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] Revert "floppy: reintroduce O_NDELAY fix" 2021-08-08 7:42 ` [PATCH] Revert "floppy: reintroduce O_NDELAY fix" Denis Efremov 2021-08-16 7:17 ` Jiri Kosina @ 2021-08-30 9:20 ` Thorsten Leemhuis 1 sibling, 0 replies; 13+ messages in thread From: Thorsten Leemhuis @ 2021-08-30 9:20 UTC (permalink / raw) To: Denis Efremov, linux-block Cc: linux-kernel, regressions, Mark Hounschell, Jiri Kosina, Wim Osterholt, Kurt Garloff, stable 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-08-30 9:44 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-23 18:47 [BUG] FLOPPY DRIVER since 5.10.20 Mark Hounschell 2021-07-26 7:57 ` Denis Efremov 2021-07-26 11:15 ` Mark Hounschell 2021-07-26 11:17 ` Mark Hounschell 2021-07-26 11:37 ` Denis Efremov 2021-07-26 12:23 ` Mark Hounschell 2021-07-26 16:34 ` Denis Efremov 2021-07-30 5:15 ` Denis Efremov 2021-08-01 11:14 ` Kurt Garloff 2021-08-08 7:42 ` [PATCH] Revert "floppy: reintroduce O_NDELAY fix" Denis Efremov 2021-08-16 7:17 ` Jiri Kosina 2021-08-18 15:53 ` Denis Efremov 2021-08-30 9:20 ` Thorsten Leemhuis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).