Use-after-free in page_cache_async_readahead

Use-after-free in page_cache_async_readahead

[Andrey Konovalov]

Hi!

While running KASAN on 4.2 with Trinity I got the following report:

==================================================================
BUG: KASan: use after free in page_cache_async_readahead+0x2cb/0x3f0
at addr ffff880034bf6690
Read of size 8 by task sshd/2571
=============================================================================
BUG kmalloc-16 (Tainted: G        W      ): kasan: bad access detected
-----------------------------------------------------------------------------

Disabling lock debugging due to kernel taint
INFO: Allocated in bdi_init+0x168/0x960 age=554826 cpu=0 pid=6
[<      none      >] __slab_alloc+0x492/0x4b0 mm/slub.c:2405
[<     inlined    >] kmem_cache_alloc_trace+0x13b/0x170
slab_alloc_node mm/slub.c:2473
[<     inlined    >] kmem_cache_alloc_trace+0x13b/0x170 slab_alloc
mm/slub.c:2515
[<      none      >] kmem_cache_alloc_trace+0x13b/0x170 mm/slub.c:2532
[<     inlined    >] bdi_init+0x168/0x960 kzalloc include/linux/slab.h:430
[<     inlined    >] bdi_init+0x168/0x960 cgwb_bdi_init mm/backing-dev.c:749
[<      none      >] bdi_init+0x168/0x960 mm/backing-dev.c:775
[<      none      >] blk_alloc_queue_node+0x147/0x670 block/blk-core.c:654
[<      none      >] blk_init_queue_node+0x1f/0x60 block/blk-core.c:750
[<      none      >] blk_init_queue+0xe/0x10 block/blk-core.c:741
[<      none      >] __scsi_alloc_queue+0x14/0x30 drivers/scsi/scsi_lib.c:2139
[<      none      >] scsi_alloc_queue+0x32/0xa0 drivers/scsi/scsi_lib.c:2151
[<      none      >] scsi_alloc_sdev+0x73b/0xd50 drivers/scsi/scsi_scan.c:266
[<      none      >] scsi_probe_and_add_lun+0x1545/0x2180
drivers/scsi/scsi_scan.c:1079
[<      none      >] __scsi_add_device+0x1eb/0x210 drivers/scsi/scsi_scan.c:1487
[<      none      >] ata_scsi_scan_host+0x13a/0x3d0
drivers/ata/libata-scsi.c:3736
[<      none      >] async_port_probe+0xae/0xe0 drivers/ata/libata-core.c:6096
[<      none      >] async_run_entry_fn+0xfa/0x3d0 kernel/async.c:123
[<      none      >] process_one_work+0x512/0x1220 kernel/workqueue.c:2032
[<      none      >] worker_thread+0xd7/0x1270 kernel/workqueue.c:2164
INFO: Freed in bdi_destroy+0x2d8/0x390 age=6389 cpu=0 pid=9823
[<      none      >] __slab_free+0x159/0x290 mm/slub.c:2590 (discriminator 1)
[<     inlined    >] kfree+0x143/0x150 slab_free mm/slub.c:2739
[<      none      >] kfree+0x143/0x150 mm/slub.c:3418
[<     inlined    >] bdi_destroy+0x2d8/0x390 wb_exit
include/linux/backing-dev.h:476
[<      none      >] bdi_destroy+0x2d8/0x390 mm/backing-dev.c:839
[<      none      >] blk_cleanup_queue+0x211/0x2b0 block/blk-core.c:581
[<      none      >] __scsi_remove_device+0xaf/0x210
drivers/scsi/scsi_sysfs.c:1087
[<      none      >] scsi_remove_device+0x39/0x50 drivers/scsi/scsi_sysfs.c:1113
[<      none      >] sdev_store_delete+0x22/0x30 drivers/scsi/scsi_sysfs.c:680
[<      none      >] dev_attr_store+0x37/0x70 drivers/base/core.c:137
[<      none      >] sysfs_kf_write+0x12c/0x1f0 fs/sysfs/file.c:131
[<      none      >] kernfs_fop_write+0x1f2/0x390 fs/kernfs/file.c:312
[<      none      >] do_loop_readv_writev+0x123/0x1e0 fs/read_write.c:680
[<      none      >] do_readv_writev+0x57b/0x680 fs/read_write.c:810
[<      none      >] vfs_writev+0x67/0xa0 fs/read_write.c:847
[<     inlined    >] SyS_writev+0x109/0x2d0 SYSC_writev fs/read_write.c:880
[<      none      >] SyS_writev+0x109/0x2d0 fs/read_write.c:872
[<      none      >] entry_SYSCALL_64_fastpath+0x12/0x71
arch/x86/entry/entry_64.S:186
INFO: Slab 0xffffea0000d2fd80 objects=12 used=11 fp=0xffff880034bf6690
flags=0x100000000000080
INFO: Object 0xffff880034bf6690 @offset=1680 fp=0x          (null)

Bytes b4 ffff880034bf6680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00  ................
Object ffff880034bf6690: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00  ................
Redzone ffff880034bf66a0: bb bb bb bb bb bb bb bb
    ........
Padding ffff880034bf67d8: 00 00 00 00 00 00 00 00
    ........
CPU: 0 PID: 2571 Comm: sshd Tainted: G    B   W       4.2.0-kasan #19
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 ffff880034bf6000 ffff880032867a18 ffffffff82d58114 0000000000000053
 ffff880035c03b00 ffff880032867a48 ffffffff81422074 ffff880035c03b00
 ffffea0000d2fd80 ffff880034bf6690 0000000000000000 ffff880032867a78
Call Trace:
 [<     inlined    >] dump_stack+0x45/0x57 __dump_stack lib/dump_stack.c:15
 [<ffffffff82d58114>] dump_stack+0x45/0x57 lib/dump_stack.c:50
 [<ffffffff81422074>] print_trailer+0xf4/0x150 mm/slub.c:650
 [<ffffffff81426aa3>] object_err+0x33/0x40 mm/slub.c:657
 [<     inlined    >] kasan_report_error+0x1e1/0x420
print_address_description mm/kasan/report.c:120
 [<ffffffff81429461>] kasan_report_error+0x1e1/0x420 mm/kasan/report.c:194
 [<ffffffff819f0e50>] ? __radix_tree_lookup+0x2c0/0x2c0 lib/radix-tree.c:532
 [<ffffffff811f76b2>] ? call_rcu_sched+0x12/0x20 kernel/rcu/tree.c:3074
 [<ffffffff814296c8>] kasan_report.part.2+0x28/0x30 mm/kasan/report.c:223
 [<     inlined    >] ? page_cache_async_readahead+0x2cb/0x3f0
wb_congested include/linux/backing-dev.h:192
 [<     inlined    >] ? page_cache_async_readahead+0x2cb/0x3f0
inode_congested include/linux/backing-dev.h:528
 [<     inlined    >] ? page_cache_async_readahead+0x2cb/0x3f0
inode_read_congested include/linux/backing-dev.h:535
 [<ffffffff8137933b>] ? page_cache_async_readahead+0x2cb/0x3f0
mm/readahead.c:544
 [<     inlined    >] __asan_report_load8_noabort+0x29/0x30
kasan_report mm/kasan/report.c:244
 [<ffffffff81429789>] __asan_report_load8_noabort+0x29/0x30
mm/kasan/report.c:244
 [<     inlined    >] page_cache_async_readahead+0x2cb/0x3f0
wb_congested include/linux/backing-dev.h:192
 [<     inlined    >] page_cache_async_readahead+0x2cb/0x3f0
inode_congested include/linux/backing-dev.h:528
 [<     inlined    >] page_cache_async_readahead+0x2cb/0x3f0
inode_read_congested include/linux/backing-dev.h:535
 [<ffffffff8137933b>] page_cache_async_readahead+0x2cb/0x3f0 mm/readahead.c:544
 [<     inlined    >] filemap_fault+0x6a6/0xaa0
do_async_mmap_readahead mm/filemap.c:1864
 [<ffffffff8135b256>] filemap_fault+0x6a6/0xaa0 mm/filemap.c:1917
 [<     inlined    >] ? unlock_page+0xfb/0x150 wake_up_page
include/linux/pagemap.h:501
 [<ffffffff81355ddb>] ? unlock_page+0xfb/0x150 mm/filemap.c:774
 [<ffffffff813c520d>] __do_fault+0xdd/0x180 mm/memory.c:2756
 [<ffffffff813c5130>] ? print_bad_pte+0x670/0x670 mm/memory.c:678
 [<     inlined    >] handle_mm_fault+0x1575/0x2910 do_fault mm/memory.c:2945
 [<     inlined    >] handle_mm_fault+0x1575/0x2910 handle_pte_fault
mm/memory.c:3255
 [<     inlined    >] handle_mm_fault+0x1575/0x2910 __handle_mm_fault
mm/memory.c:3379
 [<ffffffff813cea45>] handle_mm_fault+0x1575/0x2910 mm/memory.c:3408
 [<ffffffff81111830>] ? retarget_shared_pending+0x360/0x360
include/linux/signal.h:117
 [<ffffffff813cd4d0>] ? copy_page_range+0x1140/0x1140 mm/memory.c:1024
 [<ffffffff81118db0>] ? sigprocmask+0x140/0x290 kernel/signal.c:2573
 [<ffffffff813d564c>] ? find_vma+0x1c/0x120 mm/mmap.c:2033
 [<ffffffff810ce4e3>] __do_page_fault+0x2c3/0x840 arch/x86/mm/fault.c:1235
 [<ffffffff810f8aa0>] ? SyS_waitid+0x1d0/0x1d0 kernel/exit.c:1577
 [<ffffffff810ceafa>] trace_do_page_fault+0x6a/0x1c0 arch/x86/mm/fault.c:1328
 [<ffffffff810c2364>] do_async_page_fault+0x14/0x70 arch/x86/kernel/kvm.c:264
 [<ffffffff82d70ab8>] async_page_fault+0x28/0x30 arch/x86/entry/entry_64.S:1081
Memory state around the buggy address:
 ffff880034bf6580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff880034bf6600: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff880034bf6680: fc fc fb fb fc fc fc fc fc fc fc fc fc fc fc fc
                         ^
 ffff880034bf6700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff880034bf6780: fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc fc
==================================================================

Use-after-free in page_cache_async_readahead

[Andrey Konovalov]

Hi!

While running KASAN on 4.2 with Trinity I got the following report:

==================================================================
BUG: KASan: use after free in page_cache_async_readahead+0x2cb/0x3f0
at addr ffff880034bf6690
Read of size 8 by task sshd/2571
=============================================================================
BUG kmalloc-16 (Tainted: G        W      ): kasan: bad access detected
-----------------------------------------------------------------------------

Disabling lock debugging due to kernel taint
INFO: Allocated in bdi_init+0x168/0x960 age=554826 cpu=0 pid=6
[<      none      >] __slab_alloc+0x492/0x4b0 mm/slub.c:2405
[<     inlined    >] kmem_cache_alloc_trace+0x13b/0x170
slab_alloc_node mm/slub.c:2473
[<     inlined    >] kmem_cache_alloc_trace+0x13b/0x170 slab_alloc
mm/slub.c:2515
[<      none      >] kmem_cache_alloc_trace+0x13b/0x170 mm/slub.c:2532
[<     inlined    >] bdi_init+0x168/0x960 kzalloc include/linux/slab.h:430
[<     inlined    >] bdi_init+0x168/0x960 cgwb_bdi_init mm/backing-dev.c:749
[<      none      >] bdi_init+0x168/0x960 mm/backing-dev.c:775
[<      none      >] blk_alloc_queue_node+0x147/0x670 block/blk-core.c:654
[<      none      >] blk_init_queue_node+0x1f/0x60 block/blk-core.c:750
[<      none      >] blk_init_queue+0xe/0x10 block/blk-core.c:741
[<      none      >] __scsi_alloc_queue+0x14/0x30 drivers/scsi/scsi_lib.c:2139
[<      none      >] scsi_alloc_queue+0x32/0xa0 drivers/scsi/scsi_lib.c:2151
[<      none      >] scsi_alloc_sdev+0x73b/0xd50 drivers/scsi/scsi_scan.c:266
[<      none      >] scsi_probe_and_add_lun+0x1545/0x2180
drivers/scsi/scsi_scan.c:1079
[<      none      >] __scsi_add_device+0x1eb/0x210 drivers/scsi/scsi_scan.c:1487
[<      none      >] ata_scsi_scan_host+0x13a/0x3d0
drivers/ata/libata-scsi.c:3736
[<      none      >] async_port_probe+0xae/0xe0 drivers/ata/libata-core.c:6096
[<      none      >] async_run_entry_fn+0xfa/0x3d0 kernel/async.c:123
[<      none      >] process_one_work+0x512/0x1220 kernel/workqueue.c:2032
[<      none      >] worker_thread+0xd7/0x1270 kernel/workqueue.c:2164
INFO: Freed in bdi_destroy+0x2d8/0x390 age=6389 cpu=0 pid=9823
[<      none      >] __slab_free+0x159/0x290 mm/slub.c:2590 (discriminator 1)
[<     inlined    >] kfree+0x143/0x150 slab_free mm/slub.c:2739
[<      none      >] kfree+0x143/0x150 mm/slub.c:3418
[<     inlined    >] bdi_destroy+0x2d8/0x390 wb_exit
include/linux/backing-dev.h:476
[<      none      >] bdi_destroy+0x2d8/0x390 mm/backing-dev.c:839
[<      none      >] blk_cleanup_queue+0x211/0x2b0 block/blk-core.c:581
[<      none      >] __scsi_remove_device+0xaf/0x210
drivers/scsi/scsi_sysfs.c:1087
[<      none      >] scsi_remove_device+0x39/0x50 drivers/scsi/scsi_sysfs.c:1113
[<      none      >] sdev_store_delete+0x22/0x30 drivers/scsi/scsi_sysfs.c:680
[<      none      >] dev_attr_store+0x37/0x70 drivers/base/core.c:137
[<      none      >] sysfs_kf_write+0x12c/0x1f0 fs/sysfs/file.c:131
[<      none      >] kernfs_fop_write+0x1f2/0x390 fs/kernfs/file.c:312
[<      none      >] do_loop_readv_writev+0x123/0x1e0 fs/read_write.c:680
[<      none      >] do_readv_writev+0x57b/0x680 fs/read_write.c:810
[<      none      >] vfs_writev+0x67/0xa0 fs/read_write.c:847
[<     inlined    >] SyS_writev+0x109/0x2d0 SYSC_writev fs/read_write.c:880
[<      none      >] SyS_writev+0x109/0x2d0 fs/read_write.c:872
[<      none      >] entry_SYSCALL_64_fastpath+0x12/0x71
arch/x86/entry/entry_64.S:186
INFO: Slab 0xffffea0000d2fd80 objects=12 used=11 fp=0xffff880034bf6690
flags=0x100000000000080
INFO: Object 0xffff880034bf6690 @offset=1680 fp=0x          (null)

Bytes b4 ffff880034bf6680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00  ................
Object ffff880034bf6690: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00  ................
Redzone ffff880034bf66a0: bb bb bb bb bb bb bb bb
    ........
Padding ffff880034bf67d8: 00 00 00 00 00 00 00 00
    ........
CPU: 0 PID: 2571 Comm: sshd Tainted: G    B   W       4.2.0-kasan #19
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
 ffff880034bf6000 ffff880032867a18 ffffffff82d58114 0000000000000053
 ffff880035c03b00 ffff880032867a48 ffffffff81422074 ffff880035c03b00
 ffffea0000d2fd80 ffff880034bf6690 0000000000000000 ffff880032867a78
Call Trace:
 [<     inlined    >] dump_stack+0x45/0x57 __dump_stack lib/dump_stack.c:15
 [<ffffffff82d58114>] dump_stack+0x45/0x57 lib/dump_stack.c:50
 [<ffffffff81422074>] print_trailer+0xf4/0x150 mm/slub.c:650
 [<ffffffff81426aa3>] object_err+0x33/0x40 mm/slub.c:657
 [<     inlined    >] kasan_report_error+0x1e1/0x420
print_address_description mm/kasan/report.c:120
 [<ffffffff81429461>] kasan_report_error+0x1e1/0x420 mm/kasan/report.c:194
 [<ffffffff819f0e50>] ? __radix_tree_lookup+0x2c0/0x2c0 lib/radix-tree.c:532
 [<ffffffff811f76b2>] ? call_rcu_sched+0x12/0x20 kernel/rcu/tree.c:3074
 [<ffffffff814296c8>] kasan_report.part.2+0x28/0x30 mm/kasan/report.c:223
 [<     inlined    >] ? page_cache_async_readahead+0x2cb/0x3f0
wb_congested include/linux/backing-dev.h:192
 [<     inlined    >] ? page_cache_async_readahead+0x2cb/0x3f0
inode_congested include/linux/backing-dev.h:528
 [<     inlined    >] ? page_cache_async_readahead+0x2cb/0x3f0
inode_read_congested include/linux/backing-dev.h:535
 [<ffffffff8137933b>] ? page_cache_async_readahead+0x2cb/0x3f0
mm/readahead.c:544
 [<     inlined    >] __asan_report_load8_noabort+0x29/0x30
kasan_report mm/kasan/report.c:244
 [<ffffffff81429789>] __asan_report_load8_noabort+0x29/0x30
mm/kasan/report.c:244
 [<     inlined    >] page_cache_async_readahead+0x2cb/0x3f0
wb_congested include/linux/backing-dev.h:192
 [<     inlined    >] page_cache_async_readahead+0x2cb/0x3f0
inode_congested include/linux/backing-dev.h:528
 [<     inlined    >] page_cache_async_readahead+0x2cb/0x3f0
inode_read_congested include/linux/backing-dev.h:535
 [<ffffffff8137933b>] page_cache_async_readahead+0x2cb/0x3f0 mm/readahead.c:544
 [<     inlined    >] filemap_fault+0x6a6/0xaa0
do_async_mmap_readahead mm/filemap.c:1864
 [<ffffffff8135b256>] filemap_fault+0x6a6/0xaa0 mm/filemap.c:1917
 [<     inlined    >] ? unlock_page+0xfb/0x150 wake_up_page
include/linux/pagemap.h:501
 [<ffffffff81355ddb>] ? unlock_page+0xfb/0x150 mm/filemap.c:774
 [<ffffffff813c520d>] __do_fault+0xdd/0x180 mm/memory.c:2756
 [<ffffffff813c5130>] ? print_bad_pte+0x670/0x670 mm/memory.c:678
 [<     inlined    >] handle_mm_fault+0x1575/0x2910 do_fault mm/memory.c:2945
 [<     inlined    >] handle_mm_fault+0x1575/0x2910 handle_pte_fault
mm/memory.c:3255
 [<     inlined    >] handle_mm_fault+0x1575/0x2910 __handle_mm_fault
mm/memory.c:3379
 [<ffffffff813cea45>] handle_mm_fault+0x1575/0x2910 mm/memory.c:3408
 [<ffffffff81111830>] ? retarget_shared_pending+0x360/0x360
include/linux/signal.h:117
 [<ffffffff813cd4d0>] ? copy_page_range+0x1140/0x1140 mm/memory.c:1024
 [<ffffffff81118db0>] ? sigprocmask+0x140/0x290 kernel/signal.c:2573
 [<ffffffff813d564c>] ? find_vma+0x1c/0x120 mm/mmap.c:2033
 [<ffffffff810ce4e3>] __do_page_fault+0x2c3/0x840 arch/x86/mm/fault.c:1235
 [<ffffffff810f8aa0>] ? SyS_waitid+0x1d0/0x1d0 kernel/exit.c:1577
 [<ffffffff810ceafa>] trace_do_page_fault+0x6a/0x1c0 arch/x86/mm/fault.c:1328
 [<ffffffff810c2364>] do_async_page_fault+0x14/0x70 arch/x86/kernel/kvm.c:264
 [<ffffffff82d70ab8>] async_page_fault+0x28/0x30 arch/x86/entry/entry_64.S:1081
Memory state around the buggy address:
 ffff880034bf6580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff880034bf6600: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff880034bf6680: fc fc fb fb fc fc fc fc fc fc fc fc fc fc fc fc
                         ^
 ffff880034bf6700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff880034bf6780: fc fc fc fc fc fc fc fc fc fc fc fc 00 00 fc fc
==================================================================

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Use-after-free in page_cache_async_readahead

[Tejun Heo]

Hello, Andrey.

On Wed, Sep 02, 2015 at 01:08:52PM +0200, Andrey Konovalov wrote:
> While running KASAN on 4.2 with Trinity I got the following report:
> 
> ==================================================================
> BUG: KASan: use after free in page_cache_async_readahead+0x2cb/0x3f0
> at addr ffff880034bf6690
> Read of size 8 by task sshd/2571
> =============================================================================
> BUG kmalloc-16 (Tainted: G        W      ): kasan: bad access detected
> -----------------------------------------------------------------------------
> 
> Disabling lock debugging due to kernel taint
> INFO: Allocated in bdi_init+0x168/0x960 age=554826 cpu=0 pid=6

Can you please verify that the following patch fixes the issue?

Thanks.

---
 block/blk-core.c            |    2 +-
 block/blk-sysfs.c           |    1 +
 include/linux/backing-dev.h |    6 +++++-
 mm/backing-dev.c            |   12 +++++++++++-
 4 files changed, 18 insertions(+), 3 deletions(-)

--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -578,7 +578,7 @@ void blk_cleanup_queue(struct request_qu
 		q->queue_lock = &q->__queue_lock;
 	spin_unlock_irq(lock);
 
-	bdi_destroy(&q->backing_dev_info);
+	bdi_unregister(&q->backing_dev_info);
 
 	/* @q is and will stay empty, shutdown and put */
 	blk_put_queue(q);
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -502,6 +502,7 @@ static void blk_release_queue(struct kob
 	struct request_queue *q =
 		container_of(kobj, struct request_queue, kobj);
 
+	bdi_exit(&q->backing_dev_info);
 	blkcg_exit_queue(q);
 
 	if (q->elevator) {
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -18,13 +18,17 @@
 #include <linux/slab.h>
 
 int __must_check bdi_init(struct backing_dev_info *bdi);
-void bdi_destroy(struct backing_dev_info *bdi);
+void bdi_exit(struct backing_dev_info *bdi);
 
 __printf(3, 4)
 int bdi_register(struct backing_dev_info *bdi, struct device *parent,
 		const char *fmt, ...);
 int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev);
+void bdi_unregister(struct backing_dev_info *bdi);
+
 int __must_check bdi_setup_and_register(struct backing_dev_info *, char *);
+void bdi_destroy(struct backing_dev_info *bdi);
+
 void wb_start_writeback(struct bdi_writeback *wb, long nr_pages,
 			bool range_cyclic, enum wb_reason reason);
 void wb_start_background_writeback(struct bdi_writeback *wb);
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -823,7 +823,7 @@ static void bdi_remove_from_list(struct
 	synchronize_rcu_expedited();
 }
 
-void bdi_destroy(struct backing_dev_info *bdi)
+void bdi_unregister(struct backing_dev_info *bdi)
 {
 	/* make sure nobody finds us on the bdi_list anymore */
 	bdi_remove_from_list(bdi);
@@ -835,9 +835,19 @@ void bdi_destroy(struct backing_dev_info
 		device_unregister(bdi->dev);
 		bdi->dev = NULL;
 	}
+}
 
+void bdi_exit(struct backing_dev_info *bdi)
+{
+	WARN_ON_ONCE(bdi->dev);
 	wb_exit(&bdi->wb);
 }
+
+void bdi_destroy(struct backing_dev_info *bdi)
+{
+	bdi_unregister(bdi);
+	bdi_exit(bdi);
+}
 EXPORT_SYMBOL(bdi_destroy);
 
 /*

Re: Use-after-free in page_cache_async_readahead

[Tejun Heo]

Hello, Andrey.

On Wed, Sep 02, 2015 at 01:08:52PM +0200, Andrey Konovalov wrote:
> While running KASAN on 4.2 with Trinity I got the following report:
> 
> ==================================================================
> BUG: KASan: use after free in page_cache_async_readahead+0x2cb/0x3f0
> at addr ffff880034bf6690
> Read of size 8 by task sshd/2571
> =============================================================================
> BUG kmalloc-16 (Tainted: G        W      ): kasan: bad access detected
> -----------------------------------------------------------------------------
> 
> Disabling lock debugging due to kernel taint
> INFO: Allocated in bdi_init+0x168/0x960 age=554826 cpu=0 pid=6

Can you please verify that the following patch fixes the issue?

Thanks.

---
 block/blk-core.c            |    2 +-
 block/blk-sysfs.c           |    1 +
 include/linux/backing-dev.h |    6 +++++-
 mm/backing-dev.c            |   12 +++++++++++-
 4 files changed, 18 insertions(+), 3 deletions(-)

--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -578,7 +578,7 @@ void blk_cleanup_queue(struct request_qu
 		q->queue_lock = &q->__queue_lock;
 	spin_unlock_irq(lock);
 
-	bdi_destroy(&q->backing_dev_info);
+	bdi_unregister(&q->backing_dev_info);
 
 	/* @q is and will stay empty, shutdown and put */
 	blk_put_queue(q);
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -502,6 +502,7 @@ static void blk_release_queue(struct kob
 	struct request_queue *q =
 		container_of(kobj, struct request_queue, kobj);
 
+	bdi_exit(&q->backing_dev_info);
 	blkcg_exit_queue(q);
 
 	if (q->elevator) {
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -18,13 +18,17 @@
 #include <linux/slab.h>
 
 int __must_check bdi_init(struct backing_dev_info *bdi);
-void bdi_destroy(struct backing_dev_info *bdi);
+void bdi_exit(struct backing_dev_info *bdi);
 
 __printf(3, 4)
 int bdi_register(struct backing_dev_info *bdi, struct device *parent,
 		const char *fmt, ...);
 int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev);
+void bdi_unregister(struct backing_dev_info *bdi);
+
 int __must_check bdi_setup_and_register(struct backing_dev_info *, char *);
+void bdi_destroy(struct backing_dev_info *bdi);
+
 void wb_start_writeback(struct bdi_writeback *wb, long nr_pages,
 			bool range_cyclic, enum wb_reason reason);
 void wb_start_background_writeback(struct bdi_writeback *wb);
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -823,7 +823,7 @@ static void bdi_remove_from_list(struct
 	synchronize_rcu_expedited();
 }
 
-void bdi_destroy(struct backing_dev_info *bdi)
+void bdi_unregister(struct backing_dev_info *bdi)
 {
 	/* make sure nobody finds us on the bdi_list anymore */
 	bdi_remove_from_list(bdi);
@@ -835,9 +835,19 @@ void bdi_destroy(struct backing_dev_info
 		device_unregister(bdi->dev);
 		bdi->dev = NULL;
 	}
+}
 
+void bdi_exit(struct backing_dev_info *bdi)
+{
+	WARN_ON_ONCE(bdi->dev);
 	wb_exit(&bdi->wb);
 }
+
+void bdi_destroy(struct backing_dev_info *bdi)
+{
+	bdi_unregister(bdi);
+	bdi_exit(bdi);
+}
 EXPORT_SYMBOL(bdi_destroy);
 
 /*

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Use-after-free in page_cache_async_readahead

[Andrey Konovalov]

On Wed, Sep 2, 2015 at 9:40 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Andrey.

Hello Tejun,

> On Wed, Sep 02, 2015 at 01:08:52PM +0200, Andrey Konovalov wrote:
>> While running KASAN on 4.2 with Trinity I got the following report:
>>
>> ==================================================================
>> BUG: KASan: use after free in page_cache_async_readahead+0x2cb/0x3f0
>> at addr ffff880034bf6690
>> Read of size 8 by task sshd/2571
>> =============================================================================
>> BUG kmalloc-16 (Tainted: G        W      ): kasan: bad access detected
>> -----------------------------------------------------------------------------
>>
>> Disabling lock debugging due to kernel taint
>> INFO: Allocated in bdi_init+0x168/0x960 age=554826 cpu=0 pid=6
>
> Can you please verify that the following patch fixes the issue?

I've hit this bug only twice during 24 hours of fuzzing, so there's no
fast way to verify this.
I'll be testing with your patch now, and I'll let you know if I hit
the bug again.

Thanks!

Re: Use-after-free in page_cache_async_readahead

[Andrey Konovalov]

On Wed, Sep 2, 2015 at 9:40 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Andrey.

Hello Tejun,

> On Wed, Sep 02, 2015 at 01:08:52PM +0200, Andrey Konovalov wrote:
>> While running KASAN on 4.2 with Trinity I got the following report:
>>
>> ==================================================================
>> BUG: KASan: use after free in page_cache_async_readahead+0x2cb/0x3f0
>> at addr ffff880034bf6690
>> Read of size 8 by task sshd/2571
>> =============================================================================
>> BUG kmalloc-16 (Tainted: G        W      ): kasan: bad access detected
>> -----------------------------------------------------------------------------
>>
>> Disabling lock debugging due to kernel taint
>> INFO: Allocated in bdi_init+0x168/0x960 age=554826 cpu=0 pid=6
>
> Can you please verify that the following patch fixes the issue?

I've hit this bug only twice during 24 hours of fuzzing, so there's no
fast way to verify this.
I'll be testing with your patch now, and I'll let you know if I hit
the bug again.

Thanks!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Fwd: Use-after-free in page_cache_async_readahead

[Andrey Konovalov]

On Thu, Sep 3, 2015 at 1:49 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> On Wed, Sep 2, 2015 at 9:40 PM, Tejun Heo <tj@kernel.org> wrote:
>> Hello, Andrey.
>
> Hello Tejun,
>
>> On Wed, Sep 02, 2015 at 01:08:52PM +0200, Andrey Konovalov wrote:
>>> While running KASAN on 4.2 with Trinity I got the following report:
>>>
>>> ==================================================================
>>> BUG: KASan: use after free in page_cache_async_readahead+0x2cb/0x3f0
>>> at addr ffff880034bf6690
>>> Read of size 8 by task sshd/2571
>>> =============================================================================
>>> BUG kmalloc-16 (Tainted: G        W      ): kasan: bad access detected
>>> -----------------------------------------------------------------------------
>>>
>>> Disabling lock debugging due to kernel taint
>>> INFO: Allocated in bdi_init+0x168/0x960 age=554826 cpu=0 pid=6
>>
>> Can you please verify that the following patch fixes the issue?
>
> I've hit this bug only twice during 24 hours of fuzzing, so there's no
> fast way to verify this.
> I'll be testing with your patch now, and I'll let you know if I hit
> the bug again.

Hello Tejun,

I haven't seen any reports while testing with your patch for the last
few days, so I think it's safe to say that your patch fixes the issue.

Thanks!

>
> Thanks!

Fwd: Use-after-free in page_cache_async_readahead

[Andrey Konovalov]

On Thu, Sep 3, 2015 at 1:49 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> On Wed, Sep 2, 2015 at 9:40 PM, Tejun Heo <tj@kernel.org> wrote:
>> Hello, Andrey.
>
> Hello Tejun,
>
>> On Wed, Sep 02, 2015 at 01:08:52PM +0200, Andrey Konovalov wrote:
>>> While running KASAN on 4.2 with Trinity I got the following report:
>>>
>>> ==================================================================
>>> BUG: KASan: use after free in page_cache_async_readahead+0x2cb/0x3f0
>>> at addr ffff880034bf6690
>>> Read of size 8 by task sshd/2571
>>> =============================================================================
>>> BUG kmalloc-16 (Tainted: G        W      ): kasan: bad access detected
>>> -----------------------------------------------------------------------------
>>>
>>> Disabling lock debugging due to kernel taint
>>> INFO: Allocated in bdi_init+0x168/0x960 age=554826 cpu=0 pid=6
>>
>> Can you please verify that the following patch fixes the issue?
>
> I've hit this bug only twice during 24 hours of fuzzing, so there's no
> fast way to verify this.
> I'll be testing with your patch now, and I'll let you know if I hit
> the bug again.

Hello Tejun,

I haven't seen any reports while testing with your patch for the last
few days, so I think it's safe to say that your patch fixes the issue.

Thanks!

>
> Thanks!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH block/for-linus] block: don't release bdi while request_queue has live references

[Tejun Heo]

bdi's are initialized in two steps, bdi_init() and bdi_register(), but
destroyed in a single step by bdi_destroy() which, for a bdi embedded
in a request_queue, is called during blk_cleanup_queue() which makes
the queue invisible and starts the draining of remaining usages.

A request_queue's user can access the congestion state of the embedded
bdi as long as it holds a reference to the queue.  As such, it may
access the congested state of a queue which finished
blk_cleanup_queue() but hasn't reached blk_release_queue() yet.
Because the congested state was embedded in backing_dev_info which in
turn is embedded in request_queue, accessing the congested state after
bdi_destroy() was called was fine.  The bdi was destroyed but the
memory region for the congested state remained accessible till the
queue got released.

a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in
bdi_writeback") changed the situation.  Now, the root congested state
which is expected to be pinned while request_queue remains accessible
is separately reference counted and the base ref is put during
bdi_destroy().  This means that the root congested state may go away
prematurely while the queue is between bdi_dstroy() and
blk_cleanup_queue(), which was detected by Andrey's KASAN tests.

The root cause of this problem is that bdi doesn't distinguish the two
steps of destruction, unregistration and release, and now the root
congested state actually requires a separate release step.  To fix the
issue, this patch separates out bdi_unregister() and bdi_exit() from
bdi_destroy().  bdi_unregister() is called from blk_cleanup_queue()
and bdi_exit() from blk_release_queue().  bdi_destroy() is now just a
simple wrapper calling the two steps back-to-back.

While at it, the prototype of bdi_destroy() is moved right below
bdi_setup_and_register() so that the counterpart operations are
located together.

Signed-off-by: Tejun Heo <tj@kernel.org>
Fixes: a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in bdi_writeback")
Cc: stable@vger.kernel.org # v4.2+
Reported-and-tested-by: Andrey Konovalov <andreyknvl@google.com>
Link: http://lkml.kernel.org/g/CAAeHK+zUJ74Zn17=rOyxacHU18SgCfC6bsYW=6kCY5GXJBwGfQ@mail.gmail.com
---
 block/blk-core.c            |    2 +-
 block/blk-sysfs.c           |    1 +
 include/linux/backing-dev.h |    6 +++++-
 mm/backing-dev.c            |   12 +++++++++++-
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 60912e9..ae49240 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -576,7 +576,7 @@ void blk_cleanup_queue(struct request_queue *q)
 		q->queue_lock = &q->__queue_lock;
 	spin_unlock_irq(lock);
 
-	bdi_destroy(&q->backing_dev_info);
+	bdi_unregister(&q->backing_dev_info);
 
 	/* @q is and will stay empty, shutdown and put */
 	blk_put_queue(q);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 3e44a9d..07b42f5 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -540,6 +540,7 @@ static void blk_release_queue(struct kobject *kobj)
 	struct request_queue *q =
 		container_of(kobj, struct request_queue, kobj);
 
+	bdi_exit(&q->backing_dev_info);
 	blkcg_exit_queue(q);
 
 	if (q->elevator) {
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 0fe9df9..fe0ab98 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -18,13 +18,17 @@
 #include <linux/slab.h>
 
 int __must_check bdi_init(struct backing_dev_info *bdi);
-void bdi_destroy(struct backing_dev_info *bdi);
+void bdi_exit(struct backing_dev_info *bdi);
 
 __printf(3, 4)
 int bdi_register(struct backing_dev_info *bdi, struct device *parent,
 		const char *fmt, ...);
 int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev);
+void bdi_unregister(struct backing_dev_info *bdi);
+
 int __must_check bdi_setup_and_register(struct backing_dev_info *, char *);
+void bdi_destroy(struct backing_dev_info *bdi);
+
 void wb_start_writeback(struct bdi_writeback *wb, long nr_pages,
 			bool range_cyclic, enum wb_reason reason);
 void wb_start_background_writeback(struct bdi_writeback *wb);
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index dac5bf5..dc07d88 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -823,7 +823,7 @@ static void bdi_remove_from_list(struct backing_dev_info *bdi)
 	synchronize_rcu_expedited();
 }
 
-void bdi_destroy(struct backing_dev_info *bdi)
+void bdi_unregister(struct backing_dev_info *bdi)
 {
 	/* make sure nobody finds us on the bdi_list anymore */
 	bdi_remove_from_list(bdi);
@@ -835,9 +835,19 @@ void bdi_destroy(struct backing_dev_info *bdi)
 		device_unregister(bdi->dev);
 		bdi->dev = NULL;
 	}
+}
 
+void bdi_exit(struct backing_dev_info *bdi)
+{
+	WARN_ON_ONCE(bdi->dev);
 	wb_exit(&bdi->wb);
 }
+
+void bdi_destroy(struct backing_dev_info *bdi)
+{
+	bdi_unregister(bdi);
+	bdi_exit(bdi);
+}
 EXPORT_SYMBOL(bdi_destroy);
 
 /*

[PATCH block/for-linus] block: don't release bdi while request_queue has live references

[Tejun Heo]

bdi's are initialized in two steps, bdi_init() and bdi_register(), but
destroyed in a single step by bdi_destroy() which, for a bdi embedded
in a request_queue, is called during blk_cleanup_queue() which makes
the queue invisible and starts the draining of remaining usages.

A request_queue's user can access the congestion state of the embedded
bdi as long as it holds a reference to the queue.  As such, it may
access the congested state of a queue which finished
blk_cleanup_queue() but hasn't reached blk_release_queue() yet.
Because the congested state was embedded in backing_dev_info which in
turn is embedded in request_queue, accessing the congested state after
bdi_destroy() was called was fine.  The bdi was destroyed but the
memory region for the congested state remained accessible till the
queue got released.

a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in
bdi_writeback") changed the situation.  Now, the root congested state
which is expected to be pinned while request_queue remains accessible
is separately reference counted and the base ref is put during
bdi_destroy().  This means that the root congested state may go away
prematurely while the queue is between bdi_dstroy() and
blk_cleanup_queue(), which was detected by Andrey's KASAN tests.

The root cause of this problem is that bdi doesn't distinguish the two
steps of destruction, unregistration and release, and now the root
congested state actually requires a separate release step.  To fix the
issue, this patch separates out bdi_unregister() and bdi_exit() from
bdi_destroy().  bdi_unregister() is called from blk_cleanup_queue()
and bdi_exit() from blk_release_queue().  bdi_destroy() is now just a
simple wrapper calling the two steps back-to-back.

While at it, the prototype of bdi_destroy() is moved right below
bdi_setup_and_register() so that the counterpart operations are
located together.

Signed-off-by: Tejun Heo <tj@kernel.org>
Fixes: a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in bdi_writeback")
Cc: stable@vger.kernel.org # v4.2+
Reported-and-tested-by: Andrey Konovalov <andreyknvl@google.com>
Link: http://lkml.kernel.org/g/CAAeHK+zUJ74Zn17=rOyxacHU18SgCfC6bsYW=6kCY5GXJBwGfQ@mail.gmail.com
---
 block/blk-core.c            |    2 +-
 block/blk-sysfs.c           |    1 +
 include/linux/backing-dev.h |    6 +++++-
 mm/backing-dev.c            |   12 +++++++++++-
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 60912e9..ae49240 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -576,7 +576,7 @@ void blk_cleanup_queue(struct request_queue *q)
 		q->queue_lock = &q->__queue_lock;
 	spin_unlock_irq(lock);
 
-	bdi_destroy(&q->backing_dev_info);
+	bdi_unregister(&q->backing_dev_info);
 
 	/* @q is and will stay empty, shutdown and put */
 	blk_put_queue(q);
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 3e44a9d..07b42f5 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -540,6 +540,7 @@ static void blk_release_queue(struct kobject *kobj)
 	struct request_queue *q =
 		container_of(kobj, struct request_queue, kobj);
 
+	bdi_exit(&q->backing_dev_info);
 	blkcg_exit_queue(q);
 
 	if (q->elevator) {
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index 0fe9df9..fe0ab98 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -18,13 +18,17 @@
 #include <linux/slab.h>
 
 int __must_check bdi_init(struct backing_dev_info *bdi);
-void bdi_destroy(struct backing_dev_info *bdi);
+void bdi_exit(struct backing_dev_info *bdi);
 
 __printf(3, 4)
 int bdi_register(struct backing_dev_info *bdi, struct device *parent,
 		const char *fmt, ...);
 int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev);
+void bdi_unregister(struct backing_dev_info *bdi);
+
 int __must_check bdi_setup_and_register(struct backing_dev_info *, char *);
+void bdi_destroy(struct backing_dev_info *bdi);
+
 void wb_start_writeback(struct bdi_writeback *wb, long nr_pages,
 			bool range_cyclic, enum wb_reason reason);
 void wb_start_background_writeback(struct bdi_writeback *wb);
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index dac5bf5..dc07d88 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -823,7 +823,7 @@ static void bdi_remove_from_list(struct backing_dev_info *bdi)
 	synchronize_rcu_expedited();
 }
 
-void bdi_destroy(struct backing_dev_info *bdi)
+void bdi_unregister(struct backing_dev_info *bdi)
 {
 	/* make sure nobody finds us on the bdi_list anymore */
 	bdi_remove_from_list(bdi);
@@ -835,9 +835,19 @@ void bdi_destroy(struct backing_dev_info *bdi)
 		device_unregister(bdi->dev);
 		bdi->dev = NULL;
 	}
+}
 
+void bdi_exit(struct backing_dev_info *bdi)
+{
+	WARN_ON_ONCE(bdi->dev);
 	wb_exit(&bdi->wb);
 }
+
+void bdi_destroy(struct backing_dev_info *bdi)
+{
+	bdi_unregister(bdi);
+	bdi_exit(bdi);
+}
 EXPORT_SYMBOL(bdi_destroy);
 
 /*

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH block/for-linus] block: don't release bdi while request_queue has live references

[Jan Kara]

On Tue 08-09-15 12:20:22, Tejun Heo wrote:
> bdi's are initialized in two steps, bdi_init() and bdi_register(), but
> destroyed in a single step by bdi_destroy() which, for a bdi embedded
> in a request_queue, is called during blk_cleanup_queue() which makes
> the queue invisible and starts the draining of remaining usages.
> 
> A request_queue's user can access the congestion state of the embedded
> bdi as long as it holds a reference to the queue.  As such, it may
> access the congested state of a queue which finished
> blk_cleanup_queue() but hasn't reached blk_release_queue() yet.
> Because the congested state was embedded in backing_dev_info which in
> turn is embedded in request_queue, accessing the congested state after
> bdi_destroy() was called was fine.  The bdi was destroyed but the
> memory region for the congested state remained accessible till the
> queue got released.
> 
> a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in
> bdi_writeback") changed the situation.  Now, the root congested state
> which is expected to be pinned while request_queue remains accessible
> is separately reference counted and the base ref is put during
> bdi_destroy().  This means that the root congested state may go away
> prematurely while the queue is between bdi_dstroy() and
> blk_cleanup_queue(), which was detected by Andrey's KASAN tests.
> 
> The root cause of this problem is that bdi doesn't distinguish the two
> steps of destruction, unregistration and release, and now the root
> congested state actually requires a separate release step.  To fix the
> issue, this patch separates out bdi_unregister() and bdi_exit() from
> bdi_destroy().  bdi_unregister() is called from blk_cleanup_queue()
> and bdi_exit() from blk_release_queue().  bdi_destroy() is now just a
> simple wrapper calling the two steps back-to-back.
> 
> While at it, the prototype of bdi_destroy() is moved right below
> bdi_setup_and_register() so that the counterpart operations are
> located together.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Fixes: a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in bdi_writeback")
> Cc: stable@vger.kernel.org # v4.2+
> Reported-and-tested-by: Andrey Konovalov <andreyknvl@google.com>
> Link: http://lkml.kernel.org/g/CAAeHK+zUJ74Zn17=rOyxacHU18SgCfC6bsYW=6kCY5GXJBwGfQ@mail.gmail.com

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.com>

								Honza

> ---
>  block/blk-core.c            |    2 +-
>  block/blk-sysfs.c           |    1 +
>  include/linux/backing-dev.h |    6 +++++-
>  mm/backing-dev.c            |   12 +++++++++++-
>  4 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 60912e9..ae49240 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -576,7 +576,7 @@ void blk_cleanup_queue(struct request_queue *q)
>  		q->queue_lock = &q->__queue_lock;
>  	spin_unlock_irq(lock);
>  
> -	bdi_destroy(&q->backing_dev_info);
> +	bdi_unregister(&q->backing_dev_info);
>  
>  	/* @q is and will stay empty, shutdown and put */
>  	blk_put_queue(q);
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 3e44a9d..07b42f5 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -540,6 +540,7 @@ static void blk_release_queue(struct kobject *kobj)
>  	struct request_queue *q =
>  		container_of(kobj, struct request_queue, kobj);
>  
> +	bdi_exit(&q->backing_dev_info);
>  	blkcg_exit_queue(q);
>  
>  	if (q->elevator) {
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 0fe9df9..fe0ab98 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -18,13 +18,17 @@
>  #include <linux/slab.h>
>  
>  int __must_check bdi_init(struct backing_dev_info *bdi);
> -void bdi_destroy(struct backing_dev_info *bdi);
> +void bdi_exit(struct backing_dev_info *bdi);
>  
>  __printf(3, 4)
>  int bdi_register(struct backing_dev_info *bdi, struct device *parent,
>  		const char *fmt, ...);
>  int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev);
> +void bdi_unregister(struct backing_dev_info *bdi);
> +
>  int __must_check bdi_setup_and_register(struct backing_dev_info *, char *);
> +void bdi_destroy(struct backing_dev_info *bdi);
> +
>  void wb_start_writeback(struct bdi_writeback *wb, long nr_pages,
>  			bool range_cyclic, enum wb_reason reason);
>  void wb_start_background_writeback(struct bdi_writeback *wb);
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index dac5bf5..dc07d88 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -823,7 +823,7 @@ static void bdi_remove_from_list(struct backing_dev_info *bdi)
>  	synchronize_rcu_expedited();
>  }
>  
> -void bdi_destroy(struct backing_dev_info *bdi)
> +void bdi_unregister(struct backing_dev_info *bdi)
>  {
>  	/* make sure nobody finds us on the bdi_list anymore */
>  	bdi_remove_from_list(bdi);
> @@ -835,9 +835,19 @@ void bdi_destroy(struct backing_dev_info *bdi)
>  		device_unregister(bdi->dev);
>  		bdi->dev = NULL;
>  	}
> +}
>  
> +void bdi_exit(struct backing_dev_info *bdi)
> +{
> +	WARN_ON_ONCE(bdi->dev);
>  	wb_exit(&bdi->wb);
>  }
> +
> +void bdi_destroy(struct backing_dev_info *bdi)
> +{
> +	bdi_unregister(bdi);
> +	bdi_exit(bdi);
> +}
>  EXPORT_SYMBOL(bdi_destroy);
>  
>  /*
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH block/for-linus] block: don't release bdi while request_queue has live references

[Jan Kara]

On Tue 08-09-15 12:20:22, Tejun Heo wrote:
> bdi's are initialized in two steps, bdi_init() and bdi_register(), but
> destroyed in a single step by bdi_destroy() which, for a bdi embedded
> in a request_queue, is called during blk_cleanup_queue() which makes
> the queue invisible and starts the draining of remaining usages.
> 
> A request_queue's user can access the congestion state of the embedded
> bdi as long as it holds a reference to the queue.  As such, it may
> access the congested state of a queue which finished
> blk_cleanup_queue() but hasn't reached blk_release_queue() yet.
> Because the congested state was embedded in backing_dev_info which in
> turn is embedded in request_queue, accessing the congested state after
> bdi_destroy() was called was fine.  The bdi was destroyed but the
> memory region for the congested state remained accessible till the
> queue got released.
> 
> a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in
> bdi_writeback") changed the situation.  Now, the root congested state
> which is expected to be pinned while request_queue remains accessible
> is separately reference counted and the base ref is put during
> bdi_destroy().  This means that the root congested state may go away
> prematurely while the queue is between bdi_dstroy() and
> blk_cleanup_queue(), which was detected by Andrey's KASAN tests.
> 
> The root cause of this problem is that bdi doesn't distinguish the two
> steps of destruction, unregistration and release, and now the root
> congested state actually requires a separate release step.  To fix the
> issue, this patch separates out bdi_unregister() and bdi_exit() from
> bdi_destroy().  bdi_unregister() is called from blk_cleanup_queue()
> and bdi_exit() from blk_release_queue().  bdi_destroy() is now just a
> simple wrapper calling the two steps back-to-back.
> 
> While at it, the prototype of bdi_destroy() is moved right below
> bdi_setup_and_register() so that the counterpart operations are
> located together.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Fixes: a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in bdi_writeback")
> Cc: stable@vger.kernel.org # v4.2+
> Reported-and-tested-by: Andrey Konovalov <andreyknvl@google.com>
> Link: http://lkml.kernel.org/g/CAAeHK+zUJ74Zn17=rOyxacHU18SgCfC6bsYW=6kCY5GXJBwGfQ@mail.gmail.com

The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.com>

								Honza

> ---
>  block/blk-core.c            |    2 +-
>  block/blk-sysfs.c           |    1 +
>  include/linux/backing-dev.h |    6 +++++-
>  mm/backing-dev.c            |   12 +++++++++++-
>  4 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 60912e9..ae49240 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -576,7 +576,7 @@ void blk_cleanup_queue(struct request_queue *q)
>  		q->queue_lock = &q->__queue_lock;
>  	spin_unlock_irq(lock);
>  
> -	bdi_destroy(&q->backing_dev_info);
> +	bdi_unregister(&q->backing_dev_info);
>  
>  	/* @q is and will stay empty, shutdown and put */
>  	blk_put_queue(q);
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 3e44a9d..07b42f5 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -540,6 +540,7 @@ static void blk_release_queue(struct kobject *kobj)
>  	struct request_queue *q =
>  		container_of(kobj, struct request_queue, kobj);
>  
> +	bdi_exit(&q->backing_dev_info);
>  	blkcg_exit_queue(q);
>  
>  	if (q->elevator) {
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 0fe9df9..fe0ab98 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -18,13 +18,17 @@
>  #include <linux/slab.h>
>  
>  int __must_check bdi_init(struct backing_dev_info *bdi);
> -void bdi_destroy(struct backing_dev_info *bdi);
> +void bdi_exit(struct backing_dev_info *bdi);
>  
>  __printf(3, 4)
>  int bdi_register(struct backing_dev_info *bdi, struct device *parent,
>  		const char *fmt, ...);
>  int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev);
> +void bdi_unregister(struct backing_dev_info *bdi);
> +
>  int __must_check bdi_setup_and_register(struct backing_dev_info *, char *);
> +void bdi_destroy(struct backing_dev_info *bdi);
> +
>  void wb_start_writeback(struct bdi_writeback *wb, long nr_pages,
>  			bool range_cyclic, enum wb_reason reason);
>  void wb_start_background_writeback(struct bdi_writeback *wb);
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index dac5bf5..dc07d88 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -823,7 +823,7 @@ static void bdi_remove_from_list(struct backing_dev_info *bdi)
>  	synchronize_rcu_expedited();
>  }
>  
> -void bdi_destroy(struct backing_dev_info *bdi)
> +void bdi_unregister(struct backing_dev_info *bdi)
>  {
>  	/* make sure nobody finds us on the bdi_list anymore */
>  	bdi_remove_from_list(bdi);
> @@ -835,9 +835,19 @@ void bdi_destroy(struct backing_dev_info *bdi)
>  		device_unregister(bdi->dev);
>  		bdi->dev = NULL;
>  	}
> +}
>  
> +void bdi_exit(struct backing_dev_info *bdi)
> +{
> +	WARN_ON_ONCE(bdi->dev);
>  	wb_exit(&bdi->wb);
>  }
> +
> +void bdi_destroy(struct backing_dev_info *bdi)
> +{
> +	bdi_unregister(bdi);
> +	bdi_exit(bdi);
> +}
>  EXPORT_SYMBOL(bdi_destroy);
>  
>  /*
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH block/for-linus] block: don't release bdi while request_queue has live references

[Jeff Moyer]

Tejun Heo <tj@kernel.org> writes:

[snip]

Thanks for the nice write-up of the problem.  This looks good to me.
The only minor nit I have is that you might want to rename
cgwb_bdi_destroy to cgwb_bdi_unregister.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>


> Signed-off-by: Tejun Heo <tj@kernel.org>
> Fixes: a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in bdi_writeback")
> Cc: stable@vger.kernel.org # v4.2+
> Reported-and-tested-by: Andrey Konovalov <andreyknvl@google.com>
> Link: http://lkml.kernel.org/g/CAAeHK+zUJ74Zn17=rOyxacHU18SgCfC6bsYW=6kCY5GXJBwGfQ@mail.gmail.com
> ---
>  block/blk-core.c            |    2 +-
>  block/blk-sysfs.c           |    1 +
>  include/linux/backing-dev.h |    6 +++++-
>  mm/backing-dev.c            |   12 +++++++++++-
>  4 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 60912e9..ae49240 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -576,7 +576,7 @@ void blk_cleanup_queue(struct request_queue *q)
>  		q->queue_lock = &q->__queue_lock;
>  	spin_unlock_irq(lock);
>  
> -	bdi_destroy(&q->backing_dev_info);
> +	bdi_unregister(&q->backing_dev_info);
>  
>  	/* @q is and will stay empty, shutdown and put */
>  	blk_put_queue(q);
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 3e44a9d..07b42f5 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -540,6 +540,7 @@ static void blk_release_queue(struct kobject *kobj)
>  	struct request_queue *q =
>  		container_of(kobj, struct request_queue, kobj);
>  
> +	bdi_exit(&q->backing_dev_info);
>  	blkcg_exit_queue(q);
>  
>  	if (q->elevator) {
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 0fe9df9..fe0ab98 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -18,13 +18,17 @@
>  #include <linux/slab.h>
>  
>  int __must_check bdi_init(struct backing_dev_info *bdi);
> -void bdi_destroy(struct backing_dev_info *bdi);
> +void bdi_exit(struct backing_dev_info *bdi);
>  
>  __printf(3, 4)
>  int bdi_register(struct backing_dev_info *bdi, struct device *parent,
>  		const char *fmt, ...);
>  int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev);
> +void bdi_unregister(struct backing_dev_info *bdi);
> +
>  int __must_check bdi_setup_and_register(struct backing_dev_info *, char *);
> +void bdi_destroy(struct backing_dev_info *bdi);
> +
>  void wb_start_writeback(struct bdi_writeback *wb, long nr_pages,
>  			bool range_cyclic, enum wb_reason reason);
>  void wb_start_background_writeback(struct bdi_writeback *wb);
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index dac5bf5..dc07d88 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -823,7 +823,7 @@ static void bdi_remove_from_list(struct backing_dev_info *bdi)
>  	synchronize_rcu_expedited();
>  }
>  
> -void bdi_destroy(struct backing_dev_info *bdi)
> +void bdi_unregister(struct backing_dev_info *bdi)
>  {
>  	/* make sure nobody finds us on the bdi_list anymore */
>  	bdi_remove_from_list(bdi);
> @@ -835,9 +835,19 @@ void bdi_destroy(struct backing_dev_info *bdi)
>  		device_unregister(bdi->dev);
>  		bdi->dev = NULL;
>  	}
> +}
>  
> +void bdi_exit(struct backing_dev_info *bdi)
> +{
> +	WARN_ON_ONCE(bdi->dev);
>  	wb_exit(&bdi->wb);
>  }
> +
> +void bdi_destroy(struct backing_dev_info *bdi)
> +{
> +	bdi_unregister(bdi);
> +	bdi_exit(bdi);
> +}
>  EXPORT_SYMBOL(bdi_destroy);
>  
>  /*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH block/for-linus] block: don't release bdi while request_queue has live references

[Jeff Moyer]

Tejun Heo <tj@kernel.org> writes:

[snip]

Thanks for the nice write-up of the problem.  This looks good to me.
The only minor nit I have is that you might want to rename
cgwb_bdi_destroy to cgwb_bdi_unregister.

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>


> Signed-off-by: Tejun Heo <tj@kernel.org>
> Fixes: a13f35e87140 ("writeback: don't embed root bdi_writeback_congested in bdi_writeback")
> Cc: stable@vger.kernel.org # v4.2+
> Reported-and-tested-by: Andrey Konovalov <andreyknvl@google.com>
> Link: http://lkml.kernel.org/g/CAAeHK+zUJ74Zn17=rOyxacHU18SgCfC6bsYW=6kCY5GXJBwGfQ@mail.gmail.com
> ---
>  block/blk-core.c            |    2 +-
>  block/blk-sysfs.c           |    1 +
>  include/linux/backing-dev.h |    6 +++++-
>  mm/backing-dev.c            |   12 +++++++++++-
>  4 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 60912e9..ae49240 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -576,7 +576,7 @@ void blk_cleanup_queue(struct request_queue *q)
>  		q->queue_lock = &q->__queue_lock;
>  	spin_unlock_irq(lock);
>  
> -	bdi_destroy(&q->backing_dev_info);
> +	bdi_unregister(&q->backing_dev_info);
>  
>  	/* @q is and will stay empty, shutdown and put */
>  	blk_put_queue(q);
> diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
> index 3e44a9d..07b42f5 100644
> --- a/block/blk-sysfs.c
> +++ b/block/blk-sysfs.c
> @@ -540,6 +540,7 @@ static void blk_release_queue(struct kobject *kobj)
>  	struct request_queue *q =
>  		container_of(kobj, struct request_queue, kobj);
>  
> +	bdi_exit(&q->backing_dev_info);
>  	blkcg_exit_queue(q);
>  
>  	if (q->elevator) {
> diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
> index 0fe9df9..fe0ab98 100644
> --- a/include/linux/backing-dev.h
> +++ b/include/linux/backing-dev.h
> @@ -18,13 +18,17 @@
>  #include <linux/slab.h>
>  
>  int __must_check bdi_init(struct backing_dev_info *bdi);
> -void bdi_destroy(struct backing_dev_info *bdi);
> +void bdi_exit(struct backing_dev_info *bdi);
>  
>  __printf(3, 4)
>  int bdi_register(struct backing_dev_info *bdi, struct device *parent,
>  		const char *fmt, ...);
>  int bdi_register_dev(struct backing_dev_info *bdi, dev_t dev);
> +void bdi_unregister(struct backing_dev_info *bdi);
> +
>  int __must_check bdi_setup_and_register(struct backing_dev_info *, char *);
> +void bdi_destroy(struct backing_dev_info *bdi);
> +
>  void wb_start_writeback(struct bdi_writeback *wb, long nr_pages,
>  			bool range_cyclic, enum wb_reason reason);
>  void wb_start_background_writeback(struct bdi_writeback *wb);
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index dac5bf5..dc07d88 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -823,7 +823,7 @@ static void bdi_remove_from_list(struct backing_dev_info *bdi)
>  	synchronize_rcu_expedited();
>  }
>  
> -void bdi_destroy(struct backing_dev_info *bdi)
> +void bdi_unregister(struct backing_dev_info *bdi)
>  {
>  	/* make sure nobody finds us on the bdi_list anymore */
>  	bdi_remove_from_list(bdi);
> @@ -835,9 +835,19 @@ void bdi_destroy(struct backing_dev_info *bdi)
>  		device_unregister(bdi->dev);
>  		bdi->dev = NULL;
>  	}
> +}
>  
> +void bdi_exit(struct backing_dev_info *bdi)
> +{
> +	WARN_ON_ONCE(bdi->dev);
>  	wb_exit(&bdi->wb);
>  }
> +
> +void bdi_destroy(struct backing_dev_info *bdi)
> +{
> +	bdi_unregister(bdi);
> +	bdi_exit(bdi);
> +}
>  EXPORT_SYMBOL(bdi_destroy);
>  
>  /*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>