kernel oops when rmmod'ing ublk_drv w/ missing UBLK_CMD_START_DEV

kernel oops when rmmod'ing ublk_drv w/ missing UBLK_CMD_START_DEV

[Harris, James R]

Hi,

I can reliably hit a kernel oops with ublk_drv using the following abnormal sequence of events (repro .c file at end of this e-mail):

1) modprobe ublk_drv
2) run test app which basically does:
  a) submit UBLK_CMD_ADD_DEV
  b) submit UBLK_CMD_SET_PARAMS
  c) wait for completions
  d) do *not* submit UBLK_CMD_START_DEV
  e) exit
3) rmmod ublk_drv

Reproduces on 6.2-rc5, 6.1.5 and 6.1.

Regards,

Jim Harris


[  859.178944] ------------[ cut here ]------------
[  859.178950] sysfs group 'power' not found for kobject 'ublkc0'
[  859.178962] WARNING: CPU: 3 PID: 1109 at fs/sysfs/group.c:278 sysfs_remove_group+0x9c/0xb0
[  859.178980] Modules linked in: ublk_drv(-) nvme_fabrics nvme_core binfmt_misc nls_iso8859_1 uio_pdrv_genirq uio ramoops reed_solomon pstore_blk pstore_zone efi_pstore autofs4 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor xor_neon raid6_pq libcrc32c raid1 raid0 linear virtio_gpu virtio_dma_buf drm_shmem_helper drm_kms_helper syscopyarea sysfillrect sysimgblt usbhid hid drm crct10dif_ce ghash_ce sha3_ce virtio_net sha3_generic sha512_ce sha512_arm64 sha2_ce sha256_arm64 ahci_platform net_failover sha1_ce libahci_platform failover xhci_pci backlight libahci xhci_pci_renesas aes_neon_bs aes_neon_blk aes_ce_blk aes_ce_cipher
[  859.179044] CPU: 3 PID: 1109 Comm: rmmod Not tainted 6.2.0-rc5 #33
[  859.179048] Hardware name: Parallels International GmbH. Parallels ARM Virtual Machine/Parallels ARM Virtual Platform, BIOS 17.1.6 (51584) Thu, 01 Dec 202
[  859.179051] pstate: 61400005 (nZCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
[  859.179055] pc : sysfs_remove_group+0x9c/0xb0
[  859.179060] lr : sysfs_remove_group+0x9c/0xb0
[  859.179066] sp : ffff80000b9c3c20
[  859.179067] x29: ffff80000b9c3c20 x28: ffff0000c08f4100 x27: 0000000000000000
[  859.179072] x26: 0000000000000000 x25: 0000000000000000 x24: 0000000000000000
[  859.179075] x23: ffff0000c0263800 x22: 0000000000000000 x21: ffff0000ccfae170
[  859.179078] x20: ffff800008fa5648 x19: 0000000000000000 x18: 0000000000000000
[  859.179080] x17: 5453595342555300 x16: ffff8000084dc2c0 x15: 6b6c62752f637369
[  859.179083] x14: 0000000000000001 x13: 2730636b6c627527 x12: 207463656a626f6b
[  859.179086] x11: 20726f6620646e75 x10: 6f6620746f6e2027 x9 : ffff800008209b5c
[  859.179089] x8 : 20746f6e20277265 x7 : 0000000000000001 x6 : 0000000000000001
[  859.179092] x5 : ffff0001feb9eb48 x4 : ffff80000b9c3a60 x3 : ffff8001f525f000
[  859.179095] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000c08f4100
[  859.179098] Call trace:
[  859.179099]  sysfs_remove_group+0x9c/0xb0
[  859.179105]  dpm_sysfs_remove+0x64/0xb0
[  859.179113]  device_del+0xac/0x3a0
[  859.179119]  cdev_device_del+0x28/0x70
[  859.179125]  ublk_exit+0x84/0xe90 [ublk_drv]
[  859.179137]  __arm64_sys_delete_module+0x180/0x31c
[  859.179143]  invoke_syscall+0x78/0x100
[  859.179151]  el0_svc_common.constprop.0+0x54/0x190
[  859.179157]  do_el0_svc+0x44/0xd0
[  859.179162]  el0_svc+0x2c/0xb4
[  859.179169]  el0t_64_sync_handler+0xbc/0x13c
[  859.179174]  el0t_64_sync+0x1a4/0x1a8
[  859.179178] ---[ end trace 0000000000000000 ]---
[  859.179304] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000070
[  859.179347] Mem abort info:
[  859.179362]   ESR = 0x0000000096000004
[  859.179380]   EC = 0x25: DABT (current EL), IL = 32 bits
[  859.179406]   SET = 0, FnV = 0
[  859.179418]   EA = 0, S1PTW = 0
[  859.179431]   FSC = 0x04: level 0 translation fault
[  859.179450] Data abort info:
[  859.179462]   ISV = 0, ISS = 0x00000004
[  859.179478]   CM = 0, WnR = 0
[  859.179491] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000103cbd000
[  859.179508] [0000000000000070] pgd=0000000000000000, p4d=0000000000000000
[  859.179532] Internal error: Oops: 0000000096000004 [#1] SMP
[  859.179551] Modules linked in: ublk_drv(-) nvme_fabrics nvme_core binfmt_misc nls_iso8859_1 uio_pdrv_genirq uio ramoops reed_solomon pstore_blk pstore_zone efi_pstore autofs4 btrfs blake2b_generic raid10 raid456 async_raid6_recov async_memcpy async_pq async_xor async_tx xor xor_neon raid6_pq libcrc32c raid1 raid0 linear virtio_gpu virtio_dma_buf drm_shmem_helper drm_kms_helper syscopyarea sysfillrect sysimgblt usbhid hid drm crct10dif_ce ghash_ce sha3_ce virtio_net sha3_generic sha512_ce sha512_arm64 sha2_ce sha256_arm64 ahci_platform net_failover sha1_ce libahci_platform failover xhci_pci backlight libahci xhci_pci_renesas aes_neon_bs aes_neon_blk aes_ce_blk aes_ce_cipher
[  859.179694] CPU: 3 PID: 1109 Comm: rmmod Tainted: G        W          6.2.0-rc5 #33
[  859.179714] Hardware name: Parallels International GmbH. Parallels ARM Virtual Machine/Parallels ARM Virtual Platform, BIOS 17.1.6 (51584) Thu, 01 Dec 202
[  859.179744] pstate: 81400005 (Nzcv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
[  859.179760] pc : sysfs_delete_link+0x58/0xe0
[  859.179789] lr : device_remove_class_symlinks+0x94/0xb0
[  859.179817] sp : ffff80000b9c3c20
[  859.179833] x29: ffff80000b9c3c20 x28: ffff0000c08f4100 x27: 0000000000000000
[  859.179851] x26: 0000000000000000 x25: 0000000000000000 x24: ffff80000a1e9a78
[  859.179866] x23: ffff0000c0263800 x22: ffff000191691b00 x21: ffff0000ccfae170
[  859.179881] x20: ffff80000a1b9000 x19: ffff0000c373c018 x18: 0000000000000000
[  859.179897] x17: 5453595342555300 x16: ffff8000084dc2c0 x15: ffff800009e151f8
[  859.179912] x14: 0000000000000000 x13: 0000000000000003 x12: ffff800009dddbb0
[  859.179928] x11: 0000000000000000 x10: ffff800185ccd9c7 x9 : ffff800008aa2164
[  859.179944] x8 : 0101010101010101 x7 : 7f7f7f7f7f7f7f7f x6 : fefefefefefeff6c
[  859.179959] x5 : 8080808080808000 x4 : 0000000000000000 x3 : ffff8000091e4a79
[  859.179975] x2 : ffff0000c49e9280 x1 : 0000000000000000 x0 : 0000000000000000
[  859.179995] Call trace:
[  859.180007]  sysfs_delete_link+0x58/0xe0
[  859.180031]  device_remove_class_symlinks+0x94/0xb0
[  859.180053]  device_del+0xe8/0x3a0
[  859.180072]  cdev_device_del+0x28/0x70
[  859.180093]  ublk_exit+0x84/0xe90 [ublk_drv]
[  859.180120]  __arm64_sys_delete_module+0x180/0x31c
[  859.180144]  invoke_syscall+0x78/0x100
[  859.180169]  el0_svc_common.constprop.0+0x54/0x190
[  859.180647]  do_el0_svc+0x44/0xd0
[  859.181085]  el0_svc+0x2c/0xb4
[  859.181489]  el0t_64_sync_handler+0xbc/0x13c
[  859.181882]  el0t_64_sync+0x1a4/0x1a8
[  859.182301] Code: 350003a0 f9401aa2 b40000a2 f9401a60 (7940e000)
[  859.182777] ---[ end trace 0000000000000000 ]—

===

#include <linux/ublk_cmd.h>
#include <liburing.h>
#include <stdlib.h>
#include <assert.h>

#define UBLK_CTRL_DEV                   "/dev/ublk-control"
#define UBLK_CTRL_RING_DEPTH            32
#define UBLK_IO_MAX_BYTES               65536
#define UBLK_ID                         0

struct ublk_dev;
static void ublk_ctrl_cmd(uint32_t cmd_op);

static int g_fd;
static struct io_uring g_ring;
static uint32_t g_count;

static struct ublksrv_ctrl_dev_info g_dev_info = {
        .queue_depth = 32,
        .nr_hw_queues = 1,
        .dev_id = UBLK_ID,
        .max_io_buf_bytes = UBLK_IO_MAX_BYTES,
        .flags = UBLK_F_URING_CMD_COMP_IN_TASK,
};

static struct ublk_params g_params = {
        .types = UBLK_PARAM_TYPE_BASIC,
        .len = sizeof(g_params),
        .basic = {
                .logical_bs_shift = 9,
                .physical_bs_shift = 9,
                .io_min_shift = 9,
                .io_opt_shift = 12,
                .dev_sectors = 2048,
                .max_sectors = UBLK_IO_MAX_BYTES >> 9,
        }
};

static void
ublk_ctrl_poller(void)
{
        struct io_uring_cqe *cqe;
        int rc;

        while (1) {
                rc = io_uring_peek_cqe(&g_ring, &cqe);
                if (rc == -EAGAIN) {
                        break;
                }
                assert(cqe != NULL);
                io_uring_cqe_seen(&g_ring, cqe);
                g_count++;
        }
}

static void
ublk_ctrl_cmd(uint32_t cmd_op)
{
        int rc = -EINVAL;
        struct io_uring_sqe *sqe;
        struct ublksrv_ctrl_cmd *cmd;

        sqe = io_uring_get_sqe(&g_ring);
        assert(sqe != NULL);
        cmd = (struct ublksrv_ctrl_cmd *)&sqe->addr3;
        sqe->fd = g_fd;
        sqe->opcode = IORING_OP_URING_CMD;
        sqe->off = cmd_op;
        sqe->ioprio = 0;
        cmd->dev_id = UBLK_ID;
        cmd->queue_id = -1;

        switch (cmd_op) {
        case UBLK_CMD_ADD_DEV:
                cmd->addr = (__u64)(uintptr_t)&g_dev_info;
                cmd->len = sizeof(g_dev_info);
                break;
        case UBLK_CMD_SET_PARAMS:
                cmd->addr = (__u64)(uintptr_t)&g_params;
                cmd->len = sizeof(g_params);
                break;
        default:
                assert(false);
                break;
        }
        rc = io_uring_submit(&g_ring);
        assert(rc > 0);
}

int main(int argc, char **argv)
{
        struct io_uring_params p = {};
        int rc;

        g_fd = open(UBLK_CTRL_DEV, O_RDWR);
        assert(g_fd >= 0);
        p.flags = IORING_SETUP_SQE128 | IORING_SETUP_SQPOLL | IORING_SETUP_CQSIZE;
        p.cq_entries = UBLK_CTRL_RING_DEPTH;
        rc = io_uring_queue_init_params(UBLK_CTRL_RING_DEPTH, &g_ring, &p);
        assert(rc == 0);
        g_dev_info.ublksrv_pid = getpid();
        ublk_ctrl_cmd(UBLK_CMD_ADD_DEV);
        ublk_ctrl_cmd(UBLK_CMD_SET_PARAMS);
        while (g_count < 2) {
                ublk_ctrl_poller();
        }
        return 0;
}

Re: kernel oops when rmmod'ing ublk_drv w/ missing UBLK_CMD_START_DEV

[Jens Axboe]

On 1/25/23 12:50?PM, Harris, James R wrote:
> Hi,
> 
> I can reliably hit a kernel oops with ublk_drv using the following abnormal sequence of events (repro .c file at end of this e-mail):
> 
> 1) modprobe ublk_drv
> 2) run test app which basically does:
>   a) submit UBLK_CMD_ADD_DEV
>   b) submit UBLK_CMD_SET_PARAMS
>   c) wait for completions
>   d) do *not* submit UBLK_CMD_START_DEV
>   e) exit
> 3) rmmod ublk_drv
> 
> Reproduces on 6.2-rc5, 6.1.5 and 6.1.

Something like this may do it, but I'll let Ming sort out the details
on what's the most appropriate fix.


diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 17b677b5d3b2..dacc13e2a476 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -1149,11 +1149,17 @@ static void ublk_unquiesce_dev(struct ublk_device *ub)
 	blk_mq_kick_requeue_list(ub->ub_disk->queue);
 }
 
-static void ublk_stop_dev(struct ublk_device *ub)
+/*
+ * Returns if the device was live or not
+ */
+static bool ublk_stop_dev(struct ublk_device *ub)
 {
+	bool was_live = false;
+
 	mutex_lock(&ub->mutex);
 	if (ub->dev_info.state == UBLK_S_DEV_DEAD)
 		goto unlock;
+	was_live = true;
 	if (ublk_can_use_recovery(ub)) {
 		if (ub->dev_info.state == UBLK_S_DEV_LIVE)
 			__ublk_quiesce_dev(ub);
@@ -1168,6 +1174,7 @@ static void ublk_stop_dev(struct ublk_device *ub)
 	ublk_cancel_dev(ub);
 	mutex_unlock(&ub->mutex);
 	cancel_delayed_work_sync(&ub->monitor_work);
+	return was_live;
 }
 
 /* device can only be started after all IOs are ready */
@@ -1470,7 +1477,8 @@ static int ublk_add_tag_set(struct ublk_device *ub)
 
 static void ublk_remove(struct ublk_device *ub)
 {
-	ublk_stop_dev(ub);
+	if (!ublk_stop_dev(ub))
+		return;
 	cancel_work_sync(&ub->stop_work);
 	cancel_work_sync(&ub->quiesce_work);
 	cdev_device_del(&ub->cdev, &ub->cdev_dev);
@@ -1771,7 +1779,8 @@ static int ublk_ctrl_stop_dev(struct io_uring_cmd *cmd)
 	if (!ub)
 		return -EINVAL;
 
-	ublk_stop_dev(ub);
+	if (!ublk_stop_dev(ub))
+		return -EINVAL;
 	cancel_work_sync(&ub->stop_work);
 	cancel_work_sync(&ub->quiesce_work);
 

-- 
Jens Axboe

Re: kernel oops when rmmod'ing ublk_drv w/ missing UBLK_CMD_START_DEV

[Harris, James R]

> On Jan 25, 2023, at 1:05 PM, Jens Axboe <axboe@kernel.dk> wrote:
> 
> On 1/25/23 12:50?PM, Harris, James R wrote:
>> Hi,
>> 
>> I can reliably hit a kernel oops with ublk_drv using the following abnormal sequence of events (repro .c file at end of this e-mail):
>> 
>> 1) modprobe ublk_drv
>> 2) run test app which basically does:
>>  a) submit UBLK_CMD_ADD_DEV
>>  b) submit UBLK_CMD_SET_PARAMS
>>  c) wait for completions
>>  d) do *not* submit UBLK_CMD_START_DEV
>>  e) exit
>> 3) rmmod ublk_drv
>> 
>> Reproduces on 6.2-rc5, 6.1.5 and 6.1.
> 
> Something like this may do it, but I'll let Ming sort out the details
> on what's the most appropriate fix.

Thanks Jens.  This patch fixes things for me.  If you and Ming decide to go forward
with this patch, feel free to add:

Tested-by: Jim Harris <james.r.harris@intel.com>

> 
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 17b677b5d3b2..dacc13e2a476 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1149,11 +1149,17 @@ static void ublk_unquiesce_dev(struct ublk_device *ub)
> 	blk_mq_kick_requeue_list(ub->ub_disk->queue);
> }
> 
> -static void ublk_stop_dev(struct ublk_device *ub)
> +/*
> + * Returns if the device was live or not
> + */
> +static bool ublk_stop_dev(struct ublk_device *ub)
> {
> +	bool was_live = false;
> +
> 	mutex_lock(&ub->mutex);
> 	if (ub->dev_info.state == UBLK_S_DEV_DEAD)
> 		goto unlock;
> +	was_live = true;
> 	if (ublk_can_use_recovery(ub)) {
> 		if (ub->dev_info.state == UBLK_S_DEV_LIVE)
> 			__ublk_quiesce_dev(ub);
> @@ -1168,6 +1174,7 @@ static void ublk_stop_dev(struct ublk_device *ub)
> 	ublk_cancel_dev(ub);
> 	mutex_unlock(&ub->mutex);
> 	cancel_delayed_work_sync(&ub->monitor_work);
> +	return was_live;
> }
> 
> /* device can only be started after all IOs are ready */
> @@ -1470,7 +1477,8 @@ static int ublk_add_tag_set(struct ublk_device *ub)
> 
> static void ublk_remove(struct ublk_device *ub)
> {
> -	ublk_stop_dev(ub);
> +	if (!ublk_stop_dev(ub))
> +		return;
> 	cancel_work_sync(&ub->stop_work);
> 	cancel_work_sync(&ub->quiesce_work);
> 	cdev_device_del(&ub->cdev, &ub->cdev_dev);
> @@ -1771,7 +1779,8 @@ static int ublk_ctrl_stop_dev(struct io_uring_cmd *cmd)
> 	if (!ub)
> 		return -EINVAL;
> 
> -	ublk_stop_dev(ub);
> +	if (!ublk_stop_dev(ub))
> +		return -EINVAL;
> 	cancel_work_sync(&ub->stop_work);
> 	cancel_work_sync(&ub->quiesce_work);
> 
> 
> -- 
> Jens Axboe
> 

Re: kernel oops when rmmod'ing ublk_drv w/ missing UBLK_CMD_START_DEV

[Ming Lei]

On Wed, Jan 25, 2023 at 01:05:55PM -0700, Jens Axboe wrote:
> On 1/25/23 12:50?PM, Harris, James R wrote:
> > Hi,
> > 
> > I can reliably hit a kernel oops with ublk_drv using the following abnormal sequence of events (repro .c file at end of this e-mail):
> > 
> > 1) modprobe ublk_drv
> > 2) run test app which basically does:
> >   a) submit UBLK_CMD_ADD_DEV
> >   b) submit UBLK_CMD_SET_PARAMS
> >   c) wait for completions
> >   d) do *not* submit UBLK_CMD_START_DEV
> >   e) exit
> > 3) rmmod ublk_drv
> > 
> > Reproduces on 6.2-rc5, 6.1.5 and 6.1.
> 
> Something like this may do it, but I'll let Ming sort out the details
> on what's the most appropriate fix.

Hi Jens and James,

The ublk char device still needs to be deleted even though START_DEV
isn't called given the char device is added in ADD_DEV's handler, so
the current logic is correct.

The root cause is that 'ublk_chr_class' is destroyed too early, so
the following patch should fix the issue.

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 17b677b5d3b2..e54693204630 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -2092,13 +2092,12 @@ static void __exit ublk_exit(void)
        struct ublk_device *ub;
        int id;

-       class_destroy(ublk_chr_class);
-
-       misc_deregister(&ublk_misc);
-
        idr_for_each_entry(&ublk_index_idr, ub, id)
                ublk_remove(ub);

+       class_destroy(ublk_chr_class);
+       misc_deregister(&ublk_misc);
+
        idr_destroy(&ublk_index_idr);
        unregister_chrdev_region(ublk_chr_devt, UBLK_MINORS);
 }


Thanks, 
Ming