From: Jiri Kosina <jikos@kernel.org>
To: Dmitry Vyukov <dvyukov@google.com>, Jens Axboe <axboe@fb.com>
Cc: NeilBrown <neilb@suse.com>, Takashi Iwai <tiwai@suse.de>,
Hannes Reinecke <hare@suse.de>,
Rasmus Villemoes <linux@rasmusvillemoes.dk>,
LKML <linux-kernel@vger.kernel.org>,
syzkaller <syzkaller@googlegroups.com>,
Kostya Serebryany <kcc@google.com>,
Alexander Potapenko <glider@google.com>,
Sasha Levin <sasha.levin@oracle.com>
Subject: [PATCH] floppy: fix lock_fdc() signal handling
Date: Thu, 28 Jan 2016 11:43:36 +0100 (CET) [thread overview]
Message-ID: <alpine.LNX.2.00.1601281133400.21446@cbobk.fhfr.pm> (raw)
In-Reply-To: <CACT4Y+boTwHo-sLJc8apwyF6=7FTCsDRQ9nwR1Vf7HpzGGj-BA@mail.gmail.com>
On Thu, 28 Jan 2016, Dmitry Vyukov wrote:
> I have not seen any /dev/fd0 related crashes over night. Previously I
> seen tons of them, so I guess this bug is fixed.
> Please send this upstream.
> Thanks
Thanks a lot for testing, Dmitry.
Jens, here is the patch with proper changelog. Could you please apply it
to your tree? Or would you prefer a pull request with this single patch in
it?
Thanks.
From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] floppy: fix lock_fdc() signal handling
floppy_revalidate() doesn't perform any error handling on lock_fdc()
result. lock_fdc() might actually be interrupted by a signal (it waits for
fdc becoming non-busy interruptibly). In such case, floppy_revalidate()
proceeds as if it had claimed the lock, but it fact it doesn't.
In case of multiple threads trying to open("/dev/fdX"), this leads to
serious corruptions all over the place, because all of a sudden there is
no critical section protection (that'd otherwise be guaranteed by locked
fd) whatsoever.
While at this, fix the fact that the 'interruptible' parameter to
lock_fdc() doesn't make any sense whatsoever, because we always wait
interruptibly anyway.
Most of the lock_fdc() callsites do properly handle error (and propagate
EINTR), but floppy_revalidate() and floppy_check_events() don't. Fix this.
Spotted by 'syzkaller' tool.
Reported-by: Dmitry Vyukov <dvyukov@google.com>
Tested-by: Dmitry Vyukov <dvyukov@google.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
drivers/block/floppy.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index 9e25120..b206115 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -866,7 +866,7 @@ static void set_fdc(int drive)
}
/* locks the driver */
-static int lock_fdc(int drive, bool interruptible)
+static int lock_fdc(int drive)
{
if (WARN(atomic_read(&usage_count) == 0,
"Trying to lock fdc while usage count=0\n"))
@@ -2173,7 +2173,7 @@ static int do_format(int drive, struct format_descr *tmp_format_req)
{
int ret;
- if (lock_fdc(drive, true))
+ if (lock_fdc(drive))
return -EINTR;
set_floppy(drive);
@@ -2960,7 +2960,7 @@ static int user_reset_fdc(int drive, int arg, bool interruptible)
{
int ret;
- if (lock_fdc(drive, interruptible))
+ if (lock_fdc(drive))
return -EINTR;
if (arg == FD_RESET_ALWAYS)
@@ -3243,7 +3243,7 @@ static int set_geometry(unsigned int cmd, struct floppy_struct *g,
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
mutex_lock(&open_lock);
- if (lock_fdc(drive, true)) {
+ if (lock_fdc(drive)) {
mutex_unlock(&open_lock);
return -EINTR;
}
@@ -3263,7 +3263,7 @@ static int set_geometry(unsigned int cmd, struct floppy_struct *g,
} else {
int oldStretch;
- if (lock_fdc(drive, true))
+ if (lock_fdc(drive))
return -EINTR;
if (cmd != FDDEFPRM) {
/* notice a disk change immediately, else
@@ -3349,7 +3349,7 @@ static int get_floppy_geometry(int drive, int type, struct floppy_struct **g)
if (type)
*g = &floppy_type[type];
else {
- if (lock_fdc(drive, false))
+ if (lock_fdc(drive))
return -EINTR;
if (poll_drive(false, 0) == -EINTR)
return -EINTR;
@@ -3433,7 +3433,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int
if (UDRS->fd_ref != 1)
/* somebody else has this drive open */
return -EBUSY;
- if (lock_fdc(drive, true))
+ if (lock_fdc(drive))
return -EINTR;
/* do the actual eject. Fails on
@@ -3445,7 +3445,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int
process_fd_request();
return ret;
case FDCLRPRM:
- if (lock_fdc(drive, true))
+ if (lock_fdc(drive))
return -EINTR;
current_type[drive] = NULL;
floppy_sizes[drive] = MAX_DISK_SIZE << 1;
@@ -3467,7 +3467,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int
UDP->flags &= ~FTD_MSG;
return 0;
case FDFMTBEG:
- if (lock_fdc(drive, true))
+ if (lock_fdc(drive))
return -EINTR;
if (poll_drive(true, FD_RAW_NEED_DISK) == -EINTR)
return -EINTR;
@@ -3484,7 +3484,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int
return do_format(drive, &inparam.f);
case FDFMTEND:
case FDFLUSH:
- if (lock_fdc(drive, true))
+ if (lock_fdc(drive))
return -EINTR;
return invalidate_drive(bdev);
case FDSETEMSGTRESH:
@@ -3507,7 +3507,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int
outparam = UDP;
break;
case FDPOLLDRVSTAT:
- if (lock_fdc(drive, true))
+ if (lock_fdc(drive))
return -EINTR;
if (poll_drive(true, FD_RAW_NEED_DISK) == -EINTR)
return -EINTR;
@@ -3530,7 +3530,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int
case FDRAWCMD:
if (type)
return -EINVAL;
- if (lock_fdc(drive, true))
+ if (lock_fdc(drive))
return -EINTR;
set_floppy(drive);
i = raw_cmd_ioctl(cmd, (void __user *)param);
@@ -3539,7 +3539,7 @@ static int fd_locked_ioctl(struct block_device *bdev, fmode_t mode, unsigned int
process_fd_request();
return i;
case FDTWADDLE:
- if (lock_fdc(drive, true))
+ if (lock_fdc(drive))
return -EINTR;
twaddle();
process_fd_request();
@@ -3748,7 +3748,8 @@ static unsigned int floppy_check_events(struct gendisk *disk,
return DISK_EVENT_MEDIA_CHANGE;
if (time_after(jiffies, UDRS->last_checked + UDP->checkfreq)) {
- lock_fdc(drive, false);
+ if (lock_fdc(drive))
+ return -EINTR;
poll_drive(false, 0);
process_fd_request();
}
@@ -3847,7 +3848,9 @@ static int floppy_revalidate(struct gendisk *disk)
"VFS: revalidate called on non-open device.\n"))
return -EFAULT;
- lock_fdc(drive, false);
+ res = lock_fdc(drive);
+ if (res)
+ return res;
cf = (test_bit(FD_DISK_CHANGED_BIT, &UDRS->flags) ||
test_bit(FD_VERIFY_BIT, &UDRS->flags));
if (!(cf || test_bit(drive, &fake_change) || drive_no_geom(drive))) {
next prev parent reply other threads:[~2016-01-28 10:43 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-24 13:12 floppy: GPF in floppy_rb0_cb Dmitry Vyukov
2016-01-25 14:06 ` Jiri Kosina
2016-01-25 15:34 ` Dmitry Vyukov
2016-01-27 16:55 ` Jiri Kosina
2016-01-27 17:17 ` Dmitry Vyukov
2016-01-27 20:27 ` Jiri Kosina
2016-01-28 10:32 ` Dmitry Vyukov
2016-01-28 10:43 ` Jiri Kosina [this message]
2016-01-29 9:22 ` [PATCH] floppy: fix lock_fdc() signal handling Dmitry Vyukov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LNX.2.00.1601281133400.21446@cbobk.fhfr.pm \
--to=jikos@kernel.org \
--cc=axboe@fb.com \
--cc=dvyukov@google.com \
--cc=glider@google.com \
--cc=hare@suse.de \
--cc=kcc@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=neilb@suse.com \
--cc=sasha.levin@oracle.com \
--cc=syzkaller@googlegroups.com \
--cc=tiwai@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).