In some cases floppy disks are being indexed, even though no actual
device exists. In our case this was caused by the CMOS-RAM having a few
peculiar bits. This caused a non-existent floppy disk of the type 13 to
be registered as an possibly mountable device, even though it could not
be mounted by any user.
We believe this to be an instance of this bug, as we had similar logs
and issues:
https://bugzilla.kernel.org/show_bug.cgi?id=13486
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/384579
This patch adds the option FLOPPY_ALLOW_UNKNOWN_TYPES to prevent the
additional check that fixed the issue on our reference system, and
increases the startup time of affected systems by over a minute.
Co-developed-by: Philip K. <[email protected]>
Signed-off-by: Philip K. <[email protected]>
Signed-off-by: Moritz Müller <[email protected]>
---
drivers/block/Kconfig | 10 ++++++++++
drivers/block/floppy.c | 6 ++++++
2 files changed, 16 insertions(+)
diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 1bb8ec575352..9e6b32c50b67 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -72,6 +72,16 @@ config AMIGA_Z2RAM
To compile this driver as a module, choose M here: the
module will be called z2ram.
+config FLOPPY_ALLOW_UNKNOWN_TYPES
+ bool "Allow floppy disks of unknown type to be registered."
+ default n
+ help
+ Select this option if you want the Kernel to register floppy
+ disks of an unknown type.
+
+ This should usually not be enabled, because of cases where the
+ system falsely recognizes a non-existent floppy disk as mountable.
+
config CDROM
tristate
select BLK_SCSI_REQUEST
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 485865fd0412..9439444d46d0 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3949,7 +3949,9 @@ static void __init config_types(void)
} else
allowed_drive_mask &= ~(1 << drive);
} else {
+#ifdef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
params = &default_drive_params[0].params;
+#endif
snprintf(temparea, sizeof(temparea),
"unknown type %d (usb?)", type);
name = temparea;
@@ -4518,6 +4520,10 @@ static bool floppy_available(int drive)
return false;
if (fdc_state[FDC(drive)].version == FDC_NONE)
return false;
+#ifndef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
+ if (type >= ARRAY_SIZE(default_drive_params))
+ return false;
+#endif
return true;
}
--
2.20.1
Hi,
On 08.12.2019 22:45, Moritz Müller wrote:
> In some cases floppy disks are being indexed, even though no actual
> device exists. In our case this was caused by the CMOS-RAM having a few
> peculiar bits. This caused a non-existent floppy disk of the type 13 to
> be registered as an possibly mountable device, even though it could not
> be mounted by any user.
>
> We believe this to be an instance of this bug, as we had similar logs
> and issues:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=13486
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/384579
>
> This patch adds the option FLOPPY_ALLOW_UNKNOWN_TYPES to prevent the
> additional check that fixed the issue on our reference system, and
> increases the startup time of affected systems by over a minute.
Does driver blacklisting solves your problem? Or you have real floppy drives in
your system along with these "spurious" ones?
>
> Co-developed-by: Philip K. <[email protected]>
> Signed-off-by: Philip K. <[email protected]>
> Signed-off-by: Moritz Müller <[email protected]>
> ---
> drivers/block/Kconfig | 10 ++++++++++
> drivers/block/floppy.c | 6 ++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index 1bb8ec575352..9e6b32c50b67 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -72,6 +72,16 @@ config AMIGA_Z2RAM
> To compile this driver as a module, choose M here: the
> module will be called z2ram.
>
> +config FLOPPY_ALLOW_UNKNOWN_TYPES
> + bool "Allow floppy disks of unknown type to be registered."
> + default n
> + help
> + Select this option if you want the Kernel to register floppy
> + disks of an unknown type.
> +
> + This should usually not be enabled, because of cases where the
> + system falsely recognizes a non-existent floppy disk as mountable.
> +
> config CDROM
> tristate
> select BLK_SCSI_REQUEST
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 485865fd0412..9439444d46d0 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -3949,7 +3949,9 @@ static void __init config_types(void)
> } else
> allowed_drive_mask &= ~(1 << drive);
> } else {
> +#ifdef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
> params = &default_drive_params[0].params;
> +#endif
You can't just skip it with ifdef. This will result in uninitialized
pointer dereference down the code.
struct floppy_drive_params *params;
...
if (type < ARRAY_SIZE(default_drive_params)) {
...
} else {
#ifdef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
params = &default_drive_params[0].params;
#endif
...
}
...
*UDP = *params; // << HERE
> snprintf(temparea, sizeof(temparea),
> "unknown type %d (usb?)", type);
> name = temparea;
> @@ -4518,6 +4520,10 @@ static bool floppy_available(int drive)
> return false;
> if (fdc_state[FDC(drive)].version == FDC_NONE)
> return false;
> +#ifndef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
> + if (type >= ARRAY_SIZE(default_drive_params))
> + return false;
> +#endif
> return true;
> }
>
>
Thanks,
Denis
Hi "Moritz,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on linux/master linus/master v5.4 next-20191208]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Moritz-M-ller/floppy-hide-invalid-floppy-disk-types/20191209-035056
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: x86_64-rhel (attached as .config)
compiler: gcc-7 (Debian 7.5.0-1) 7.5.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers//block/floppy.c: In function 'floppy_available':
>> drivers//block/floppy.c:4524:6: error: 'type' undeclared (first use in this function); did you mean 'true'?
if (type >= ARRAY_SIZE(default_drive_params))
^~~~
true
drivers//block/floppy.c:4524:6: note: each undeclared identifier is reported only once for each function it appears in
vim +4524 drivers//block/floppy.c
4516
4517 static bool floppy_available(int drive)
4518 {
4519 if (!(allowed_drive_mask & (1 << drive)))
4520 return false;
4521 if (fdc_state[FDC(drive)].version == FDC_NONE)
4522 return false;
4523 #ifndef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
> 4524 if (type >= ARRAY_SIZE(default_drive_params))
4525 return false;
4526 #endif
4527 return true;
4528 }
4529
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
In some cases floppy disks are being indexed, even though no actual
device exists. In our case this was caused by the CMOS-RAM having a few
peculiar bits. This caused a non-existent floppy disk of the type 13 (in
our case) to be registered as an possibly mountable device, even though
it could not be mounted by any user.
We believe this to be an instance of this bug:
https://bugzilla.kernel.org/show_bug.cgi?id=13486
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/384579
This patch adds the option FLOPPY_ALLOW_UNKNOWN_TYPES to prevent the
additional check that fixed the issue on our reference system, and
increases the startup time of affected systems by over a minute.
Co-developed-by: Philip K. <[email protected]>
Signed-off-by: Philip K. <[email protected]>
Signed-off-by: Moritz Müller <[email protected]>
---
drivers/block/Kconfig | 10 ++++++++++
drivers/block/floppy.c | 6 ++++++
2 files changed, 16 insertions(+)
diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 1bb8ec575352..9e6b32c50b67 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -72,6 +72,16 @@ config AMIGA_Z2RAM
To compile this driver as a module, choose M here: the
module will be called z2ram.
+config FLOPPY_ALLOW_UNKNOWN_TYPES
+ bool "Allow floppy disks of unknown type to be registered."
+ default n
+ help
+ Select this option if you want the Kernel to register floppy
+ disks of an unknown type.
+
+ This should usually not be enabled, because of cases where the
+ system falsely recognizes a non-existent floppy disk as mountable.
+
config CDROM
tristate
select BLK_SCSI_REQUEST
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 485865fd0412..9439444d46d0 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3949,7 +3949,11 @@ static void __init config_types(void)
} else
allowed_drive_mask &= ~(1 << drive);
} else {
+#ifdef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
params = &default_drive_params[0].params;
+#else
+ params = UDP;
+#endif
snprintf(temparea, sizeof(temparea),
"unknown type %d (usb?)", type);
name = temparea;
@@ -4518,7 +4520,10 @@ static bool floppy_available(int drive)
return false;
if (fdc_state[FDC(drive)].version == FDC_NONE)
return false;
+#ifndef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
+ if (UDP->cmos >= ARRAY_SIZE(default_drive_params))
+ return false;
+#endif
return true;
}
--
2.20.1
Hi,
On 12/9/19 12:32 PM, Moritz Müller wrote:
> In some cases floppy disks are being indexed, even though no actual
> device exists. In our case this was caused by the CMOS-RAM having a few
> peculiar bits. This caused a non-existent floppy disk of the type 13 (in
> our case) to be registered as an possibly mountable device, even though
> it could not be mounted by any user.
>
> We believe this to be an instance of this bug:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=13486
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/384579
>
> This patch adds the option FLOPPY_ALLOW_UNKNOWN_TYPES to prevent the
> additional check that fixed the issue on our reference system, and
> increases the startup time of affected systems by over a minute.
>
> Co-developed-by: Philip K. <[email protected]>
> Signed-off-by: Philip K. <[email protected]>
> Signed-off-by: Moritz Müller <[email protected]>
Thank you for the patch!
Have you tested your patch with FLOPPY_ALLOW_UNKNOWN_TYPES and without
FLOPPY_ALLOW_UNKNOWN_TYPES?
I will answer about motivation for this change in V2 branch of the patch.
> ---
> drivers/block/Kconfig | 10 ++++++++++
> drivers/block/floppy.c | 6 ++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index 1bb8ec575352..9e6b32c50b67 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -72,6 +72,16 @@ config AMIGA_Z2RAM
> To compile this driver as a module, choose M here: the
> module will be called z2ram.
>
> +config FLOPPY_ALLOW_UNKNOWN_TYPES
> + bool "Allow floppy disks of unknown type to be registered."
> + default n
> + help
> + Select this option if you want the Kernel to register floppy
> + disks of an unknown type.
> +
> + This should usually not be enabled, because of cases where the
> + system falsely recognizes a non-existent floppy disk as mountable.
> +
> config CDROM
> tristate
> select BLK_SCSI_REQUEST
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index 485865fd0412..9439444d46d0 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -3949,7 +3949,11 @@ static void __init config_types(void)
> } else
> allowed_drive_mask &= ~(1 << drive);
> } else {
> +#ifdef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
> params = &default_drive_params[0].params;
> +#else
> + params = UDP;
> +#endif
> snprintf(temparea, sizeof(temparea),
> "unknown type %d (usb?)", type);
> name = temparea;
Maybe just skip the else branch completely here? This will omit
snprintf, following if (name) block and UDP update.
+#ifdef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
params = &default_drive_params[0].params;
snprintf(temparea, sizeof(temparea),
"unknown type %d (usb?)", type);
name = temparea;
+#else
+ continue;
+#endif
> @@ -4518,7 +4520,10 @@ static bool floppy_available(int drive)
> return false;
> if (fdc_state[FDC(drive)].version == FDC_NONE)
> return false;
> +#ifndef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
> + if (UDP->cmos >= ARRAY_SIZE(default_drive_params))
> + return false;
> +#endif
> return true;
> }
>
>
Thanks,
Denis
On 12/9/19 12:04 AM, Philip K. wrote:
> Denis Efremov <[email protected]> writes:
>
>> Hi,
>>
>> On 08.12.2019 22:45, Moritz Müller wrote:
>>> In some cases floppy disks are being indexed, even though no actual
>>> device exists. In our case this was caused by the CMOS-RAM having a few
>>> peculiar bits. This caused a non-existent floppy disk of the type 13 to
>>> be registered as an possibly mountable device, even though it could not
>>> be mounted by any user.
>>>
>>> We believe this to be an instance of this bug, as we had similar logs
>>> and issues:
>>>
>>> https://bugzilla.kernel.org/show_bug.cgi?id=13486
>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/384579
Well, this is a ten years old bug. It seems like that time it was decided
to fix the problem either by blacklisting the driver or by disabling the
BIOS option. I doubt that many people are facing this problem since then.
I think that more-or-less modern motherboards don't have this issue.
>>>
>>> This patch adds the option FLOPPY_ALLOW_UNKNOWN_TYPES to prevent the
>>> additional check that fixed the issue on our reference system, and
>>> increases the startup time of affected systems by over a minute.
>>
>> Does driver blacklisting solves your problem? Or you have real floppy drives in
>> your system along with these "spurious" ones?
>
> No there were not, and blacklisting would solve the bug too. We just
> thought that fixing the bug this way would prevent the issue from
> appearing in the first place on systems that have the floppy module
> enabled, in the first place.
Hmm, I would say that driver blacklisting is a more proper solution in
this case. I doubt there are people with this issue and real floppy drives
in their setup. Altering the default driver's initialization scheme seems
superfluous to me. This will force users (if there are ones) who depends on this
behavior to rebuild the kernel. blacklisting doesn't require kernel rebuild.
>
>>> Co-developed-by: Philip K. <[email protected]>
>>> Signed-off-by: Philip K. <[email protected]>
>>> Signed-off-by: Moritz Müller <[email protected]>
>>> ---
>>> drivers/block/Kconfig | 10 ++++++++++
>>> drivers/block/floppy.c | 6 ++++++
>>> 2 files changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
>>> index 1bb8ec575352..9e6b32c50b67 100644
>>> --- a/drivers/block/Kconfig
>>> +++ b/drivers/block/Kconfig
>>> @@ -72,6 +72,16 @@ config AMIGA_Z2RAM
>>> To compile this driver as a module, choose M here: the
>>> module will be called z2ram.
>>>
>>> +config FLOPPY_ALLOW_UNKNOWN_TYPES
>>> + bool "Allow floppy disks of unknown type to be registered."
>>> + default n
>>> + help
>>> + Select this option if you want the Kernel to register floppy
>>> + disks of an unknown type.
>>> +
>>> + This should usually not be enabled, because of cases where the
>>> + system falsely recognizes a non-existent floppy disk as mountable.
>>> +
>>> config CDROM
>>> tristate
>>> select BLK_SCSI_REQUEST
>>> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
>>> index 485865fd0412..9439444d46d0 100644
>>> --- a/drivers/block/floppy.c
>>> +++ b/drivers/block/floppy.c
>>> @@ -3949,7 +3949,9 @@ static void __init config_types(void)
>>> } else
>>> allowed_drive_mask &= ~(1 << drive);
>>> } else {
>>> +#ifdef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
>>> params = &default_drive_params[0].params;
>>> +#endif
>>
>> You can't just skip it with ifdef. This will result in uninitialized
>> pointer dereference down the code.
>>
>> struct floppy_drive_params *params;
>> ...
>>
>> if (type < ARRAY_SIZE(default_drive_params)) {
>> ...
>> } else {
>> #ifdef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
>> params = &default_drive_params[0].params;
>> #endif
>> ...
>> }
>> ...
>> *UDP = *params; // << HERE
>
> Oops, you're right, will fix.
>
>>> snprintf(temparea, sizeof(temparea),
>>> "unknown type %d (usb?)", type);
>>> name = temparea;
>>> @@ -4518,6 +4520,10 @@ static bool floppy_available(int drive)
>>> return false;
>>> if (fdc_state[FDC(drive)].version == FDC_NONE)
>>> return false;
>>> +#ifndef CONFIG_FLOPPY_ALLOW_UNKNOWN_TYPES
>>> + if (type >= ARRAY_SIZE(default_drive_params))
>>> + return false;
>>> +#endif
>>> return true;
>>> }
>>>
>>>
>>
>> Thanks,
>> Denis
>
> Hmm, I would say that driver blacklisting is a more proper solution in
> this case. I doubt there are people with this issue and real floppy drives
> in their setup. Altering the default driver's initialization scheme seems
> superfluous to me.
As long as major distributions like Ubuntu ship the floppy module, there
are enough people who could be affected by this peculiar behaviour.
While I agree that blacklisting the module would be more elegant, I
still think that a patch that goes in this direction could help more
people, especially those who don't want or cannot solve kernel-related
issues.
> This will force users (if there are ones) who depends on this behavior
> to rebuild the kernel. blacklisting doesn't require kernel rebuild.
Are there floppy disks of unknown types? Our patch is intentionally
conservative: We won't hide false negatives. If the motherboard reports
an non-existent disk
If you're ready to think about it, we could consider extending the patch
to un-register the device if it can recognise that it's (probably) not
real. In our case, for example, fdisk reported that fd0 had a size of
4k, something think is a strong indicator that something's not right.
Alternatively, we could look into what the comment
/* FIXME: additional physical CMOS drive detection should go here */
would imply. This particular bug can only affect fd0 and fd1, so if we
spent some more time, we could find something.
--
With kind regards,
Philip K.