linux-kernel - Re: [PATCH v4] block/loop: Serialize ioctl operations.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 25 Sep 2018 10:06:22 +0200
From:   Jan Kara <jack@...e.cz>
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Jan Kara <jack@...e.cz>, Ming Lei <ming.lei@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
        Jens Axboe <axboe@...nel.dk>,
        syzbot 
        <syzbot+4684a000d5abdade83fac55b1e7d1f935ef1936e@...kaller.appspotmail.com>,
        syzbot <syzbot+bf89c128e05dd6c62523@...kaller.appspotmail.com>
Subject: Re: [PATCH v4] block/loop: Serialize ioctl operations.

On Tue 25-09-18 06:06:56, Tetsuo Handa wrote:
> On 2018/09/25 3:47, Jan Kara wrote:
> >> +/*
> >> + * 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.
> 
> Yeah, this is why Jens hates this patch. But any alternative?

So can you explain why this conditional locking is really necessary?

> >> @@ -630,7 +669,12 @@ static void loop_reread_partitions(struct loop_device *lo,
> >> +	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?
> 
> Really? I think that just elevating lo->lo_refcnt will cause another lockdep
> warning because __blkdev_reread_part() requires bdev->bd_mutex being held.
> Don't we need to drop the lock in order to solve original lockdep warning at [2] ?

Yes, you have to drop the lo_ctl_mutex before calling
loop_reread_partitions(). But AFAICS all places calling loop_reread_part()
are called from ioctl where we are sure the device is open and thus
lo_refcnt is > 0. So in these places calling loop_reread_partitions()
without lo_ctl_mutex should be fine. The only exception is lo_clr_fd() that
can get called from __lo_release() - and I think we can protect that case
against LOOP_CTL_REMOVE (it cannot really race with anything else) by
keeping lo_state at Lo_rundown until after loop_reread_partitions() has
finished.

								Honza

-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR