LinuxLists.cc - [PATCH -next 0/8] iocost bugfix

2022-11-28 16:14:50

Subject: [PATCH -next 0/8] iocost bugfix

This patchset fixes problem in iocost. It contains Yu Kuai's patchset he
sent before because of conflicts.

Li Nan (4):
blk-iocost: fix divide by 0 error in calc_lcoefs()
blk-iocost: change div64_u64 to DIV64_U64_ROUND_UP in
ioc_refresh_params()
blk-iocost: fix possible UAF in ioc_pd_free
block: fix null-pointer dereference in ioc_pd_init

Yu Kuai (4):
blk-iocost: cleanup ioc_qos_write() and ioc_cost_model_write()
blk-iocost: improve hanlder of match_u64()
blk-iocost: don't allow to configure bio based device
blk-iocost: read params inside lock in sysfs apis

block/blk-iocost.c | 114 ++++++++++++++++++++++++++++-----------------
block/genhd.c | 2 +-
2 files changed, 73 insertions(+), 43 deletions(-)

--
2.31.1

2022-11-28 16:16:16

by Li Nan

[permalink] [raw]

Subject: [PATCH -next 3/8] blk-iocost: don't allow to configure bio based device

From: Yu Kuai <[email protected]>

iocost is based on rq_qos, which can only work for request based device,
thus it doesn't make sense to configure iocost for bio based device.

Signed-off-by: Yu Kuai <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Signed-off-by: Li Nan <[email protected]>
---
block/blk-iocost.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index c532129a1456..2bfecc511dd9 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -3181,6 +3181,11 @@ static ssize_t ioc_qos_write(struct kernfs_open_file *of, char *input,
return PTR_ERR(bdev);

disk = bdev->bd_disk;
+ if (!queue_is_mq(disk->queue)) {
+ ret = -EPERM;
+ goto out;
+ }
+
ioc = q_to_ioc(disk->queue);
if (!ioc) {
ret = blk_iocost_init(disk);
@@ -3364,6 +3369,11 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
return PTR_ERR(bdev);

q = bdev_get_queue(bdev);
+ if (!queue_is_mq(q)) {
+ ret = -EPERM;
+ goto out;
+ }
+
ioc = q_to_ioc(q);
if (!ioc) {
ret = blk_iocost_init(bdev->bd_disk);
--
2.31.1

2022-11-28 16:43:09

by Li Nan

[permalink] [raw]

Subject: [PATCH -next 8/8] block: fix null-pointer dereference in ioc_pd_init

Remove block device when iocost is initializing may cause
null-pointer dereference:

CPU1 CPU2
ioc_qos_write
blkcg_conf_open_bdev
blkdev_get_no_open
kobject_get_unless_zero
blk_iocost_init
rq_qos_add
del_gendisk
rq_qos_exit
q->rq_qos = rqos->next
//iocost is removed from q->roqs
blkcg_activate_policy
pd_init_fn
ioc_pd_init
ioc = q_to_ioc(blkg->q)
//cant find iocost and return null

Fix problem by moving rq_qos_exit() to disk_release(). ioc_qos_write() get
bd_device.kobj in blkcg_conf_open_bdev(), so disk_release will not be
actived until iocost initialization is complited.

Signed-off-by: Li Nan <[email protected]>
---
block/genhd.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/genhd.c b/block/genhd.c
index dcf200bcbd3e..c264da49eaaa 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -656,7 +656,6 @@ void del_gendisk(struct gendisk *disk)
elevator_exit(q);
mutex_unlock(&q->sysfs_lock);
}
- rq_qos_exit(q);
blk_mq_unquiesce_queue(q);

/*
@@ -1168,6 +1167,7 @@ static void disk_release(struct device *dev)
!test_bit(GD_ADDED, &disk->state))
blk_mq_exit_queue(disk->queue);

+ rq_qos_exit(q);
blkcg_exit_disk(disk);

bioset_exit(&disk->bio_split);
--
2.31.1

2022-11-29 11:55:12

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH -next 8/8] block: fix null-pointer dereference in ioc_pd_init

Attachments:

(No filename) (3.67 kB)
config (178.42 kB)
Download all attachments

2022-11-29 14:36:47

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH -next 8/8] block: fix null-pointer dereference in ioc_pd_init

On Mon, Nov 28, 2022 at 11:44:34PM +0800, Li Nan wrote:
> Fix problem by moving rq_qos_exit() to disk_release().

No, that now means it is removed to later. You need to add proper
synchronization.

2022-11-30 01:32:53

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH -next 8/8] block: fix null-pointer dereference in ioc_pd_init

Hi Li,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20221128]

url: https://github.com/intel-lab-lkp/linux/commits/Li-Nan/iocost-bugfix/20221128-232536
patch link: https://lore.kernel.org/r/20221128154434.4177442-9-linan122%40huawei.com
patch subject: [PATCH -next 8/8] block: fix null-pointer dereference in ioc_pd_init
config: s390-randconfig-r044-20221128
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 6e4cea55f0d1104408b26ac574566a0e4de48036)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/44da423d235b6c23ecf6a38325d0429cfac20ee2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Li-Nan/iocost-bugfix/20221128-232536
git checkout 44da423d235b6c23ecf6a38325d0429cfac20ee2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

In file included from block/genhd.c:28:
In file included from block/blk-throttle.h:4:
In file included from block/blk-cgroup-rwstat.h:9:
In file included from block/blk-cgroup.h:20:
In file included from include/linux/blk-mq.h:8:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from block/genhd.c:28:
In file included from block/blk-throttle.h:4:
In file included from block/blk-cgroup-rwstat.h:9:
In file included from block/blk-cgroup.h:20:
In file included from include/linux/blk-mq.h:8:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from block/genhd.c:28:
In file included from block/blk-throttle.h:4:
In file included from block/blk-cgroup-rwstat.h:9:
In file included from block/blk-cgroup.h:20:
In file included from include/linux/blk-mq.h:8:
In file included from include/linux/scatterlist.h:9:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:692:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:700:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:708:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:717:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:726:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:735:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
>> block/genhd.c:1170:14: error: use of undeclared identifier 'q'
rq_qos_exit(q);
^
12 warnings and 1 error generated.

vim +/q +1170 block/genhd.c

1136
1137 /**
1138 * disk_release - releases all allocated resources of the gendisk
1139 * @dev: the device representing this disk
1140 *
1141 * This function releases all allocated resources of the gendisk.
1142 *
1143 * Drivers which used __device_add_disk() have a gendisk with a request_queue
1144 * assigned. Since the request_queue sits on top of the gendisk for these
1145 * drivers we also call blk_put_queue() for them, and we expect the
1146 * request_queue refcount to reach 0 at this point, and so the request_queue
1147 * will also be freed prior to the disk.
1148 *
1149 * Context: can sleep
1150 */
1151 static void disk_release(struct device *dev)
1152 {
1153 struct gendisk *disk = dev_to_disk(dev);
1154
1155 might_sleep();
1156 WARN_ON_ONCE(disk_live(disk));
1157
1158 /*
1159 * To undo the all initialization from blk_mq_init_allocated_queue in
1160 * case of a probe failure where add_disk is never called we have to
1161 * call blk_mq_exit_queue here. We can't do this for the more common
1162 * teardown case (yet) as the tagset can be gone by the time the disk
1163 * is released once it was added.
1164 */
1165 if (queue_is_mq(disk->queue) &&
1166 test_bit(GD_OWNS_QUEUE, &disk->state) &&
1167 !test_bit(GD_ADDED, &disk->state))
1168 blk_mq_exit_queue(disk->queue);
1169
> 1170 rq_qos_exit(q);
1171 blkcg_exit_disk(disk);
1172
1173 bioset_exit(&disk->bio_split);
1174
1175 disk_release_events(disk);
1176 kfree(disk->random);
1177 disk_free_zone_bitmaps(disk);
1178 xa_destroy(&disk->part_tbl);
1179
1180 disk->queue->disk = NULL;
1181 blk_put_queue(disk->queue);
1182
1183 if (test_bit(GD_ADDED, &disk->state) && disk->fops->free_disk)
1184 disk->fops->free_disk(disk);
1185
1186 iput(disk->part0->bd_inode); /* frees the disk */
1187 }
1188

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

(No filename) (8.52 kB)
config (124.44 kB)
Download all attachments

2022-11-30 02:06:01

by Yu Kuai

[permalink] [raw]

Subject: Re: [PATCH -next 8/8] block: fix null-pointer dereference in ioc_pd_init

Hi,

?? 2022/11/29 22:25, Christoph Hellwig д??:
> On Mon, Nov 28, 2022 at 11:44:34PM +0800, Li Nan wrote:
>> Fix problem by moving rq_qos_exit() to disk_release().
>
> No, that now means it is removed to later. You need to add proper
> synchronization.
> .
>

Can you explain a bit more? Maybe I'm being noob, here disk is about to
be freed, and I can think of any contention.

Thanks,
Kuai

2022-11-30 17:24:53

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH -next 8/8] block: fix null-pointer dereference in ioc_pd_init

On Wed, Nov 30, 2022 at 09:32:58AM +0800, Yu Kuai wrote:
> > No, that now means it is removed to later. You need to add proper
> > synchronization.
> > .
> >
>
> Can you explain a bit more? Maybe I'm being noob, here disk is about to
> be freed, and I can think of any contention.

Right now we need synchronization with e.g. open_mutex and a check
for a dead disk, which I suggst to add insted of creating a lifetime
imbalance.