[PATCH 0/2] Fix some bugs in ubi_resize_volume() function

[PATCH 0/2] Fix some bugs in ubi_resize_volume() function

[ZhaoLong Wang]

This series of patches does two things:
  1. Rebase the patch commited by Guo Xuenan on the latest mainline.
  2. Fixed an issue where the number of PEBs is incorrectly displayed.

Guo Xuenan (1):
  ubi: fix slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130

ZhaoLong Wang (1):
  ubi: Correct the number of PEBs after a volume resize failure

 drivers/mtd/ubi/vmt.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

-- 
2.31.1

[PATCH 1/2] ubi: fix slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130

[ZhaoLong Wang]

From: Guo Xuenan <guoxuenan@huawei.com>

When using ioctl interface to resize ubi volume, ubi_resize_volume will
resize eba table first, but not change vol->reserved_pebs in the same
atomic context which may cause concurrency access eba table.

For example, When user do shrink ubi volume A calling ubi_resize_volume,
while the other thread is writing (volume B) and triggering wear-leveling,
which may calling ubi_write_fastmap, under these circumstances, KASAN may
report: slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130.

The main work of this patch include:
1. fix races in ubi_resize_volume and ubi_update_fastmap, to avoid
   eba_tbl read out of bounds. first, we make eba_tbl and reserved_pebs
   updating under the protect of vol->volumes_lock. second, rollback
   volume in case of resize failure. Also mention that for volume
   shrinking failure, since part of volume has been shrunk and unmapped,
   there is no need to recover {rsvd/avail}_pebs.
2. fix some memleak in error path of ubi_resize_volume when destroy
   new_eba_tbl.

==================================================================
BUG: KASAN: slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130 [ubi]
Read of size 4 at addr ffff88800f43f570 by task kworker/u16:0/7
CPU: 0 PID: 7 Comm: kworker/u16:0 Not tainted 5.16.0-rc7 #3
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
Workqueue: writeback wb_workfn (flush-ubifs_0_0)
Call Trace:
 <TASK>
 dump_stack_lvl+0x4d/0x66
 print_address_description.constprop.0+0x41/0x60
 kasan_report.cold+0x83/0xdf
 ubi_eba_get_ldesc+0xfb/0x130 [ubi]
 ubi_update_fastmap.cold+0x60f/0xc7d [ubi]
 ubi_wl_get_peb+0x25b/0x4f0 [ubi]
 try_write_vid_and_data+0x9a/0x4d0 [ubi]
 ubi_eba_write_leb+0x7e4/0x17d0 [ubi]
 ubi_leb_map+0x1a0/0x2c0 [ubi]
 ubifs_leb_map+0x139/0x270 [ubifs]
 ubifs_add_bud_to_log+0xb40/0xf30 [ubifs]
 make_reservation+0x86e/0xb00 [ubifs]
 ubifs_jnl_write_data+0x430/0x9d0 [ubifs]
 do_writepage+0x1d1/0x550 [ubifs]
 ubifs_writepage+0x37c/0x670 [ubifs]
 __writepage+0x67/0x170
 write_cache_pages+0x259/0xa90
 do_writepages+0x277/0x5d0
 __writeback_single_inode+0xb8/0x850
 writeback_sb_inodes+0x4b3/0xb20
 __writeback_inodes_wb+0xc1/0x220
 wb_writeback+0x59f/0x740
 wb_workfn+0x6d0/0xca0
 process_one_work+0x711/0xfc0
 worker_thread+0x95/0xd00
 kthread+0x3a6/0x490
 ret_from_fork+0x1f/0x30
 </TASK>

Allocated by task 711:
 kasan_save_stack+0x1e/0x50
 __kasan_kmalloc+0x81/0xa0
 ubi_eba_create_table+0x88/0x1a0 [ubi]
 ubi_resize_volume.cold+0x175/0xae7 [ubi]
 ubi_cdev_ioctl+0x57f/0x1a60 [ubi]
 __x64_sys_ioctl+0x13a/0x1c0
 do_syscall_64+0x35/0x80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Last potentially related work creation:
 kasan_save_stack+0x1e/0x50
 __kasan_record_aux_stack+0xb7/0xc0
 call_rcu+0xd6/0x1000
 blk_stat_free_callback+0x28/0x30
 blk_release_queue+0x8a/0x2e0
 kobject_put+0x186/0x4c0
 scsi_device_dev_release_usercontext+0x620/0xbd0
 execute_in_process_context+0x2f/0x120
 device_release+0xa4/0x240
 kobject_put+0x186/0x4c0
 put_device+0x20/0x30
 __scsi_remove_device+0x1c3/0x300
 scsi_probe_and_add_lun+0x2140/0x2eb0
 __scsi_scan_target+0x1f2/0xbb0
 scsi_scan_channel+0x11b/0x1a0
 scsi_scan_host_selected+0x24c/0x310
 do_scsi_scan_host+0x1e0/0x250
 do_scan_async+0x45/0x490
 async_run_entry_fn+0xa2/0x530
 process_one_work+0x711/0xfc0
 worker_thread+0x95/0xd00
 kthread+0x3a6/0x490
 ret_from_fork+0x1f/0x30
The buggy address belongs to the object at ffff88800f43f500
 which belongs to the cache kmalloc-128 of size 128
The buggy address is located 112 bytes inside of
 128-byte region [ffff88800f43f500, ffff88800f43f580)
The buggy address belongs to the page:
page:ffffea00003d0f00 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0xf43c
head:ffffea00003d0f00 order:2 compound_mapcount:0 compound_pincount:0
flags: 0x1fffff80010200(slab|head|node=0|zone=1|lastcpupid=0x1fffff)
raw: 001fffff80010200 ffffea000046ba08 ffffea0000457208 ffff88810004d1c0
raw: 0000000000000000 0000000000190019 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
 ffff88800f43f400: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88800f43f480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff88800f43f500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fc fc
                                                             ^
 ffff88800f43f580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
 ffff88800f43f600: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc

The following steps can used to reproduce:
Process 1: write and trigger ubi wear-leveling
    ubimkvol /dev/ubi0 -s 5000MiB -N v1
    ubimkvol /dev/ubi0 -s 2000MiB -N v2
    ubimkvol /dev/ubi0 -s 10MiB -N v3
    mount -t ubifs /dev/ubi0_0 /mnt/ubifs
    while true;
    do
        filename=/mnt/ubifs/$((RANDOM))
        dd if=/dev/random of=${filename} bs=1M count=$((RANDOM % 1000))
        rm -rf ${filename}
        sync /mnt/ubifs/
    done

Process 2: do random resize
    struct ubi_rsvol_req req;
    req.vol_id = 1;
    req.bytes = (rand() % 50) * 512KB;
    ioctl(fd, UBI_IOCRSVOL, &req);

Signed-off-by: Guo Xuenan <guoxuenan@huawei.com>
Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
---
 drivers/mtd/ubi/vmt.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 2c867d16f89f..97294def01eb 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -408,6 +408,7 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
 	struct ubi_device *ubi = vol->ubi;
 	struct ubi_vtbl_record vtbl_rec;
 	struct ubi_eba_table *new_eba_tbl = NULL;
+	struct ubi_eba_table *old_eba_tbl = NULL;
 	int vol_id = vol->vol_id;
 
 	if (ubi->ro_mode)
@@ -453,10 +454,13 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
 			err = -ENOSPC;
 			goto out_free;
 		}
+
 		ubi->avail_pebs -= pebs;
 		ubi->rsvd_pebs += pebs;
 		ubi_eba_copy_table(vol, new_eba_tbl, vol->reserved_pebs);
-		ubi_eba_replace_table(vol, new_eba_tbl);
+		old_eba_tbl = vol->eba_tbl;
+		vol->eba_tbl = new_eba_tbl;
+		vol->reserved_pebs = reserved_pebs;
 		spin_unlock(&ubi->volumes_lock);
 	}
 
@@ -471,7 +475,9 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
 		ubi->avail_pebs -= pebs;
 		ubi_update_reserved(ubi);
 		ubi_eba_copy_table(vol, new_eba_tbl, reserved_pebs);
-		ubi_eba_replace_table(vol, new_eba_tbl);
+		old_eba_tbl = vol->eba_tbl;
+		vol->eba_tbl = new_eba_tbl;
+		vol->reserved_pebs = reserved_pebs;
 		spin_unlock(&ubi->volumes_lock);
 	}
 
@@ -493,7 +499,6 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
 	if (err)
 		goto out_acc;
 
-	vol->reserved_pebs = reserved_pebs;
 	if (vol->vol_type == UBI_DYNAMIC_VOLUME) {
 		vol->used_ebs = reserved_pebs;
 		vol->last_eb_bytes = vol->usable_leb_size;
@@ -501,19 +506,24 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
 			(long long)vol->used_ebs * vol->usable_leb_size;
 	}
 
+	/* destroy old table */
+	ubi_eba_destroy_table(old_eba_tbl);
 	ubi_volume_notify(ubi, vol, UBI_VOLUME_RESIZED);
 	self_check_volumes(ubi);
 	return err;
 
 out_acc:
+	spin_lock(&ubi->volumes_lock);
+	vol->reserved_pebs = reserved_pebs - pebs;
 	if (pebs > 0) {
-		spin_lock(&ubi->volumes_lock);
 		ubi->rsvd_pebs -= pebs;
 		ubi->avail_pebs += pebs;
-		spin_unlock(&ubi->volumes_lock);
+		ubi_eba_copy_table(vol, old_eba_tbl, vol->reserved_pebs);
+	} else {
+		ubi_eba_copy_table(vol, old_eba_tbl, reserved_pebs);
 	}
-	return err;
-
+	vol->eba_tbl = old_eba_tbl;
+	spin_unlock(&ubi->volumes_lock);
 out_free:
 	ubi_eba_destroy_table(new_eba_tbl);
 	return err;
-- 
2.31.1

[PATCH 2/2] ubi: Correct the number of PEBs after a volume resize failure

[ZhaoLong Wang]

In the error handing path `out_acc` of the ubi_resize_volume(),
when `pebs < 0`, it means that the volume table record fails to be
updated when the volume is shrinked. In this case, the number of
ubi->avail_pebs` and `ubi->rsvd_pebs` should also be restored,
otherwise the UBI will display an incorrect number of available PEBs.

Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
---
 drivers/mtd/ubi/vmt.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
index 97294def01eb..990571287e84 100644
--- a/drivers/mtd/ubi/vmt.c
+++ b/drivers/mtd/ubi/vmt.c
@@ -515,13 +515,12 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
 out_acc:
 	spin_lock(&ubi->volumes_lock);
 	vol->reserved_pebs = reserved_pebs - pebs;
-	if (pebs > 0) {
-		ubi->rsvd_pebs -= pebs;
-		ubi->avail_pebs += pebs;
+	ubi->rsvd_pebs -= pebs;
+	ubi->avail_pebs += pebs;
+	if (pebs > 0)
 		ubi_eba_copy_table(vol, old_eba_tbl, vol->reserved_pebs);
-	} else {
+	else
 		ubi_eba_copy_table(vol, old_eba_tbl, reserved_pebs);
-	}
 	vol->eba_tbl = old_eba_tbl;
 	spin_unlock(&ubi->volumes_lock);
 out_free:
-- 
2.31.1

Re: [PATCH 1/2] ubi: fix slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130

[Zhihao Cheng]

Hi
> From: Guo Xuenan <guoxuenan@huawei.com>
> 
> When using ioctl interface to resize ubi volume, ubi_resize_volume will
> resize eba table first, but not change vol->reserved_pebs in the same
> atomic context which may cause concurrency access eba table.
> 
> For example, When user do shrink ubi volume A calling ubi_resize_volume,
> while the other thread is writing (volume B) and triggering wear-leveling,
> which may calling ubi_write_fastmap, under these circumstances, KASAN may
> report: slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130.
> 
> The main work of this patch include:
> 1. fix races in ubi_resize_volume and ubi_update_fastmap, to avoid
>     eba_tbl read out of bounds. first, we make eba_tbl and reserved_pebs
>     updating under the protect of vol->volumes_lock. second, rollback
>     volume in case of resize failure. Also mention that for volume
>     shrinking failure, since part of volume has been shrunk and unmapped,
>     there is no need to recover {rsvd/avail}_pebs.
> 2. fix some memleak in error path of ubi_resize_volume when destroy
>     new_eba_tbl.
> 
> ==================================================================
> BUG: KASAN: slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130 [ubi]
> Read of size 4 at addr ffff88800f43f570 by task kworker/u16:0/7
> CPU: 0 PID: 7 Comm: kworker/u16:0 Not tainted 5.16.0-rc7 #3
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> Workqueue: writeback wb_workfn (flush-ubifs_0_0)
> Call Trace:
>   <TASK>
>   dump_stack_lvl+0x4d/0x66
>   print_address_description.constprop.0+0x41/0x60
>   kasan_report.cold+0x83/0xdf
>   ubi_eba_get_ldesc+0xfb/0x130 [ubi]
>   ubi_update_fastmap.cold+0x60f/0xc7d [ubi]
>   ubi_wl_get_peb+0x25b/0x4f0 [ubi]
>   try_write_vid_and_data+0x9a/0x4d0 [ubi]
>   ubi_eba_write_leb+0x7e4/0x17d0 [ubi]
>   ubi_leb_map+0x1a0/0x2c0 [ubi]
>   ubifs_leb_map+0x139/0x270 [ubifs]
>   ubifs_add_bud_to_log+0xb40/0xf30 [ubifs]
>   make_reservation+0x86e/0xb00 [ubifs]
>   ubifs_jnl_write_data+0x430/0x9d0 [ubifs]
>   do_writepage+0x1d1/0x550 [ubifs]
>   ubifs_writepage+0x37c/0x670 [ubifs]
>   __writepage+0x67/0x170
>   write_cache_pages+0x259/0xa90
>   do_writepages+0x277/0x5d0
>   __writeback_single_inode+0xb8/0x850
>   writeback_sb_inodes+0x4b3/0xb20
>   __writeback_inodes_wb+0xc1/0x220
>   wb_writeback+0x59f/0x740
>   wb_workfn+0x6d0/0xca0
>   process_one_work+0x711/0xfc0
>   worker_thread+0x95/0xd00
>   kthread+0x3a6/0x490
>   ret_from_fork+0x1f/0x30
>   </TASK>
> 
> Allocated by task 711:
>   kasan_save_stack+0x1e/0x50
>   __kasan_kmalloc+0x81/0xa0
>   ubi_eba_create_table+0x88/0x1a0 [ubi]
>   ubi_resize_volume.cold+0x175/0xae7 [ubi]
>   ubi_cdev_ioctl+0x57f/0x1a60 [ubi]
>   __x64_sys_ioctl+0x13a/0x1c0
>   do_syscall_64+0x35/0x80
>   entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> Last potentially related work creation:
>   kasan_save_stack+0x1e/0x50
>   __kasan_record_aux_stack+0xb7/0xc0
>   call_rcu+0xd6/0x1000
>   blk_stat_free_callback+0x28/0x30
>   blk_release_queue+0x8a/0x2e0
>   kobject_put+0x186/0x4c0
>   scsi_device_dev_release_usercontext+0x620/0xbd0
>   execute_in_process_context+0x2f/0x120
>   device_release+0xa4/0x240
>   kobject_put+0x186/0x4c0
>   put_device+0x20/0x30
>   __scsi_remove_device+0x1c3/0x300
>   scsi_probe_and_add_lun+0x2140/0x2eb0
>   __scsi_scan_target+0x1f2/0xbb0
>   scsi_scan_channel+0x11b/0x1a0
>   scsi_scan_host_selected+0x24c/0x310
>   do_scsi_scan_host+0x1e0/0x250
>   do_scan_async+0x45/0x490
>   async_run_entry_fn+0xa2/0x530
>   process_one_work+0x711/0xfc0
>   worker_thread+0x95/0xd00
>   kthread+0x3a6/0x490
>   ret_from_fork+0x1f/0x30
> The buggy address belongs to the object at ffff88800f43f500
>   which belongs to the cache kmalloc-128 of size 128
> The buggy address is located 112 bytes inside of
>   128-byte region [ffff88800f43f500, ffff88800f43f580)
> The buggy address belongs to the page:
> page:ffffea00003d0f00 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0xf43c
> head:ffffea00003d0f00 order:2 compound_mapcount:0 compound_pincount:0
> flags: 0x1fffff80010200(slab|head|node=0|zone=1|lastcpupid=0x1fffff)
> raw: 001fffff80010200 ffffea000046ba08 ffffea0000457208 ffff88810004d1c0
> raw: 0000000000000000 0000000000190019 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
> Memory state around the buggy address:
>   ffff88800f43f400: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>   ffff88800f43f480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>> ffff88800f43f500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fc fc
>                                                               ^
>   ffff88800f43f580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>   ffff88800f43f600: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> 
> The following steps can used to reproduce:
> Process 1: write and trigger ubi wear-leveling
>      ubimkvol /dev/ubi0 -s 5000MiB -N v1
>      ubimkvol /dev/ubi0 -s 2000MiB -N v2
>      ubimkvol /dev/ubi0 -s 10MiB -N v3
>      mount -t ubifs /dev/ubi0_0 /mnt/ubifs
>      while true;
>      do
>          filename=/mnt/ubifs/$((RANDOM))
>          dd if=/dev/random of=${filename} bs=1M count=$((RANDOM % 1000))
>          rm -rf ${filename}
>          sync /mnt/ubifs/
>      done
> 
> Process 2: do random resize
>      struct ubi_rsvol_req req;
>      req.vol_id = 1;
>      req.bytes = (rand() % 50) * 512KB;
>      ioctl(fd, UBI_IOCRSVOL, &req);
> 
> Signed-off-by: Guo Xuenan <guoxuenan@huawei.com>
> Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>
> ---
>   drivers/mtd/ubi/vmt.c | 24 +++++++++++++++++-------
>   1 file changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
> index 2c867d16f89f..97294def01eb 100644
> --- a/drivers/mtd/ubi/vmt.c
> +++ b/drivers/mtd/ubi/vmt.c
> @@ -408,6 +408,7 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
>   	struct ubi_device *ubi = vol->ubi;
>   	struct ubi_vtbl_record vtbl_rec;
>   	struct ubi_eba_table *new_eba_tbl = NULL;
> +	struct ubi_eba_table *old_eba_tbl = NULL;
>   	int vol_id = vol->vol_id;
>   
>   	if (ubi->ro_mode)
> @@ -453,10 +454,13 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
>   			err = -ENOSPC;
>   			goto out_free;
>   		}
> +
>   		ubi->avail_pebs -= pebs;
>   		ubi->rsvd_pebs += pebs;
>   		ubi_eba_copy_table(vol, new_eba_tbl, vol->reserved_pebs);
> -		ubi_eba_replace_table(vol, new_eba_tbl);
> +		old_eba_tbl = vol->eba_tbl;
> +		vol->eba_tbl = new_eba_tbl;
> +		vol->reserved_pebs = reserved_pebs;
>   		spin_unlock(&ubi->volumes_lock);
>   	}
>   
> @@ -471,7 +475,9 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
>   		ubi->avail_pebs -= pebs;
>   		ubi_update_reserved(ubi);
>   		ubi_eba_copy_table(vol, new_eba_tbl, reserved_pebs);
> -		ubi_eba_replace_table(vol, new_eba_tbl);
> +		old_eba_tbl = vol->eba_tbl;
> +		vol->eba_tbl = new_eba_tbl;
> +		vol->reserved_pebs = reserved_pebs;
>   		spin_unlock(&ubi->volumes_lock);
>   	}
>   
> @@ -493,7 +499,6 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
>   	if (err)
>   		goto out_acc;
>   
> -	vol->reserved_pebs = reserved_pebs;
>   	if (vol->vol_type == UBI_DYNAMIC_VOLUME) {
>   		vol->used_ebs = reserved_pebs;
>   		vol->last_eb_bytes = vol->usable_leb_size;
> @@ -501,19 +506,24 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
>   			(long long)vol->used_ebs * vol->usable_leb_size;
>   	}
>   
> +	/* destroy old table */
> +	ubi_eba_destroy_table(old_eba_tbl);
>   	ubi_volume_notify(ubi, vol, UBI_VOLUME_RESIZED);
>   	self_check_volumes(ubi);
>   	return err;
>   
>   out_acc:
> +	spin_lock(&ubi->volumes_lock);
> +	vol->reserved_pebs = reserved_pebs - pebs;
>   	if (pebs > 0) {
> -		spin_lock(&ubi->volumes_lock);
>   		ubi->rsvd_pebs -= pebs;
>   		ubi->avail_pebs += pebs;
> -		spin_unlock(&ubi->volumes_lock);
> +		ubi_eba_copy_table(vol, old_eba_tbl, vol->reserved_pebs);
> +	} else {
> +		ubi_eba_copy_table(vol, old_eba_tbl, reserved_pebs);
>   	}
> -	return err;
> -
> +	vol->eba_tbl = old_eba_tbl;
> +	spin_unlock(&ubi->volumes_lock);
>   out_free:
>   	ubi_eba_destroy_table(new_eba_tbl);
>   	return err;
> 

Besides that, it's better to protect 'vol->eba_tbl->entries' assignment 
like:
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 403b79d6efd5..5ae0c1bc6f41 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -1450,7 +1450,9 @@ int ubi_eba_copy_leb(struct ubi_device *ubi, int 
from, int to,
         }

         ubi_assert(vol->eba_tbl->entries[lnum].pnum == from);
+       spin_lock(&ubi->volumes_lock);
         vol->eba_tbl->entries[lnum].pnum = to;
+       spin_unlock(&ubi->volumes_lock);

  out_unlock_buf:
         mutex_unlock(&ubi->buf_mutex);

Otherwise, a race between wear_leveling_work and shrinking volume could 
happen:

  ubi_resize_volume         wear_leveling_worker
   ubi_eba_copy_table(vol, new_eba_tbl, reserved_pebs);
                               vol->eba_tbl->entries[lnum].pnum = to; 
// update old eba_tbl
   vol->eba_tbl = new_eba_tbl

Re: [PATCH 2/2] ubi: Correct the number of PEBs after a volume resize failure

[Zhihao Cheng]

> In the error handing path `out_acc` of the ubi_resize_volume(),
> when `pebs < 0`, it means that the volume table record fails to be
> updated when the volume is shrinked. In this case, the number of
> ubi->avail_pebs` and `ubi->rsvd_pebs` should also be restored,
> otherwise the UBI will display an incorrect number of available PEBs.
> 
> Signed-off-by: ZhaoLong Wang <wangzhaolong1@huawei.com>

Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>

> ---
>   drivers/mtd/ubi/vmt.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
> index 97294def01eb..990571287e84 100644
> --- a/drivers/mtd/ubi/vmt.c
> +++ b/drivers/mtd/ubi/vmt.c
> @@ -515,13 +515,12 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
>   out_acc:
>   	spin_lock(&ubi->volumes_lock);
>   	vol->reserved_pebs = reserved_pebs - pebs;
> -	if (pebs > 0) {
> -		ubi->rsvd_pebs -= pebs;
> -		ubi->avail_pebs += pebs;
> +	ubi->rsvd_pebs -= pebs;
> +	ubi->avail_pebs += pebs;
> +	if (pebs > 0)
>   		ubi_eba_copy_table(vol, old_eba_tbl, vol->reserved_pebs);
> -	} else {
> +	else
>   		ubi_eba_copy_table(vol, old_eba_tbl, reserved_pebs);
> -	}
>   	vol->eba_tbl = old_eba_tbl;
>   	spin_unlock(&ubi->volumes_lock);
>   out_free:
> 

Re: [PATCH 1/2] ubi: fix slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130

[Zhihao Cheng]

HI,
> From: Guo Xuenan <guoxuenan@huawei.com>
> 
> When using ioctl interface to resize ubi volume, ubi_resize_volume will
> resize eba table first, but not change vol->reserved_pebs in the same
> atomic context which may cause concurrency access eba table.
> 
> For example, When user do shrink ubi volume A calling ubi_resize_volume,
> while the other thread is writing (volume B) and triggering wear-leveling,
> which may calling ubi_write_fastmap, under these circumstances, KASAN may
> report: slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130.
> 
[...]
> diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
> index 2c867d16f89f..97294def01eb 100644
> --- a/drivers/mtd/ubi/vmt.c
> +++ b/drivers/mtd/ubi/vmt.c
> @@ -408,6 +408,7 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
>   	struct ubi_device *ubi = vol->ubi;
>   	struct ubi_vtbl_record vtbl_rec;
>   	struct ubi_eba_table *new_eba_tbl = NULL;
> +	struct ubi_eba_table *old_eba_tbl = NULL;
>   	int vol_id = vol->vol_id;
>   
>   	if (ubi->ro_mode)
> @@ -453,10 +454,13 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
>   			err = -ENOSPC;
>   			goto out_free;
>   		}
> +
>   		ubi->avail_pebs -= pebs;
>   		ubi->rsvd_pebs += pebs;
>   		ubi_eba_copy_table(vol, new_eba_tbl, vol->reserved_pebs);
> -		ubi_eba_replace_table(vol, new_eba_tbl);
> +		old_eba_tbl = vol->eba_tbl;
> +		vol->eba_tbl = new_eba_tbl;
> +		vol->reserved_pebs = reserved_pebs;
>   		spin_unlock(&ubi->volumes_lock);
>   	}
>   
> @@ -471,7 +475,9 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
>   		ubi->avail_pebs -= pebs;
>   		ubi_update_reserved(ubi);
>   		ubi_eba_copy_table(vol, new_eba_tbl, reserved_pebs);
> -		ubi_eba_replace_table(vol, new_eba_tbl);
> +		old_eba_tbl = vol->eba_tbl;
> +		vol->eba_tbl = new_eba_tbl;
> +		vol->reserved_pebs = reserved_pebs;
>   		spin_unlock(&ubi->volumes_lock);
>   	}
>   
> @@ -493,7 +499,6 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
>   	if (err)
>   		goto out_acc;
>   
> -	vol->reserved_pebs = reserved_pebs;
>   	if (vol->vol_type == UBI_DYNAMIC_VOLUME) {
>   		vol->used_ebs = reserved_pebs;
>   		vol->last_eb_bytes = vol->usable_leb_size;
> @@ -501,19 +506,24 @@ int ubi_resize_volume(struct ubi_volume_desc *desc, int reserved_pebs)
>   			(long long)vol->used_ebs * vol->usable_leb_size;
>   	}
>   
> +	/* destroy old table */
> +	ubi_eba_destroy_table(old_eba_tbl);
>   	ubi_volume_notify(ubi, vol, UBI_VOLUME_RESIZED);
>   	self_check_volumes(ubi);
>   	return err;
>   
>   out_acc:
> +	spin_lock(&ubi->volumes_lock);
> +	vol->reserved_pebs = reserved_pebs - pebs;
>   	if (pebs > 0) {
> -		spin_lock(&ubi->volumes_lock);
>   		ubi->rsvd_pebs -= pebs;
>   		ubi->avail_pebs += pebs;
> -		spin_unlock(&ubi->volumes_lock);
> +		ubi_eba_copy_table(vol, old_eba_tbl, vol->reserved_pebs);
> +	} else {
> +		ubi_eba_copy_table(vol, old_eba_tbl, reserved_pebs);
>   	}
> -	return err;
> -
> +	vol->eba_tbl = old_eba_tbl;
> +	spin_unlock(&ubi->volumes_lock);
>   out_free:
>   	ubi_eba_destroy_table(new_eba_tbl);
>   	return err;
> 


Besides that, it's better to protect 'vol->eba_tbl->entries' assignment 
like:
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index 403b79d6efd5..5ae0c1bc6f41 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -1450,7 +1450,9 @@ int ubi_eba_copy_leb(struct ubi_device *ubi, int 
from, int to,
         }

         ubi_assert(vol->eba_tbl->entries[lnum].pnum == from);
+       spin_lock(&ubi->volumes_lock);
         vol->eba_tbl->entries[lnum].pnum = to;
+       spin_unlock(&ubi->volumes_lock);

  out_unlock_buf:
         mutex_unlock(&ubi->buf_mutex);

Otherwise, a race between wear_leveling_work and shrinking volume could 
happen:

  ubi_resize_volume         wear_leveling_worker
   ubi_eba_copy_table(vol, new_eba_tbl, reserved_pebs);
                               vol->eba_tbl->entries[lnum].pnum = to; // 
update old eba_tbl
   vol->eba_tbl = new_eba_tbl

Re: [PATCH 1/2] ubi: fix slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130

[ZhaoLong Wang]

Yes, that could happen. I was able to reproduce the problem despite the
low probability of triggering it.

This race between wear_leveling_work() and ubi_resize_volume() can cause
data corruption in the UBIFS running on the UBI volume.. ubi->volumes_lock
must be added to protect the update of eba_tbl in the ubi_eba_copy_leb().

I'll do a V2 patch later to fix this issue.

With appreciation
ZhaoLong Wang

> HI,
>> From: Guo Xuenan <guoxuenan@huawei.com>
>>
>> When using ioctl interface to resize ubi volume, ubi_resize_volume will
>> resize eba table first, but not change vol->reserved_pebs in the same
>> atomic context which may cause concurrency access eba table.
>>
>> For example, When user do shrink ubi volume A calling ubi_resize_volume,
>> while the other thread is writing (volume B) and triggering 
>> wear-leveling,
>> which may calling ubi_write_fastmap, under these circumstances, KASAN 
>> may
>> report: slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130.
>>
> [...]
>> diff --git a/drivers/mtd/ubi/vmt.c b/drivers/mtd/ubi/vmt.c
>> index 2c867d16f89f..97294def01eb 100644
>> --- a/drivers/mtd/ubi/vmt.c
>> +++ b/drivers/mtd/ubi/vmt.c
>> @@ -408,6 +408,7 @@ int ubi_resize_volume(struct ubi_volume_desc 
>> *desc, int reserved_pebs)
>>       struct ubi_device *ubi = vol->ubi;
>>       struct ubi_vtbl_record vtbl_rec;
>>       struct ubi_eba_table *new_eba_tbl = NULL;
>> +    struct ubi_eba_table *old_eba_tbl = NULL;
>>       int vol_id = vol->vol_id;
>>         if (ubi->ro_mode)
>> @@ -453,10 +454,13 @@ int ubi_resize_volume(struct ubi_volume_desc 
>> *desc, int reserved_pebs)
>>               err = -ENOSPC;
>>               goto out_free;
>>           }
>> +
>>           ubi->avail_pebs -= pebs;
>>           ubi->rsvd_pebs += pebs;
>>           ubi_eba_copy_table(vol, new_eba_tbl, vol->reserved_pebs);
>> -        ubi_eba_replace_table(vol, new_eba_tbl);
>> +        old_eba_tbl = vol->eba_tbl;
>> +        vol->eba_tbl = new_eba_tbl;
>> +        vol->reserved_pebs = reserved_pebs;
>>           spin_unlock(&ubi->volumes_lock);
>>       }
>>   @@ -471,7 +475,9 @@ int ubi_resize_volume(struct ubi_volume_desc 
>> *desc, int reserved_pebs)
>>           ubi->avail_pebs -= pebs;
>>           ubi_update_reserved(ubi);
>>           ubi_eba_copy_table(vol, new_eba_tbl, reserved_pebs);
>> -        ubi_eba_replace_table(vol, new_eba_tbl);
>> +        old_eba_tbl = vol->eba_tbl;
>> +        vol->eba_tbl = new_eba_tbl;
>> +        vol->reserved_pebs = reserved_pebs;
>>           spin_unlock(&ubi->volumes_lock);
>>       }
>>   @@ -493,7 +499,6 @@ int ubi_resize_volume(struct ubi_volume_desc 
>> *desc, int reserved_pebs)
>>       if (err)
>>           goto out_acc;
>>   -    vol->reserved_pebs = reserved_pebs;
>>       if (vol->vol_type == UBI_DYNAMIC_VOLUME) {
>>           vol->used_ebs = reserved_pebs;
>>           vol->last_eb_bytes = vol->usable_leb_size;
>> @@ -501,19 +506,24 @@ int ubi_resize_volume(struct ubi_volume_desc 
>> *desc, int reserved_pebs)
>>               (long long)vol->used_ebs * vol->usable_leb_size;
>>       }
>>   +    /* destroy old table */
>> +    ubi_eba_destroy_table(old_eba_tbl);
>>       ubi_volume_notify(ubi, vol, UBI_VOLUME_RESIZED);
>>       self_check_volumes(ubi);
>>       return err;
>>     out_acc:
>> +    spin_lock(&ubi->volumes_lock);
>> +    vol->reserved_pebs = reserved_pebs - pebs;
>>       if (pebs > 0) {
>> -        spin_lock(&ubi->volumes_lock);
>>           ubi->rsvd_pebs -= pebs;
>>           ubi->avail_pebs += pebs;
>> -        spin_unlock(&ubi->volumes_lock);
>> +        ubi_eba_copy_table(vol, old_eba_tbl, vol->reserved_pebs);
>> +    } else {
>> +        ubi_eba_copy_table(vol, old_eba_tbl, reserved_pebs);
>>       }
>> -    return err;
>> -
>> +    vol->eba_tbl = old_eba_tbl;
>> +    spin_unlock(&ubi->volumes_lock);
>>   out_free:
>>       ubi_eba_destroy_table(new_eba_tbl);
>>       return err;
>>
>
>
> Besides that, it's better to protect 'vol->eba_tbl->entries' 
> assignment like:
> diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
> index 403b79d6efd5..5ae0c1bc6f41 100644
> --- a/drivers/mtd/ubi/eba.c
> +++ b/drivers/mtd/ubi/eba.c
> @@ -1450,7 +1450,9 @@ int ubi_eba_copy_leb(struct ubi_device *ubi, int 
> from, int to,
>         }
>
>         ubi_assert(vol->eba_tbl->entries[lnum].pnum == from);
> +       spin_lock(&ubi->volumes_lock);
>         vol->eba_tbl->entries[lnum].pnum = to;
> +       spin_unlock(&ubi->volumes_lock);
>
>  out_unlock_buf:
>         mutex_unlock(&ubi->buf_mutex);
>
> Otherwise, a race between wear_leveling_work and shrinking volume 
> could happen:
>
>  ubi_resize_volume         wear_leveling_worker
>   ubi_eba_copy_table(vol, new_eba_tbl, reserved_pebs);
> vol->eba_tbl->entries[lnum].pnum = to; // update old eba_tbl
>   vol->eba_tbl = new_eba_tbl

Re: [PATCH 0/2] Fix some bugs in ubi_resize_volume() function

[ZhaoLong Wang]

Please don't forget to discuss this issue.

V2
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=353394

Friendly ping.

Thanks
Zhaolong

> This series of patches does two things:
>    1. Rebase the patch commited by Guo Xuenan on the latest mainline.
>    2. Fixed an issue where the number of PEBs is incorrectly displayed.
> 
> Guo Xuenan (1):
>    ubi: fix slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130
> 
> ZhaoLong Wang (1):
>    ubi: Correct the number of PEBs after a volume resize failure
> 
>   drivers/mtd/ubi/vmt.c | 31 ++++++++++++++++++++-----------
>   1 file changed, 20 insertions(+), 11 deletions(-)
> 

Re: [PATCH 0/2] Fix some bugs in ubi_resize_volume() function

[ZhaoLong Wang]

Friendly ping.



> Please don't forget to discuss this issue.
> 
> V2
> https://patchwork.ozlabs.org/project/linux-mtd/list/?series=353394
> 
> Friendly ping.
> 
> Thanks
> Zhaolong
> 
>> This series of patches does two things:
>>    1. Rebase the patch commited by Guo Xuenan on the latest mainline.
>>    2. Fixed an issue where the number of PEBs is incorrectly displayed.
>>
>> Guo Xuenan (1):
>>    ubi: fix slab-out-of-bounds in ubi_eba_get_ldesc+0xfb/0x130
>>
>> ZhaoLong Wang (1):
>>    ubi: Correct the number of PEBs after a volume resize failure
>>
>>   drivers/mtd/ubi/vmt.c | 31 ++++++++++++++++++++-----------
>>   1 file changed, 20 insertions(+), 11 deletions(-)
>>
>