linux-kernel - Re: fs: GPF in locked_inode_to_wb_and_lock

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]

Message-ID: <CACT4Y+bHst2Cq1zk6RMVh78mDM2J2fQN4Q0ULROtb04-EamK7Q@mail.gmail.com>
Date:	Fri, 22 Apr 2016 20:55:27 +0200
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	Andrey Ryabinin <ryabinin.a.a@...il.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Ilya Dryomov <idryomov@...il.com>, Jens Axboe <axboe@...com>,
	Jan Kara <jack@...e.com>
Subject: Re: fs: GPF in locked_inode_to_wb_and_lock_list

On Thu, Apr 21, 2016 at 7:06 PM, Tejun Heo <tj@...nel.org> wrote:
> Hello,
>
> (cc'ing Ilya, Jan and Jens)
>
> On Thu, Apr 21, 2016 at 12:00:38PM +0200, Dmitry Vyukov wrote:
>> On Thu, Apr 21, 2016 at 11:45 AM, Andrey Ryabinin
>> <ryabinin.a.a@...il.com> wrote:
>> > 2016-04-21 11:35 GMT+03:00 Dmitry Vyukov <dvyukov@...gle.com>:
>> >>
>> >> ffffffff818884dd:       48 8b 03                mov    (%rbx),%rax
>> >>
>> >> So whatever load "&wb->bdi->wb" produces is a NULL deref. (is it wb
>> >> that is NULL?)
>> >
>> > Yes it's NULL wb, because there is only one load:
>> >     mov    (%rbx),%rax        =>       rax = wb->bdi
>> >     add    $0x50,%rax         =>       rax = &bdi->wb
>>
>>
>> I bet that wb becomes NULL on the second iteration of the loop. The
>> loop loops in case of a race with another thread, so it would also
>> explain why it is difficult to reproduce.
>>
>> Tejun, does it make any sense to you?
>
> Yeah, that makes sense.  I think the culprit is 43d1c0eb7e11 ("block:
> detach bdev inode from its wb in __blkdev_put()") which allows inode
> to wb association to be broken while other operations including
> writeback are in progress.  I thought it should be okay as the inode
> must be clean at that point but that obviously doesn't mean that there
> can be no writeback operations in flight.
>
> I hope we could eventually move away from the current model where we
> try to swap out an underlying data structure while upper layers may
> still be referring to it in the future but for now we can make sure
> the writeback operation is finished before detaching wb.
>
> Dmitry, I understand that the bug is difficult to reproduce but can
> you please give the following patch a try?


I've merged it into my tree and will restart the fuzzer and leave it
running for the weekend.
Though, yeah, it is difficult to reproduce...


> Thanks.
>
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 20a2c02..209ea33 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1530,12 +1530,7 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part)
>                 kill_bdev(bdev);
>
>                 bdev_write_inode(bdev);
> -               /*
> -                * Detaching bdev inode from its wb in __destroy_inode()
> -                * is too late: the queue which embeds its bdi (along with
> -                * root wb) can be gone as soon as we put_disk() below.
> -                */
> -               inode_detach_wb(bdev->bd_inode);
> +               inode_detach_blkdev_wb(bdev);
>         }
>         if (bdev->bd_contains == bdev) {
>                 if (disk->fops->release)
> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> index d0b5ca5..ec1f530 100644
> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -230,6 +230,25 @@ static inline void inode_detach_wb(struct inode *inode)
>  }
>
>  /**
> + * inode_detach_blkdev_wb - disassociate a bd_inode from its wb
> + * @bdev: block_device of interest
> + *
> + * @bdev is being put for the last time.  Detaching bdev inode in
> + * __destroy_inode() is too late: the queue which embeds its bdi (along
> + * with root wb) can be gone as soon as the containing disk is put.
> + *
> + * This function dissociates @bdev->bd_inode from its wb.  The inode must
> + * be clean and no further operations should be started on it.
> + */
> +static inline void inode_detach_blkdev_wb(struct block_device *bdev)
> +{
> +       if (bdev->bd_inode->i_wb) {
> +               flush_delayed_work(&bdev->bd_inode->i_wb->dwork);
> +               inode_detach_wb(bdev->bd_inode);
> +       }
> +}
> +
> +/**
>   * wbc_attach_fdatawrite_inode - associate wbc and inode for fdatawrite
>   * @wbc: writeback_control of interest
>   * @inode: target inode
> @@ -277,6 +296,10 @@ static inline void inode_detach_wb(struct inode *inode)
>  {
>  }
>
> +static inline void inode_detach_blkdev_wb(struct block_device *bdev)
> +{
> +}
> +
>  static inline void wbc_attach_and_unlock_inode(struct writeback_control *wbc,
>                                                struct inode *inode)
>         __releases(&inode->i_lock)