| Message ID | 1d9351dc-baeb-1a54-625c-04ce01b009b0@i-love.sakura.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ataflop: unlock ataflop_probe_lock at atari_floppy_init() | expand |
On Sat, 16 Oct 2021, Tetsuo Handa wrote: > Commit bf9c0538e485b591 ("ataflop: use a separate gendisk for each media > format") introduced ataflop_probe_lock mutex, but forgot to unlock the > mutex when atari_floppy_init() (i.e. module loading) succeeded. If > ataflop_probe() is called, it will deadlock on ataflop_probe_lock mutex. > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format") > --- > To m68k users > > This patch suggests that nobody is testing this module using a real hardware. > Can somebody test this module? > Is current m68k hardware still supporting Atari floppy? > If Atari floppy is no longer supported, do we still need this module? > It is only to be expected that no-one would have reported this bug yet. 2 months ago, Debian 11 shipped with a 5.10 kernel, but the bug you found first appeared in Linux 5.11. The existence of buggy drivers in mainline is undesirable but the real problem here is the rate at which new bugs get added. So I wonder if it would have been possible to use Aranym to find the regression, or avoid it in the first place?
Hi Tetsuo, thank you for fixing this bug! On 17/10/21 02:25, Tetsuo Handa wrote: > Commit bf9c0538e485b591 ("ataflop: use a separate gendisk for each media > format") introduced ataflop_probe_lock mutex, but forgot to unlock the > mutex when atari_floppy_init() (i.e. module loading) succeeded. If > ataflop_probe() is called, it will deadlock on ataflop_probe_lock mutex. > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format") > --- > To m68k users > > This patch suggests that nobody is testing this module using a real hardware. Not as a module, no. I use the Atari floppy driver built-in. Latest kernel version I ran was 5.13. Relevant kernel log excerpt: calling atari_floppy_init+0x0/0x4d4 @ 1 Atari floppy driver: max. HD, track buffering Probing floppy drive(s): fd0 initcall atari_floppy_init+0x0/0x4d4 returned 0 after 675082 usecs Haven't tried to read from the drive in a while though... waiting for floppy I/O isn't my favourite spectator sport. I take it a read attempt should fail, without your patch? > Can somebody test this module? Yes. > Is current m68k hardware still supporting Atari floppy? Yes. Cheers, Michael Schmitz > If Atari floppy is no longer supported, do we still need this module? > > To Christoph Hellwig and Luis Chamberlain > > If we move __register_blkdev() in atari_floppy_init() to the end of > atari_floppy_init() and move unregister_blkdev() in atari_floppy_exit() to > the beginning of atari_floppy_exit(), we can remove unregister_blkdev() from > atari_floppy_init(), and I think we can also remove ataflop_probe_lock mutex > because probe function and __exit function are serialized by major_names_lock > mutex. > > drivers/block/ataflop.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c > index a093644ac39f..39b42cb8d173 100644 > --- a/drivers/block/ataflop.c > +++ b/drivers/block/ataflop.c > @@ -2072,7 +2072,8 @@ static int __init atari_floppy_init (void) > UseTrackbuffer ? "" : "no "); > config_types(); > > - return 0; > + ret = 0; > + goto out_unlock; > > err: > while (--i >= 0) { >
diff --git a/drivers/block/ataflop.c b/drivers/block/ataflop.c index a093644ac39f..39b42cb8d173 100644 --- a/drivers/block/ataflop.c +++ b/drivers/block/ataflop.c @@ -2072,7 +2072,8 @@ static int __init atari_floppy_init (void) UseTrackbuffer ? "" : "no "); config_types(); - return 0; + ret = 0; + goto out_unlock; err: while (--i >= 0) {
Commit bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format") introduced ataflop_probe_lock mutex, but forgot to unlock the mutex when atari_floppy_init() (i.e. module loading) succeeded. If ataflop_probe() is called, it will deadlock on ataflop_probe_lock mutex. Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Fixes: bf9c0538e485b591 ("ataflop: use a separate gendisk for each media format") --- To m68k users This patch suggests that nobody is testing this module using a real hardware. Can somebody test this module? Is current m68k hardware still supporting Atari floppy? If Atari floppy is no longer supported, do we still need this module? To Christoph Hellwig and Luis Chamberlain If we move __register_blkdev() in atari_floppy_init() to the end of atari_floppy_init() and move unregister_blkdev() in atari_floppy_exit() to the beginning of atari_floppy_exit(), we can remove unregister_blkdev() from atari_floppy_init(), and I think we can also remove ataflop_probe_lock mutex because probe function and __exit function are serialized by major_names_lock mutex. drivers/block/ataflop.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)