[PATCH 0/2] libblkid: don't use O_NONBLOCK for floppy

[PATCH 0/2] libblkid: don't use O_NONBLOCK for floppy

[Vladimir Sementsov-Ogievskiy]

Good day everyone!

The commit "floppy: reintroduce O_NDELAY fix" was removed from kernel,
so we faced the bug described and discussed here:
https://bugzilla.suse.com/show_bug.cgi?id=1181018

Discussion in kernel list on reverting the commit:
https://www.spinics.net/lists/stable/msg493061.html

In short, I can quote Jiri Kosina's comment:

   opening floppy device node with O_NONBLOCK is asking for all kinds
   of trouble

So opening floppy with O_NONBLOCK in blkid leads to failure of blkid,
probable failure of mount and unpleasant error messages in dmesg (see
also patch 02 for details).

Vladimir Sementsov-Ogievskiy (2):
  libblkid: introduce blkid_safe_open
  libblkid: reopen floppy without O_NONBLOCK

 libblkid/src/blkid.h.in   |  2 ++
 libblkid/src/evaluate.c   |  2 +-
 libblkid/src/libblkid.sym |  4 ++++
 libblkid/src/probe.c      | 44 ++++++++++++++++++++++++++++++++++++++-
 libblkid/src/verify.c     |  2 +-
 misc-utils/blkid.c        |  2 +-
 misc-utils/wipefs.c       |  2 +-
 7 files changed, 53 insertions(+), 5 deletions(-)

-- 
2.31.1

[PATCH 1/2] libblkid: introduce blkid_safe_open

[Vladimir Sementsov-Ogievskiy]

Commit 39f5af25982d8b0244000e92a9d0e0e6557d0e17 modified open() calls
used to get fds for probing by adding O_NONBLOCK flag.

We want to modify this logic in the following commit. So, as a first
step create a generic wrapper on open, so blkid-related open logic live
in one place.

Formally blkid_safe_open() becomes recommended method to open device
file for probing, as it workarounds problems of opening CDROM (and in
further patch also of FLOPPY). So, it's good to make a public API
function.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 libblkid/src/blkid.h.in   |  2 ++
 libblkid/src/evaluate.c   |  2 +-
 libblkid/src/libblkid.sym |  4 ++++
 libblkid/src/probe.c      | 19 ++++++++++++++++++-
 libblkid/src/verify.c     |  2 +-
 misc-utils/blkid.c        |  2 +-
 misc-utils/wipefs.c       |  2 +-
 7 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/libblkid/src/blkid.h.in b/libblkid/src/blkid.h.in
index 3cd4116d9..7ecf4f9be 100644
--- a/libblkid/src/blkid.h.in
+++ b/libblkid/src/blkid.h.in
@@ -217,6 +217,8 @@ extern char *blkid_evaluate_spec(const char *spec, blkid_cache *cache)
 			__ul_attribute__((warn_unused_result));
 
 /* probe.c */
+extern int blkid_safe_open(const char *filename, int mode)
+			__ul_attribute__((warn_unused_result));
 extern blkid_probe blkid_new_probe(void)
 			__ul_attribute__((warn_unused_result));
 extern blkid_probe blkid_new_probe_from_filename(const char *filename)
diff --git a/libblkid/src/evaluate.c b/libblkid/src/evaluate.c
index 710eac956..1eefe455c 100644
--- a/libblkid/src/evaluate.c
+++ b/libblkid/src/evaluate.c
@@ -73,7 +73,7 @@ static int verify_tag(const char *devname, const char *name, const char *value)
 	blkid_probe_enable_partitions(pr, TRUE);
 	blkid_probe_set_partitions_flags(pr, BLKID_PARTS_ENTRY_DETAILS);
 
-	fd = open(devname, O_RDONLY|O_CLOEXEC|O_NONBLOCK);
+	fd = blkid_safe_open(devname, O_RDONLY|O_CLOEXEC);
 	if (fd < 0) {
 		errsv = errno;
 		goto done;
diff --git a/libblkid/src/libblkid.sym b/libblkid/src/libblkid.sym
index 366f2c0c0..1549a29f4 100644
--- a/libblkid/src/libblkid.sym
+++ b/libblkid/src/libblkid.sym
@@ -183,3 +183,7 @@ BLKID_2_37 {
 	blkid_probe_set_hint;
 	blkid_probe_reset_hints;
 } BLKID_2_36;
+
+BLKID_2_38 {
+	blkid_safe_open;
+} BLKID_2_37;
diff --git a/libblkid/src/probe.c b/libblkid/src/probe.c
index 3685ea5e1..70e3dc0eb 100644
--- a/libblkid/src/probe.c
+++ b/libblkid/src/probe.c
@@ -189,6 +189,23 @@ blkid_probe blkid_clone_probe(blkid_probe parent)
 }
 
 
+/**
+ * blkid_safe_open
+ * @filename: device or regular file
+ * @mode: open mode
+ *
+ * This wrapper is blkid-specific wrapper on open(). It's "safe" in a meaning
+ * that it doesn't change
+ *
+ * We add O_NONBLOCK flag to the mode, as opening CDROM without this flag may
+ * load to closing the rom (if it's open), which is bad thing in context of
+ * blkid: we don't want to change the actual device state.
+ */
+int blkid_safe_open(const char *filename, int mode)
+{
+	return open(filename, mode | O_NONBLOCK);
+}
+
 
 /**
  * blkid_new_probe_from_filename:
@@ -208,7 +225,7 @@ blkid_probe blkid_new_probe_from_filename(const char *filename)
 	int fd;
 	blkid_probe pr = NULL;
 
-	fd = open(filename, O_RDONLY|O_CLOEXEC|O_NONBLOCK);
+	fd = blkid_safe_open(filename, O_RDONLY|O_CLOEXEC);
 	if (fd < 0)
 		return NULL;
 
diff --git a/libblkid/src/verify.c b/libblkid/src/verify.c
index 3b9754f57..96b43634c 100644
--- a/libblkid/src/verify.c
+++ b/libblkid/src/verify.c
@@ -126,7 +126,7 @@ blkid_dev blkid_verify(blkid_cache cache, blkid_dev dev)
 		}
 	}
 
-	fd = open(dev->bid_name, O_RDONLY|O_CLOEXEC|O_NONBLOCK);
+	fd = blkid_safe_open(dev->bid_name, O_RDONLY|O_CLOEXEC);
 	if (fd < 0) {
 		DBG(PROBE, ul_debug("blkid_verify: error %s (%d) while "
 					"opening %s", strerror(errno), errno,
diff --git a/misc-utils/blkid.c b/misc-utils/blkid.c
index 41826e6dc..1f7be80eb 100644
--- a/misc-utils/blkid.c
+++ b/misc-utils/blkid.c
@@ -516,7 +516,7 @@ static int lowprobe_device(blkid_probe pr, const char *devname,
 	int rc = 0;
 	static int first = 1;
 
-	fd = open(devname, O_RDONLY|O_CLOEXEC|O_NONBLOCK);
+	fd = blkid_safe_open(devname, O_RDONLY|O_CLOEXEC);
 	if (fd < 0) {
 		warn(_("error: %s"), devname);
 		return BLKID_EXIT_NOTFOUND;
diff --git a/misc-utils/wipefs.c b/misc-utils/wipefs.c
index 78dc63ee7..2597135b5 100644
--- a/misc-utils/wipefs.c
+++ b/misc-utils/wipefs.c
@@ -390,7 +390,7 @@ new_probe(const char *devname, int mode)
 		return NULL;
 
 	if (mode) {
-		int fd = open(devname, mode | O_NONBLOCK);
+		int fd = blkid_safe_open(devname, mode);
 		if (fd < 0)
 			goto error;
 
-- 
2.31.1

[PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Vladimir Sementsov-Ogievskiy]

Since c7e9d0020361f4308a70cdfd6d5335e273eb8717
"Revert "floppy: reintroduce O_NDELAY fix"" commit in linux kernel,
floppy drive works bad when opened with O_NONBLOCK: first read may
fail. This cause probing fail and leave error messages in dmesg. So, if
we detect that openedfd is floppy, reopen it without O_NONBLOCK flag.

Reproduce is simple:
1. start the linux system (kernel should include the mentioned commit)
   in QEMU virtual machine with floppy device and with floppy disk
   inserted.
2. If floppy module is not inserted, modprobe it.
3. Try "blkid /dev/fd9": it will show nothing, errors will appear in
   dmesg
4. Try "mount /dev/fd0 /mnt": it may succeed (as mount not only probing
   but also try filesystems one by one, if you have vfat in
   /etc/filesytems or in /proc/filesystems, mount will succeed), but
   errors about failed read still appear in dmesg, as probing was done.

Mentioned errors in dmesg looks like this:
 floppy0: disk absent or changed during operation
 blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
 floppy0: disk absent or changed during operation
 blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
 Buffer I/O error on dev fd0, logical block 0, async page read

Note also, that these errors also appear in early dmesg messages, if
probing is done on system startup. For example, it happens when
cloud-init package is installed.

Note2: O_NONBLOCK flag for probing is used since
39f5af25982d8b0244000e92a9d0e0e6557d0e17
"libblkid: open device in nonblock mode", which was done to fix the
issue with cdrom: if tray is open and we call open() without O_NONBLOCK
the tray may be automatically closed, which is not what we want in
blkid.

Good discussion on this bug is here:
https://bugzilla.suse.com/show_bug.cgi?id=1181018

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

Note, that this commit is done as a "minimal change", i.e. I only try to
rollback O_NONBLOCK for floppy. The other way is to detect CDROM
instead, and reopen with original flags for everything except CDROM.

I also tried fcntl instead of close/open, and that didn't help.

 libblkid/src/probe.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/libblkid/src/probe.c b/libblkid/src/probe.c
index 70e3dc0eb..68a644597 100644
--- a/libblkid/src/probe.c
+++ b/libblkid/src/probe.c
@@ -94,6 +94,9 @@
 #ifdef HAVE_LINUX_CDROM_H
 #include <linux/cdrom.h>
 #endif
+#ifdef HAVE_LINUX_FD_H
+#include <linux/fd.h>
+#endif
 #ifdef HAVE_LINUX_BLKZONED_H
 #include <linux/blkzoned.h>
 #endif
@@ -200,10 +203,32 @@ blkid_probe blkid_clone_probe(blkid_probe parent)
  * We add O_NONBLOCK flag to the mode, as opening CDROM without this flag may
  * load to closing the rom (if it's open), which is bad thing in context of
  * blkid: we don't want to change the actual device state.
+ *
+ * Still, since c7e9d0020361f4308a70cdfd6d5335e273eb8717
+ * "Revert "floppy: reintroduce O_NDELAY fix"" commit in linux kernel, floppy
+ * drive works bad when opened with O_NONBLOCK: first read may fail. This cause
+ * probing fail and leave error messages in dmesg. So, if we detect that opened
+ * fd is floppy, reopen it without O_NONBLOCK flag.
  */
 int blkid_safe_open(const char *filename, int mode)
 {
-	return open(filename, mode | O_NONBLOCK);
+	int fd = open(filename, mode | O_NONBLOCK);
+	if (fd < 0) {
+		return fd;
+	}
+
+#ifdef FDGETDRVTYP
+	{
+		char name[1000];
+
+		if (ioctl(fd, FDGETDRVTYP, &name) >= 0) {
+			close(fd);
+			fd = open(filename, mode);
+		}
+	}
+#endif /* FDGETDRVTYP */
+
+	return fd;
 }
 
 
-- 
2.31.1

Re: [PATCH 0/2] libblkid: don't use O_NONBLOCK for floppy

[Jiri Kosina]

On Thu, 9 Dec 2021, Vladimir Sementsov-Ogievskiy wrote:

> Good day everyone!
> 
> The commit "floppy: reintroduce O_NDELAY fix" was removed from kernel,
> so we faced the bug described and discussed here:
> https://bugzilla.suse.com/show_bug.cgi?id=1181018
> 
> Discussion in kernel list on reverting the commit:
> https://www.spinics.net/lists/stable/msg493061.html
> 
> In short, I can quote Jiri Kosina's comment:
> 
>    opening floppy device node with O_NONBLOCK is asking for all kinds
>    of trouble
> 
> So opening floppy with O_NONBLOCK in blkid leads to failure of blkid,
> probable failure of mount and unpleasant error messages in dmesg (see
> also patch 02 for details).
> 
> Vladimir Sementsov-Ogievskiy (2):
>   libblkid: introduce blkid_safe_open
>   libblkid: reopen floppy without O_NONBLOCK

Ah, thanks, finally someone found the motivation to look into what it'd 
take to fix this properly in libblkid. FWIW,

	Acked-by: Jiri Kosina <jkosina@suse.cz>

for the changes.

Thank you!

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/2] libblkid: don't use O_NONBLOCK for floppy

[Kurt Garloff]

Hi,

On 14/12/2021 11:25, Jiri Kosina wrote:
> On Thu, 9 Dec 2021, Vladimir Sementsov-Ogievskiy wrote:
>
>> Good day everyone!
>>
>> The commit "floppy: reintroduce O_NDELAY fix" was removed from kernel,
>> so we faced the bug described and discussed here:
>> https://bugzilla.suse.com/show_bug.cgi?id=1181018
>>
>> Discussion in kernel list on reverting the commit:
>> https://www.spinics.net/lists/stable/msg493061.html
>>
>> In short, I can quote Jiri Kosina's comment:
>>
>>     opening floppy device node with O_NONBLOCK is asking for all kinds
>>     of trouble
>>
>> So opening floppy with O_NONBLOCK in blkid leads to failure of blkid,
>> probable failure of mount and unpleasant error messages in dmesg (see
>> also patch 02 for details).
>>
>> Vladimir Sementsov-Ogievskiy (2):
>>    libblkid: introduce blkid_safe_open
>>    libblkid: reopen floppy without O_NONBLOCK
> Ah, thanks, finally someone found the motivation to look into what it'd
> take to fix this properly in libblkid. FWIW,
>
> 	Acked-by: Jiri Kosina <jkosina@suse.cz>
>
> for the changes.

As the one who was affected by the issue (having some old scripts that
use floppy images to inject metadata as data source for cloud-init
instead of CD-Rom images), I definitely appreciate this being taken
care of. Thank you!

Changes look good to me, but I guess best would be to test things.
Let me know if you need me to do this ...

Best,

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

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Michal Suchánek]

On Thu, Dec 09, 2021 at 03:12:33PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Since c7e9d0020361f4308a70cdfd6d5335e273eb8717
> "Revert "floppy: reintroduce O_NDELAY fix"" commit in linux kernel,
> floppy drive works bad when opened with O_NONBLOCK: first read may
> fail. This cause probing fail and leave error messages in dmesg. So, if
> we detect that openedfd is floppy, reopen it without O_NONBLOCK flag.
> 
> Reproduce is simple:
> 1. start the linux system (kernel should include the mentioned commit)
>    in QEMU virtual machine with floppy device and with floppy disk
>    inserted.
> 2. If floppy module is not inserted, modprobe it.
> 3. Try "blkid /dev/fd9": it will show nothing, errors will appear in
>    dmesg
> 4. Try "mount /dev/fd0 /mnt": it may succeed (as mount not only probing
>    but also try filesystems one by one, if you have vfat in
>    /etc/filesytems or in /proc/filesystems, mount will succeed), but
>    errors about failed read still appear in dmesg, as probing was done.
> 
> Mentioned errors in dmesg looks like this:
>  floppy0: disk absent or changed during operation
>  blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
>  floppy0: disk absent or changed during operation
>  blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
>  Buffer I/O error on dev fd0, logical block 0, async page read
> 
> Note also, that these errors also appear in early dmesg messages, if
> probing is done on system startup. For example, it happens when
> cloud-init package is installed.
> 
> Note2: O_NONBLOCK flag for probing is used since
> 39f5af25982d8b0244000e92a9d0e0e6557d0e17
> "libblkid: open device in nonblock mode", which was done to fix the
> issue with cdrom: if tray is open and we call open() without O_NONBLOCK
> the tray may be automatically closed, which is not what we want in
> blkid.
> 
> Good discussion on this bug is here:
> https://bugzilla.suse.com/show_bug.cgi?id=1181018
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> Note, that this commit is done as a "minimal change", i.e. I only try to
> rollback O_NONBLOCK for floppy. The other way is to detect CDROM
> instead, and reopen with original flags for everything except CDROM.
> 
> I also tried fcntl instead of close/open, and that didn't help.
> 
>  libblkid/src/probe.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/libblkid/src/probe.c b/libblkid/src/probe.c
> index 70e3dc0eb..68a644597 100644
> --- a/libblkid/src/probe.c
> +++ b/libblkid/src/probe.c
> @@ -94,6 +94,9 @@
>  #ifdef HAVE_LINUX_CDROM_H
>  #include <linux/cdrom.h>
>  #endif
> +#ifdef HAVE_LINUX_FD_H
> +#include <linux/fd.h>
> +#endif
>  #ifdef HAVE_LINUX_BLKZONED_H
>  #include <linux/blkzoned.h>
>  #endif
> @@ -200,10 +203,32 @@ blkid_probe blkid_clone_probe(blkid_probe parent)
>   * We add O_NONBLOCK flag to the mode, as opening CDROM without this flag may
>   * load to closing the rom (if it's open), which is bad thing in context of
>   * blkid: we don't want to change the actual device state.
> + *
> + * Still, since c7e9d0020361f4308a70cdfd6d5335e273eb8717
> + * "Revert "floppy: reintroduce O_NDELAY fix"" commit in linux kernel, floppy
> + * drive works bad when opened with O_NONBLOCK: first read may fail. This cause
> + * probing fail and leave error messages in dmesg. So, if we detect that opened
> + * fd is floppy, reopen it without O_NONBLOCK flag.
>   */
>  int blkid_safe_open(const char *filename, int mode)
>  {
> -	return open(filename, mode | O_NONBLOCK);
> +	int fd = open(filename, mode | O_NONBLOCK);
> +	if (fd < 0) {
> +		return fd;
> +	}
> +
> +#ifdef FDGETDRVTYP
> +	{
> +		char name[1000];
Hello,

I wonder if it's better to use FDGETFDCSTAT which seems to be meant as
stable API.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/fd.h#n271

As is this allocates 1k from stack and can be presumably called from
application context with arbitrarily deep stack so it seems a bit
wasteful. floppy_fdc_state has under 60 bytes.

Also if you are not interested in the result you can make the buffer
static. Not sure it makes sense to bother if the buffer size is
reasonable.

Thanks

Michal

> +
> +		if (ioctl(fd, FDGETDRVTYP, &name) >= 0) {
> +			close(fd);
> +			fd = open(filename, mode);
> +		}
> +	}
> +#endif /* FDGETDRVTYP */
> +
> +	return fd;
>  }
>  
>  
> -- 
> 2.31.1
> 

Re: [PATCH 0/2] libblkid: don't use O_NONBLOCK for floppy

[Michal Suchánek]

On Thu, Dec 09, 2021 at 03:12:31PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Good day everyone!
> 
> The commit "floppy: reintroduce O_NDELAY fix" was removed from kernel,
> so we faced the bug described and discussed here:
> https://bugzilla.suse.com/show_bug.cgi?id=1181018
> 
> Discussion in kernel list on reverting the commit:
> https://www.spinics.net/lists/stable/msg493061.html
> 
> In short, I can quote Jiri Kosina's comment:
> 
>    opening floppy device node with O_NONBLOCK is asking for all kinds
>    of trouble
> 
> So opening floppy with O_NONBLOCK in blkid leads to failure of blkid,
> probable failure of mount and unpleasant error messages in dmesg (see
> also patch 02 for details).
> 
> Vladimir Sementsov-Ogievskiy (2):
>   libblkid: introduce blkid_safe_open
>   libblkid: reopen floppy without O_NONBLOCK

Reviewed-by: Michal Suchánek <msuchanek@suse.de>

See nit in patch 2.

Thanks

Michal
> 
>  libblkid/src/blkid.h.in   |  2 ++
>  libblkid/src/evaluate.c   |  2 +-
>  libblkid/src/libblkid.sym |  4 ++++
>  libblkid/src/probe.c      | 44 ++++++++++++++++++++++++++++++++++++++-
>  libblkid/src/verify.c     |  2 +-
>  misc-utils/blkid.c        |  2 +-
>  misc-utils/wipefs.c       |  2 +-
>  7 files changed, 53 insertions(+), 5 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Karel Zak]

 Hi Vladimir,

On Thu, Dec 09, 2021 at 03:12:33PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Note, that this commit is done as a "minimal change", i.e. I only try to
> rollback O_NONBLOCK for floppy. The other way is to detect CDROM
> instead, and reopen with original flags for everything except CDROM.
> 
> I also tried fcntl instead of close/open, and that didn't help.

What does it mean didn't help?             

I guess that drop O_NONBLOCk in blkid_probe_set_device() for floppies
would be enough, something like:           

int blkid_probe_set_device(blkid_probe pr, int fd,  
                blkid_loff_t off, blkid_loff_t size)
{                                          
   ...                                     

    if (ioctl(fd, FDGETDRVTYP, &name) >= 0) {       
        int flags;                         

        if ((flags = fcntl(fd, F_GETFL, 0)) != -1)  
            fcntl(fd, F_SETFL, flags & ~O_NONBLOCK);          
    }                                      
   ...                                     
}                                          

Yes, it's a little bit dirty to modify FD in the library (if the FD is
provided by the application), but if O_NONBLOCK is wrong in all cases for
floppies, then it seems like a good thing.    

This solution will fix the problem without libblkid API change, and it
will fix it in all current applications.   

The solution based on blkid_safe_open() means that we have to modify
many applications. For example, systemd/udevd uses   
                                           

    fd = open(devnode, O_RDONLY|O_CLOEXEC|O_NONBLOCK);              
    r = blkid_probe_set_device(pr, fd, offset, 0);            

The same is probably in many other places (mkfs-like programs, etc.).

What do you think?

 Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Vladimir Sementsov-Ogievskiy]

14.12.2021 14:45, Michal Suchánek wrote:
> On Thu, Dec 09, 2021 at 03:12:33PM +0100, Vladimir Sementsov-Ogievskiy wrote:
>> Since c7e9d0020361f4308a70cdfd6d5335e273eb8717
>> "Revert "floppy: reintroduce O_NDELAY fix"" commit in linux kernel,
>> floppy drive works bad when opened with O_NONBLOCK: first read may
>> fail. This cause probing fail and leave error messages in dmesg. So, if
>> we detect that openedfd is floppy, reopen it without O_NONBLOCK flag.
>>
>> Reproduce is simple:
>> 1. start the linux system (kernel should include the mentioned commit)
>>     in QEMU virtual machine with floppy device and with floppy disk
>>     inserted.
>> 2. If floppy module is not inserted, modprobe it.
>> 3. Try "blkid /dev/fd9": it will show nothing, errors will appear in
>>     dmesg
>> 4. Try "mount /dev/fd0 /mnt": it may succeed (as mount not only probing
>>     but also try filesystems one by one, if you have vfat in
>>     /etc/filesytems or in /proc/filesystems, mount will succeed), but
>>     errors about failed read still appear in dmesg, as probing was done.
>>
>> Mentioned errors in dmesg looks like this:
>>   floppy0: disk absent or changed during operation
>>   blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x80700 phys_seg 1 prio class 0
>>   floppy0: disk absent or changed during operation
>>   blk_update_request: I/O error, dev fd0, sector 0 op 0x0:(READ) flags 0x0 phys_seg 1 prio class 0
>>   Buffer I/O error on dev fd0, logical block 0, async page read
>>
>> Note also, that these errors also appear in early dmesg messages, if
>> probing is done on system startup. For example, it happens when
>> cloud-init package is installed.
>>
>> Note2: O_NONBLOCK flag for probing is used since
>> 39f5af25982d8b0244000e92a9d0e0e6557d0e17
>> "libblkid: open device in nonblock mode", which was done to fix the
>> issue with cdrom: if tray is open and we call open() without O_NONBLOCK
>> the tray may be automatically closed, which is not what we want in
>> blkid.
>>
>> Good discussion on this bug is here:
>> https://bugzilla.suse.com/show_bug.cgi?id=1181018
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> Note, that this commit is done as a "minimal change", i.e. I only try to
>> rollback O_NONBLOCK for floppy. The other way is to detect CDROM
>> instead, and reopen with original flags for everything except CDROM.
>>
>> I also tried fcntl instead of close/open, and that didn't help.
>>
>>   libblkid/src/probe.c | 27 ++++++++++++++++++++++++++-
>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/libblkid/src/probe.c b/libblkid/src/probe.c
>> index 70e3dc0eb..68a644597 100644
>> --- a/libblkid/src/probe.c
>> +++ b/libblkid/src/probe.c
>> @@ -94,6 +94,9 @@
>>   #ifdef HAVE_LINUX_CDROM_H
>>   #include <linux/cdrom.h>
>>   #endif
>> +#ifdef HAVE_LINUX_FD_H
>> +#include <linux/fd.h>
>> +#endif
>>   #ifdef HAVE_LINUX_BLKZONED_H
>>   #include <linux/blkzoned.h>
>>   #endif
>> @@ -200,10 +203,32 @@ blkid_probe blkid_clone_probe(blkid_probe parent)
>>    * We add O_NONBLOCK flag to the mode, as opening CDROM without this flag may
>>    * load to closing the rom (if it's open), which is bad thing in context of
>>    * blkid: we don't want to change the actual device state.
>> + *
>> + * Still, since c7e9d0020361f4308a70cdfd6d5335e273eb8717
>> + * "Revert "floppy: reintroduce O_NDELAY fix"" commit in linux kernel, floppy
>> + * drive works bad when opened with O_NONBLOCK: first read may fail. This cause
>> + * probing fail and leave error messages in dmesg. So, if we detect that opened
>> + * fd is floppy, reopen it without O_NONBLOCK flag.
>>    */
>>   int blkid_safe_open(const char *filename, int mode)
>>   {
>> -	return open(filename, mode | O_NONBLOCK);
>> +	int fd = open(filename, mode | O_NONBLOCK);
>> +	if (fd < 0) {
>> +		return fd;
>> +	}
>> +
>> +#ifdef FDGETDRVTYP
>> +	{
>> +		char name[1000];
> Hello,
> 
> I wonder if it's better to use FDGETFDCSTAT which seems to be meant as
> stable API.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/fd.h#n271
> 
> As is this allocates 1k from stack and can be presumably called from
> application context with arbitrarily deep stack so it seems a bit
> wasteful. floppy_fdc_state has under 60 bytes.
> 
> Also if you are not interested in the result you can make the buffer
> static. Not sure it makes sense to bother if the buffer size is
> reasonable.
> 

Thanks for advice, will do for v2. I'm rather far of kernel development, and not fan of floppy disks:) So my choice was "something I can understand from drivers/block/floppy.c and which never fail". I tried FDGETPRM (because it used somewhere else in util-linux), but it fails in described case.. and in kernel code it's obvious that it may fail.

> 
>> +
>> +		if (ioctl(fd, FDGETDRVTYP, &name) >= 0) {
>> +			close(fd);
>> +			fd = open(filename, mode);
>> +		}
>> +	}
>> +#endif /* FDGETDRVTYP */
>> +
>> +	return fd;
>>   }
>>   
>>   
>> -- 
>> 2.31.1
>>


-- 
Best regards,
Vladimir

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Vladimir Sementsov-Ogievskiy]

14.12.2021 15:03, Karel Zak wrote:
> 
>   Hi Vladimir,
> 
> On Thu, Dec 09, 2021 at 03:12:33PM +0100, Vladimir Sementsov-Ogievskiy wrote:
>> Note, that this commit is done as a "minimal change", i.e. I only try to
>> rollback O_NONBLOCK for floppy. The other way is to detect CDROM
>> instead, and reopen with original flags for everything except CDROM.
>>
>> I also tried fcntl instead of close/open, and that didn't help.
> 
> What does it mean didn't help?

I tried it, but the bug remains as it was.. As I understand, dropping O_NONBLOCK by fcntl just removes this flag, to change further behavior of device to non-blocking. But it doesn't do any additional actions in driver.

In floppy.c the only place where FMODE_NDELAY take place is floppy_open(). So, we want to change the behavior exactly of thet open() call. As I understand fcntl() doesn't trigger floppy_open() call, that's why it can't help.

> 
> I guess that drop O_NONBLOCk in blkid_probe_set_device() for floppies
> would be enough, something like:
> 
> int blkid_probe_set_device(blkid_probe pr, int fd,
>                  blkid_loff_t off, blkid_loff_t size)
> {
>     ...
> 
>      if (ioctl(fd, FDGETDRVTYP, &name) >= 0) {
>          int flags;
> 
>          if ((flags = fcntl(fd, F_GETFL, 0)) != -1)
>              fcntl(fd, F_SETFL, flags & ~O_NONBLOCK);
>      }
>     ...
> }
> 
> Yes, it's a little bit dirty to modify FD in the library (if the FD is
> provided by the application), but if O_NONBLOCK is wrong in all cases for
> floppies, then it seems like a good thing.
> 
> This solution will fix the problem without libblkid API change, and it
> will fix it in all current applications.
> 
> The solution based on blkid_safe_open() means that we have to modify
> many applications. For example, systemd/udevd uses
>                                             
> 
>      fd = open(devnode, O_RDONLY|O_CLOEXEC|O_NONBLOCK);
>      r = blkid_probe_set_device(pr, fd, offset, 0);

Hmm, yes, that's not very good :(

> 
> The same is probably in many other places (mkfs-like programs, etc.).
> 
> What do you think?
> 

Maybe be we can get filename from fd reading it from /proc, then do open() and than dup() to old fd.. But that's even more dirty to do in a library call.



-- 
Best regards,
Vladimir

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Karel Zak]

On Tue, Dec 14, 2021 at 05:24:44PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > What does it mean didn't help?
> 
> I tried it, but the bug remains as it was.. As I understand,
> dropping O_NONBLOCK by fcntl just removes this flag, to change
> further behavior of device to non-blocking. But it doesn't do any
> additional actions in driver.

Hmm ...

> > The solution based on blkid_safe_open() means that we have to modify
> > many applications. For example, systemd/udevd uses
> > 
> >      fd = open(devnode, O_RDONLY|O_CLOEXEC|O_NONBLOCK);
> >      r = blkid_probe_set_device(pr, fd, offset, 0);
> 
> Hmm, yes, that's not very good :(
> 
> > 
> > The same is probably in many other places (mkfs-like programs, etc.).
> > 
> > What do you think?
> > 
> 
> Maybe be we can get filename from fd reading it from /proc, then do
> open() and than dup() to old fd.. But that's even more dirty to do
> in a library call.

Yes, re-open() sounds like a possible way.

The dup() is not necessary. The library already support private FD for
probing. See blkid_new_probe_from_filename() and BLKID_FL_PRIVATE_FD.

It's also often used when the library probe whole-disk device (when you
probe sda1 than it also reads data from partition table on sda). In
this case it opens a separate FD.

It's for floppies (0.001% of all cases). IMHO re-open is good enough and
better than force changes in all applications :-)

I'll try to prepare a patch and ask you for test/review. Thanks!

 Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Vladimir Sementsov-Ogievskiy]

15.12.2021 15:56, Karel Zak wrote:
> On Tue, Dec 14, 2021 at 05:24:44PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> What does it mean didn't help?
>>
>> I tried it, but the bug remains as it was.. As I understand,
>> dropping O_NONBLOCK by fcntl just removes this flag, to change
>> further behavior of device to non-blocking. But it doesn't do any
>> additional actions in driver.
> 
> Hmm ...
> 
>>> The solution based on blkid_safe_open() means that we have to modify
>>> many applications. For example, systemd/udevd uses
>>>
>>>       fd = open(devnode, O_RDONLY|O_CLOEXEC|O_NONBLOCK);
>>>       r = blkid_probe_set_device(pr, fd, offset, 0);
>>
>> Hmm, yes, that's not very good :(
>>
>>>
>>> The same is probably in many other places (mkfs-like programs, etc.).
>>>
>>> What do you think?
>>>
>>
>> Maybe be we can get filename from fd reading it from /proc, then do
>> open() and than dup() to old fd.. But that's even more dirty to do
>> in a library call.
> 
> Yes, re-open() sounds like a possible way.
> 
> The dup() is not necessary. The library already support private FD for
> probing. See blkid_new_probe_from_filename() and BLKID_FL_PRIVATE_FD.
> 
> It's also often used when the library probe whole-disk device (when you
> probe sda1 than it also reads data from partition table on sda). In
> this case it opens a separate FD.
> 
> It's for floppies (0.001% of all cases). IMHO re-open is good enough and
> better than force changes in all applications :-)> 
> I'll try to prepare a patch and ask you for test/review. Thanks!
> 

Great!

-- 
Best regards,
Vladimir

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Vladimir Sementsov-Ogievskiy]

16.12.2021 11:47, Vladimir Sementsov-Ogievskiy wrote:
> 15.12.2021 15:56, Karel Zak wrote:
>> On Tue, Dec 14, 2021 at 05:24:44PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> What does it mean didn't help?
>>>
>>> I tried it, but the bug remains as it was.. As I understand,
>>> dropping O_NONBLOCK by fcntl just removes this flag, to change
>>> further behavior of device to non-blocking. But it doesn't do any
>>> additional actions in driver.
>>
>> Hmm ...
>>
>>>> The solution based on blkid_safe_open() means that we have to modify
>>>> many applications. For example, systemd/udevd uses
>>>>
>>>>       fd = open(devnode, O_RDONLY|O_CLOEXEC|O_NONBLOCK);
>>>>       r = blkid_probe_set_device(pr, fd, offset, 0);
>>>
>>> Hmm, yes, that's not very good :(
>>>
>>>>
>>>> The same is probably in many other places (mkfs-like programs, etc.).
>>>>
>>>> What do you think?
>>>>
>>>
>>> Maybe be we can get filename from fd reading it from /proc, then do
>>> open() and than dup() to old fd.. But that's even more dirty to do
>>> in a library call.
>>
>> Yes, re-open() sounds like a possible way.
>>
>> The dup() is not necessary. The library already support private FD for
>> probing. See blkid_new_probe_from_filename() and BLKID_FL_PRIVATE_FD.
>>
>> It's also often used when the library probe whole-disk device (when you
>> probe sda1 than it also reads data from partition table on sda). In
>> this case it opens a separate FD.
>>
>> It's for floppies (0.001% of all cases). IMHO re-open is good enough and
>> better than force changes in all applications :-)> I'll try to prepare a patch and ask you for test/review. Thanks!
>>
> 
> Great!
> 

Hi! Are you working on this? If not I can try to make a v2.

-- 
Best regards,
Vladimir

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Karel Zak]

On Fri, Dec 24, 2021 at 06:31:39PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > > The same is probably in many other places (mkfs-like programs, etc.).
> > > > > 
> > > > > What do you think?
> > > > > 
> > > > 
> > > > Maybe be we can get filename from fd reading it from /proc, then do
> > > > open() and than dup() to old fd.. But that's even more dirty to do
> > > > in a library call.
> > > 
> > > Yes, re-open() sounds like a possible way.
> > > 
> > > The dup() is not necessary. The library already support private FD for
> > > probing. See blkid_new_probe_from_filename() and BLKID_FL_PRIVATE_FD.
> > > 
> > > It's also often used when the library probe whole-disk device (when you
> > > probe sda1 than it also reads data from partition table on sda). In
> > > this case it opens a separate FD.
> > > 
> > > It's for floppies (0.001% of all cases). IMHO re-open is good enough and
> > > better than force changes in all applications :-)> I'll try to prepare a patch and ask you for test/review. Thanks!
> > > 
> > 
> > Great!
> > 
> 
> Hi! Are you working on this? If not I can try to make a v2.


I had a vacation in the last 14 days; nothing is done. Go ahead if you   
have time for this task. I'm going to work on something else this
week.

  Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Karel Zak]

On Mon, Jan 03, 2022 at 09:43:11AM +0100, Karel Zak wrote:
> > Hi! Are you working on this? If not I can try to make a v2.
> 
> I had a vacation in the last 14 days; nothing is done. Go ahead if you   
> have time for this task. I'm going to work on something else this
> week.

It seems you're busy with something else ;-), so I have implemented it,
but it's not tested yet.

Please, please, can you review and test it with your environment?

Thanks!

See the patch bellow or topic/blkid-floppy branch on github.

 Karel


From 49be1b256ad351cde2e0cf5000c7046e7010cc02 Mon Sep 17 00:00:00 2001
From: Karel Zak <kzak@redhat.com>
Date: Mon, 17 Jan 2022 12:37:13 +0100
Subject: [PATCH] libblkid: reopen floppy without O_NONBLOCK

Vladimir Sementsov-Ogievskiy wrote:
> The commit "floppy: reintroduce O_NDELAY fix" was removed from kernel,
> so we faced the bug described and discussed here:
> https://bugzilla.suse.com/show_bug.cgi?id=3D1181018
>
> Discussion in kernel list on reverting the commit:
> https://www.spinics.net/lists/stable/msg493061.html
>
> In short, I can quote Jiri Kosina's comment:
>
>   opening floppy device node with O_NONBLOCK is asking for all kinds
>   of trouble
>
> So opening floppy with O_NONBLOCK in blkid leads to failure of blkid,
> probable failure of mount and unpleasant error messages in dmesg (see
> also patch 02 for details).

Based on patch from Vladimir.

CC: Jiri Kosina <jkosina@suse.cz>
Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Karel Zak <kzak@redhat.com>
---
 include/fileutils.h  |  3 +++
 lib/fileutils.c      | 17 +++++++++++++++++
 libblkid/src/probe.c | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+)

diff --git a/include/fileutils.h b/include/fileutils.h
index c36ce6353..8722ed59b 100644
--- a/include/fileutils.h
+++ b/include/fileutils.h
@@ -97,4 +97,7 @@ extern void ul_close_all_fds(unsigned int first, unsigned int last);
 #define UL_COPY_WRITE_ERROR (-2)
 int ul_copy_file(int from, int to);
 
+
+extern int ul_reopen(int fd, int flags);
+
 #endif /* UTIL_LINUX_FILEUTILS */
diff --git a/lib/fileutils.c b/lib/fileutils.c
index 7a8fce26f..5b15d4916 100644
--- a/lib/fileutils.c
+++ b/lib/fileutils.c
@@ -288,3 +288,20 @@ int ul_copy_file(int from, int to)
 	return copy_file_simple(from, to);
 #endif
 }
+
+int ul_reopen(int fd, int flags)
+{
+	ssize_t ssz;
+	char buf[PATH_MAX];
+	char fdpath[ sizeof(_PATH_PROC_FDDIR) +  sizeof(stringify_value(INT_MAX)) ];
+
+	snprintf(fdpath, sizeof(fdpath), _PATH_PROC_FDDIR "/%d", fd);
+
+	ssz = readlink(fdpath, buf, sizeof(buf) - 1);
+	if (ssz <= 0)
+		return -EINVAL;
+
+	buf[ssz] = '\0';
+
+	return open(buf, flags);
+}
diff --git a/libblkid/src/probe.c b/libblkid/src/probe.c
index 6168370e2..d69ff530f 100644
--- a/libblkid/src/probe.c
+++ b/libblkid/src/probe.c
@@ -103,6 +103,9 @@
 #ifdef HAVE_ERRNO_H
 #include <errno.h>
 #endif
+#ifdef HAVE_LINUX_FD_H
+#include <linux/fd.h>
+#endif
 #include <inttypes.h>
 #include <stdint.h>
 #include <stdarg.h>
@@ -113,6 +116,7 @@
 #include "sysfs.h"
 #include "strutils.h"
 #include "list.h"
+#include "fileutils.h"
 
 /*
  * All supported chains
@@ -907,6 +911,35 @@ int blkid_probe_set_device(blkid_probe pr, int fd,
 	if (fd < 0)
 		return 1;
 
+#ifdef FDGETFDCSTAT
+	{
+		/*
+		 * Re-open without O_NONBLOCK for floppy device.
+		 *
+		 * Since kernel commit c7e9d0020361f4308a70cdfd6d5335e273eb8717
+		 * floppy drive works bad when opened with O_NONBLOCK.
+		 */
+		struct floppy_fdc_state  flst;
+
+		if (ioctl(fd, FDGETFDCSTAT, &flst) >= 0) {
+			int flags = fcntl(fd, F_GETFL, 0);
+
+			if (flags < 0)
+				goto err;
+			if (flags & O_NONBLOCK) {
+				flags &= ~O_NONBLOCK;
+
+				fd = ul_reopen(fd, flags | O_CLOEXEC);
+				if (fd < 0)
+					goto err;
+
+				pr->flags |= BLKID_FL_PRIVATE_FD;
+				pr->fd = fd;
+			}
+		}
+	}
+#endif
+
 #if defined(POSIX_FADV_RANDOM) && defined(HAVE_POSIX_FADVISE)
 	/* Disable read-ahead */
 	posix_fadvise(fd, 0, 0, POSIX_FADV_RANDOM);
-- 
2.31.1

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Vladimir Sementsov-Ogievskiy]

17.01.2022 14:50, Karel Zak wrote:
> On Mon, Jan 03, 2022 at 09:43:11AM +0100, Karel Zak wrote:
>>> Hi! Are you working on this? If not I can try to make a v2.
>>
>> I had a vacation in the last 14 days; nothing is done. Go ahead if you
>> have time for this task. I'm going to work on something else this
>> week.
> 
> It seems you're busy with something else ;-), so I have implemented it,
> but it's not tested yet.

Oh seems yes ;) Great that you've done it!

> 
> Please, please, can you review and test it with your environment?

Ohh. Will try to remember and restore the environment this week :)

> 
> Thanks!
> 
> See the patch bellow or topic/blkid-floppy branch on github.
> 
>   Karel
> 
> 
>  From 49be1b256ad351cde2e0cf5000c7046e7010cc02 Mon Sep 17 00:00:00 2001
> From: Karel Zak <kzak@redhat.com>
> Date: Mon, 17 Jan 2022 12:37:13 +0100
> Subject: [PATCH] libblkid: reopen floppy without O_NONBLOCK
> 
> Vladimir Sementsov-Ogievskiy wrote:
>> The commit "floppy: reintroduce O_NDELAY fix" was removed from kernel,
>> so we faced the bug described and discussed here:
>> https://bugzilla.suse.com/show_bug.cgi?id=3D1181018
>>
>> Discussion in kernel list on reverting the commit:
>> https://www.spinics.net/lists/stable/msg493061.html
>>
>> In short, I can quote Jiri Kosina's comment:
>>
>>    opening floppy device node with O_NONBLOCK is asking for all kinds
>>    of trouble
>>
>> So opening floppy with O_NONBLOCK in blkid leads to failure of blkid,
>> probable failure of mount and unpleasant error messages in dmesg (see
>> also patch 02 for details).
> 
> Based on patch from Vladimir.
> 
> CC: Jiri Kosina <jkosina@suse.cz>
> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Karel Zak <kzak@redhat.com>
> ---
>   include/fileutils.h  |  3 +++
>   lib/fileutils.c      | 17 +++++++++++++++++
>   libblkid/src/probe.c | 33 +++++++++++++++++++++++++++++++++
>   3 files changed, 53 insertions(+)
> 
> diff --git a/include/fileutils.h b/include/fileutils.h
> index c36ce6353..8722ed59b 100644
> --- a/include/fileutils.h
> +++ b/include/fileutils.h
> @@ -97,4 +97,7 @@ extern void ul_close_all_fds(unsigned int first, unsigned int last);
>   #define UL_COPY_WRITE_ERROR (-2)
>   int ul_copy_file(int from, int to);
>   
> +
> +extern int ul_reopen(int fd, int flags);
> +
>   #endif /* UTIL_LINUX_FILEUTILS */
> diff --git a/lib/fileutils.c b/lib/fileutils.c
> index 7a8fce26f..5b15d4916 100644
> --- a/lib/fileutils.c
> +++ b/lib/fileutils.c
> @@ -288,3 +288,20 @@ int ul_copy_file(int from, int to)
>   	return copy_file_simple(from, to);
>   #endif
>   }
> +
> +int ul_reopen(int fd, int flags)

"reopen" sounds a bit like we are going to close old fd, but we don't... No better suggestion :(

> +{
> +	ssize_t ssz;
> +	char buf[PATH_MAX];
> +	char fdpath[ sizeof(_PATH_PROC_FDDIR) +  sizeof(stringify_value(INT_MAX)) ];

Extra whitespace after '+'

Also, I think you should add +1 for '/' (you use it below), and +1 for finishing \0.

Or alternatively we may just use PATH_MAX here too and not care.

> +
> +	snprintf(fdpath, sizeof(fdpath), _PATH_PROC_FDDIR "/%d", fd);
> +
> +	ssz = readlink(fdpath, buf, sizeof(buf) - 1);
> +	if (ssz <= 0)
> +		return -EINVAL;

alternatively you may return -errno on ssz < 0, and assert that ssz > 0, I hope 0 can't be returned.

> +
> +	buf[ssz] = '\0';
> +
> +	return open(buf, flags);
> +}
> diff --git a/libblkid/src/probe.c b/libblkid/src/probe.c
> index 6168370e2..d69ff530f 100644
> --- a/libblkid/src/probe.c
> +++ b/libblkid/src/probe.c
> @@ -103,6 +103,9 @@
>   #ifdef HAVE_ERRNO_H
>   #include <errno.h>
>   #endif
> +#ifdef HAVE_LINUX_FD_H
> +#include <linux/fd.h>
> +#endif
>   #include <inttypes.h>
>   #include <stdint.h>
>   #include <stdarg.h>
> @@ -113,6 +116,7 @@
>   #include "sysfs.h"
>   #include "strutils.h"
>   #include "list.h"
> +#include "fileutils.h"
>   
>   /*
>    * All supported chains
> @@ -907,6 +911,35 @@ int blkid_probe_set_device(blkid_probe pr, int fd,
>   	if (fd < 0)
>   		return 1;
>   
> +#ifdef FDGETFDCSTAT
> +	{
> +		/*
> +		 * Re-open without O_NONBLOCK for floppy device.
> +		 *
> +		 * Since kernel commit c7e9d0020361f4308a70cdfd6d5335e273eb8717
> +		 * floppy drive works bad when opened with O_NONBLOCK.
> +		 */
> +		struct floppy_fdc_state  flst;

hmm, extra whitespace?

> +
> +		if (ioctl(fd, FDGETFDCSTAT, &flst) >= 0) {
> +			int flags = fcntl(fd, F_GETFL, 0);
> +
> +			if (flags < 0)
> +				goto err;
> +			if (flags & O_NONBLOCK) {
> +				flags &= ~O_NONBLOCK;
> +
> +				fd = ul_reopen(fd, flags | O_CLOEXEC);
> +				if (fd < 0)
> +					goto err;
> +
> +				pr->flags |= BLKID_FL_PRIVATE_FD;
> +				pr->fd = fd;
> +			}
> +		}
> +	}
> +#endif
> +
>   #if defined(POSIX_FADV_RANDOM) && defined(HAVE_POSIX_FADVISE)
>   	/* Disable read-ahead */
>   	posix_fadvise(fd, 0, 0, POSIX_FADV_RANDOM);
> 


Looks good to me. I hope I'll be able also to test it soon.

-- 
Best regards,
Vladimir

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Karel Zak]

On Mon, Jan 17, 2022 at 06:46:17PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > +{
> > +	ssize_t ssz;
> > +	char buf[PATH_MAX];
> > +	char fdpath[ sizeof(_PATH_PROC_FDDIR) +  sizeof(stringify_value(INT_MAX)) ];
> 
> Extra whitespace after '+'
> 
> Also, I think you should add +1 for '/' (you use it below), and +1 for finishing \0.

sizeof() counts also \0, it means there is space for '/' and '\0'.


Thanks, I'll fix the rest.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Vladimir Sementsov-Ogievskiy]

17.01.2022 18:46, Vladimir Sementsov-Ogievskiy wrote:
> 17.01.2022 14:50, Karel Zak wrote:
>> On Mon, Jan 03, 2022 at 09:43:11AM +0100, Karel Zak wrote:
>>>> Hi! Are you working on this? If not I can try to make a v2.
>>>
>>> I had a vacation in the last 14 days; nothing is done. Go ahead if you
>>> have time for this task. I'm going to work on something else this
>>> week.
>>
>> It seems you're busy with something else ;-), so I have implemented it,
>> but it's not tested yet.
> 
> Oh seems yes ;) Great that you've done it!
> 
>>
>> Please, please, can you review and test it with your environment?
> 
> Ohh. Will try to remember and restore the environment this week :)
> 
>>
>> Thanks!
>>
>> See the patch bellow or topic/blkid-floppy branch on github.
>>
>>   Karel
>>
>>
>>  From 49be1b256ad351cde2e0cf5000c7046e7010cc02 Mon Sep 17 00:00:00 2001
>> From: Karel Zak <kzak@redhat.com>
>> Date: Mon, 17 Jan 2022 12:37:13 +0100
>> Subject: [PATCH] libblkid: reopen floppy without O_NONBLOCK
>>
>> Vladimir Sementsov-Ogievskiy wrote:
>>> The commit "floppy: reintroduce O_NDELAY fix" was removed from kernel,
>>> so we faced the bug described and discussed here:
>>> https://bugzilla.suse.com/show_bug.cgi?id=3D1181018
>>>
>>> Discussion in kernel list on reverting the commit:
>>> https://www.spinics.net/lists/stable/msg493061.html
>>>
>>> In short, I can quote Jiri Kosina's comment:
>>>
>>>    opening floppy device node with O_NONBLOCK is asking for all kinds
>>>    of trouble
>>>
>>> So opening floppy with O_NONBLOCK in blkid leads to failure of blkid,
>>> probable failure of mount and unpleasant error messages in dmesg (see
>>> also patch 02 for details).
>>
>> Based on patch from Vladimir.
>>
>> CC: Jiri Kosina <jkosina@suse.cz>
>> Reported-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Karel Zak <kzak@redhat.com>
>> ---
>>   include/fileutils.h  |  3 +++
>>   lib/fileutils.c      | 17 +++++++++++++++++
>>   libblkid/src/probe.c | 33 +++++++++++++++++++++++++++++++++
>>   3 files changed, 53 insertions(+)
>>
>> diff --git a/include/fileutils.h b/include/fileutils.h
>> index c36ce6353..8722ed59b 100644
>> --- a/include/fileutils.h
>> +++ b/include/fileutils.h
>> @@ -97,4 +97,7 @@ extern void ul_close_all_fds(unsigned int first, unsigned int last);
>>   #define UL_COPY_WRITE_ERROR (-2)
>>   int ul_copy_file(int from, int to);
>> +
>> +extern int ul_reopen(int fd, int flags);
>> +
>>   #endif /* UTIL_LINUX_FILEUTILS */
>> diff --git a/lib/fileutils.c b/lib/fileutils.c
>> index 7a8fce26f..5b15d4916 100644
>> --- a/lib/fileutils.c
>> +++ b/lib/fileutils.c
>> @@ -288,3 +288,20 @@ int ul_copy_file(int from, int to)
>>       return copy_file_simple(from, to);
>>   #endif
>>   }
>> +
>> +int ul_reopen(int fd, int flags)
> 
> "reopen" sounds a bit like we are going to close old fd, but we don't... No better suggestion :(
> 
>> +{
>> +    ssize_t ssz;
>> +    char buf[PATH_MAX];
>> +    char fdpath[ sizeof(_PATH_PROC_FDDIR) +  sizeof(stringify_value(INT_MAX)) ];
> 
> Extra whitespace after '+'
> 
> Also, I think you should add +1 for '/' (you use it below), and +1 for finishing \0.
> 
> Or alternatively we may just use PATH_MAX here too and not care.
> 
>> +
>> +    snprintf(fdpath, sizeof(fdpath), _PATH_PROC_FDDIR "/%d", fd);
>> +
>> +    ssz = readlink(fdpath, buf, sizeof(buf) - 1);
>> +    if (ssz <= 0)
>> +        return -EINVAL;
> 
> alternatively you may return -errno on ssz < 0, and assert that ssz > 0, I hope 0 can't be returned.
> 
>> +
>> +    buf[ssz] = '\0';
>> +
>> +    return open(buf, flags);
>> +}
>> diff --git a/libblkid/src/probe.c b/libblkid/src/probe.c
>> index 6168370e2..d69ff530f 100644
>> --- a/libblkid/src/probe.c
>> +++ b/libblkid/src/probe.c
>> @@ -103,6 +103,9 @@
>>   #ifdef HAVE_ERRNO_H
>>   #include <errno.h>
>>   #endif
>> +#ifdef HAVE_LINUX_FD_H
>> +#include <linux/fd.h>
>> +#endif
>>   #include <inttypes.h>
>>   #include <stdint.h>
>>   #include <stdarg.h>
>> @@ -113,6 +116,7 @@
>>   #include "sysfs.h"
>>   #include "strutils.h"
>>   #include "list.h"
>> +#include "fileutils.h"
>>   /*
>>    * All supported chains
>> @@ -907,6 +911,35 @@ int blkid_probe_set_device(blkid_probe pr, int fd,
>>       if (fd < 0)
>>           return 1;
>> +#ifdef FDGETFDCSTAT
>> +    {
>> +        /*
>> +         * Re-open without O_NONBLOCK for floppy device.
>> +         *
>> +         * Since kernel commit c7e9d0020361f4308a70cdfd6d5335e273eb8717
>> +         * floppy drive works bad when opened with O_NONBLOCK.
>> +         */
>> +        struct floppy_fdc_state  flst;
> 
> hmm, extra whitespace?
> 
>> +
>> +        if (ioctl(fd, FDGETFDCSTAT, &flst) >= 0) {
>> +            int flags = fcntl(fd, F_GETFL, 0);
>> +
>> +            if (flags < 0)
>> +                goto err;
>> +            if (flags & O_NONBLOCK) {
>> +                flags &= ~O_NONBLOCK;
>> +
>> +                fd = ul_reopen(fd, flags | O_CLOEXEC);
>> +                if (fd < 0)
>> +                    goto err;
>> +
>> +                pr->flags |= BLKID_FL_PRIVATE_FD;
>> +                pr->fd = fd;
>> +            }
>> +        }
>> +    }
>> +#endif
>> +
>>   #if defined(POSIX_FADV_RANDOM) && defined(HAVE_POSIX_FADVISE)
>>       /* Disable read-ahead */
>>       posix_fadvise(fd, 0, 0, POSIX_FADV_RANDOM);
>>
> 
> 
> Looks good to me. I hope I'll be able also to test it soon.
> 

OK, seems that works (I tested the branch topic/blkid-floppy):

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Karel Zak]

On Mon, Jan 17, 2022 at 09:10:05PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> OK, seems that works (I tested the branch topic/blkid-floppy):

 Updated and merged into the master branch. Thanks!

  Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Karel Zak]

On Thu, Dec 09, 2021 at 03:12:33PM +0100, Vladimir Sementsov-Ogievskiy wrote:
> Since c7e9d0020361f4308a70cdfd6d5335e273eb8717
> "Revert "floppy: reintroduce O_NDELAY fix"" commit in linux kernel,
> floppy drive works bad when opened with O_NONBLOCK: first read may
> fail. This cause probing fail and leave error messages in dmesg. So, if
> we detect that openedfd is floppy, reopen it without O_NONBLOCK flag.

 I'd like to a little bit optimize the code in libblkid and reduce
 number of situations when we call FDGETFDCSTAT ioctl.

 If I good understand the problem is read() with O_NONBLOCK, right?

 I'd like to call stat() before FDGETFDCSTAT ioctl to make sure that
 the file descriptor is really block-device etc. (libmount supports
 regular files too)

   Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Vladimir Sementsov-Ogievskiy]

21.01.2022 13:18, Karel Zak wrote:
> On Thu, Dec 09, 2021 at 03:12:33PM +0100, Vladimir Sementsov-Ogievskiy wrote:
>> Since c7e9d0020361f4308a70cdfd6d5335e273eb8717
>> "Revert "floppy: reintroduce O_NDELAY fix"" commit in linux kernel,
>> floppy drive works bad when opened with O_NONBLOCK: first read may
>> fail. This cause probing fail and leave error messages in dmesg. So, if
>> we detect that openedfd is floppy, reopen it without O_NONBLOCK flag.
> 
>   I'd like to a little bit optimize the code in libblkid and reduce
>   number of situations when we call FDGETFDCSTAT ioctl.
> 
>   If I good understand the problem is read() with O_NONBLOCK, right?

My understanding goes no further :)

> 
>   I'd like to call stat() before FDGETFDCSTAT ioctl to make sure that
>   the file descriptor is really block-device etc. (libmount supports
>   regular files too)
> 

Sounds good. I see, fstat() is called in blkid_probe_set_device(), so you just need to move your code below fstat() call.

-- 
Best regards,
Vladimir

Re: [PATCH 2/2] libblkid: reopen floppy without O_NONBLOCK

[Karel Zak]

On Fri, Jan 21, 2022 at 03:13:00PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 21.01.2022 13:18, Karel Zak wrote:
> >   I'd like to call stat() before FDGETFDCSTAT ioctl to make sure that
> >   the file descriptor is really block-device etc. (libmount supports
> >   regular files too)
> > 
> 
> Sounds good. I see, fstat() is called in blkid_probe_set_device(),
> so you just need to move your code below fstat() call.

That's the plan. Updated in git tree.

  Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com