On Mon 24-09-18 19:29:10, Tetsuo Handa wrote: > From 2278250ac8c5b912f7eb7af55e36ed40e2f7116b Mon Sep 17 00:00:00 2001 > From: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> > Date: Mon, 24 Sep 2018 18:58:37 +0900 > Subject: [PATCH v4] block/loop: Serialize ioctl operations. > > syzbot is reporting NULL pointer dereference [1] which is caused by > race condition between ioctl(loop_fd, LOOP_CLR_FD, 0) versus > ioctl(other_loop_fd, LOOP_SET_FD, loop_fd) due to traversing other > loop devices without holding corresponding locks. > > syzbot is also reporting circular locking dependency between bdev->bd_mutex > and lo->lo_ctl_mutex [2] which is caused by calling blkdev_reread_part() > with lock held. Please explain in the changelog that it is indeed loop_validate_file() and only that AFAICT that is doing these racy unlocked accesses. Otherwise it's pretty hard to find that. > Since ioctl() request on loop devices is not frequent operation, we don't > need fine grained locking. Let's use global lock and simplify the locking > order. > > Strategy is that the global lock is held upon entry of ioctl() request, > and release it before either starting operations which might deadlock or > leaving ioctl() request. After the global lock is released, current thread > no longer uses "struct loop_device" memory because it might be modified > by next ioctl() request which was waiting for current ioctl() request. > > In order to enforce this strategy, this patch inverted > loop_reread_partitions() and loop_unprepare_queue() in loop_clr_fd(). > According to Ming Lei, calling loop_unprepare_queue() before > loop_reread_partitions() (like we did until 3.19) is fine. > > Since this patch serializes using global lock, race bugs should no longer > exist. Thus, it will be easy to test whether this patch broke something. > Since syzbot is reporting various bugs [3] where a race in the loop module > is suspected, let's check whether this patch affects these bugs too. > > [1] https://syzkaller.appspot.com/bug?id=f3cfe26e785d85f9ee259f385515291d21bd80a3 > [2] https://syzkaller.appspot.com/bug?id=bf154052f0eea4bc7712499e4569505907d15889 > [3] https://syzkaller.appspot.com/bug?id=b3c7e1440aa8ece16bf557dbac427fdff1dad9d6 > > Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> > Reported-by: syzbot <syzbot+bf89c128e05dd6c62523@xxxxxxxxxxxxxxxxxxxxxxxxx> > Reported-by: syzbot <syzbot+4684a000d5abdade83fac55b1e7d1f935ef1936e@xxxxxxxxxxxxxxxxxxxxxxxxx> > Cc: Ming Lei <ming.lei@xxxxxxxxxx> > Cc: Jan Kara <jack@xxxxxxx> > Cc: Jens Axboe <axboe@xxxxxxxxx> Looking into the patch, this is convoluting several things together and as a result the patch is very difficult to review. Can you please split it up so that it's reviewable? I can see there: 1) Change to not call blkdev_reread_part() with loop mutex held - that deserves a separate patch. 2) Switch to global loop_mutex - another patch. 3) Some unrelated adjustments - like using path instead of file in loop_get_status(), doing int ret = foo(); instead of int ret; ret = foo(); etc. One patch for each such change please. Some more comments inline. > diff --git a/drivers/block/loop.c b/drivers/block/loop.c > index ea9debf..a7058ec 100644 > --- a/drivers/block/loop.c > +++ b/drivers/block/loop.c > @@ -83,11 +83,50 @@ > #include <linux/uaccess.h> > > static DEFINE_IDR(loop_index_idr); > -static DEFINE_MUTEX(loop_index_mutex); > +static DEFINE_MUTEX(loop_mutex); > +static void *loop_mutex_owner; /* == __mutex_owner(&loop_mutex) */ > > static int max_part; > static int part_shift; > > +/* > + * lock_loop - Lock loop_mutex. > + */ > +static void lock_loop(void) > +{ > + mutex_lock(&loop_mutex); > + loop_mutex_owner = current; > +} > + > +/* > + * lock_loop_killable - Lock loop_mutex unless killed. > + */ > +static int lock_loop_killable(void) > +{ > + int err = mutex_lock_killable(&loop_mutex); > + > + if (err) > + return err; > + loop_mutex_owner = current; > + return 0; > +} > + > +/* > + * unlock_loop - Unlock loop_mutex as needed. > + * > + * Explicitly call this function before calling fput() or blkdev_reread_part() > + * in order to avoid circular lock dependency. After this function is called, > + * current thread is no longer allowed to access "struct loop_device" memory, > + * for another thread would access that memory as soon as loop_mutex is held. > + */ > +static void unlock_loop(void) > +{ > + if (loop_mutex_owner == current) { Urgh, why this check? Conditional locking / unlocking is evil so it has to have *very* good reasons and there is not any explanation here. So far I don't see a reason why this is needed at all. > + loop_mutex_owner = NULL; > + mutex_unlock(&loop_mutex); > + } > +} > + > static int transfer_xor(struct loop_device *lo, int cmd, > struct page *raw_page, unsigned raw_off, > struct page *loop_page, unsigned loop_off, > @@ -630,7 +669,12 @@ static void loop_reread_partitions(struct loop_device *lo, > struct block_device *bdev) > { > int rc; > + /* Save variables which might change after unlock_loop() is called. */ > + char filename[sizeof(lo->lo_file_name)]; ^^^^ LO_NAME_SIZE would do. I don't think it's any less maintainable and certainly easier to read. > + const int num = lo->lo_number; > > + memmove(filename, lo->lo_file_name, sizeof(filename)); Why not memcpy? Buffers certainly don't overlap. > + unlock_loop(); Unlocking in loop_reread_partitions() makes the locking rules ugly. And unnecessarily AFAICT. Can't we just use lo_refcnt to protect us against loop_clr_fd() and freeing of 'lo' structure itself? Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
