Re: [RFC] How to fix an async scan - rmmod race?

[Mike Christie <michaelc@cs.wisc.edu>]

On 04/05/2012 08:58 AM, Tomas Henzl wrote:
> When a rmmod is tried then in some cases the kernel is not able to handle a paging request:
> [  727.154296] BUG: unable to handle kernel paging request at ffffffffa01874b8
> [  727.161473] IP: [<ffffffff8141d1e1>] do_scsi_scan_host+0x61/0xa0
> [  727.167637] PGD 1c07067 PUD 1c0b063 PMD 178d9ed067 PTE 0
> [  727.173369] Oops: 0000 [#1] SMP
> [  727.176808] CPU 0
> [  727.178691] Modules linked in: ses enclosure mlx4_ib mlx4_en microcode serio_raw joydev i2c_i801 iTCO_wdt i2c_core iTCO_vendor_support ioatdma mlx4_core scsi_transport_sas igb raid_class i7core_edac dca edac_core ib_ipoib ib_cm ib_addr ib_sa ib_uverbs ib_umad ib_mad ib_core ipmi_poweroff ipmi_watchdog ipmi_devintf ipmi_si ipmi_msghandler [last unloaded: mpt2sas]
> [  727.213474]
> [  727.215065] Pid: 2439, comm: scsi_scan_6 Tainted: G        W    3.2.3-2.fc16.x86_64.debug #1 Supermicro X8DTH-i/6/iF/6F/X8DTH
> [  727.226690] RIP: 0010:[<ffffffff8141d1e1>]  [<ffffffff8141d1e1>] do_scsi_scan_host+0x61/0xa0
> [  727.235334] RSP: 0018:ffff88178b2abe40  EFLAGS: 00010246
> [  727.240736] RAX: ffffffffa0187420 RBX: ffff881775f74290 RCX: 0000000000000001
> [  727.247962] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000282
> [  727.255188] RBP: ffff88178b2abe50 R08: 0000000000000000 R09: 0000000000000001
> [  727.262413] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000100067e40
> [  727.269638] R13: ffffffff8141d220 R14: ffff88178c3d28f8 R15: 0000000000000000
> [  727.276857] FS:  0000000000000000(0000) GS:ffff8817de600000(0000) knlGS:0000000000000000
> [  727.285094] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> [  727.290936] CR2: ffffffffa01874b8 CR3: 0000000001c05000 CR4: 00000000000006f0
> [  727.298163] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [  727.305388] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [  727.312607] Process scsi_scan_6 (pid: 2439, threadinfo ffff88178b2aa000, task ffff88178a552450)
> [  727.321439] Stack:
> [  727.323548]  ffff881775f74290 ffff88178c3d28f8 ffff88178b2abe90 ffffffff8141d245
> [  727.331336]  ffff88178c3d28f8 ffff8817745cbcf8 ffff88178c3d28f8 ffffffff8141d220
> [  727.339131]  0000000000000000 0000000000000000 ffff88178b2abf40 ffffffff810a33e0
> [  727.346928] Call Trace:
> [  727.349472]  [<ffffffff8141d245>] do_scan_async+0x25/0x160
> [  727.355049]  [<ffffffff8141d220>] ? do_scsi_scan_host+0xa0/0xa0
> [  727.362260]  [<ffffffff810a33e0>] kthread+0xb0/0xc0
> [  727.367228]  [<ffffffff8167ec04>] kernel_thread_helper+0x4/0x10
> [  727.373244]  [<ffffffff816744b4>] ? retint_restore_args+0x13/0x13
> [  727.379432]  [<ffffffff810a3330>] ? __init_kthread_worker+0x70/0x70
> [  727.385792]  [<ffffffff8167ec00>] ? gs_change+0x13/0x13
> [  727.391105] Code: d2 48 8b 83 00 02 00 00 48 8b 80 98 00 00 00 eb 21 66 0f 1f 84 00 00 00 00 00 bf 0a 00 00 00 e8 a6 1b c7 ff 48 8b 83 00 02 00 00 <48> 8b 80 98 00 00 00 48 8b 35 11 4e 8d 00 48 89 df 4c 29 e6 ff
> [  727.414105] RIP  [<ffffffff8141d1e1>] do_scsi_scan_host+0x61/0xa0
> [  727.420355]  RSP <ffff88178b2abe40>
> [  727.423933] CR2: ffffffffa01874b8
> [  727.427340] ---[ end trace 87ef4ae7ab723385 ]---
> From what I observerved it happens when when we call the rmmod only a while after a modprobe
> (in this case it is the mpt2sas driver). More accurately said, it happens when rmmod is called
> while scsi async is still at work. The driver is removed but the scsi_host_template is still filled
> with now invalid pointers, in this case it is most likely the hostt->scan_finished which causes the BUG.
> 
> A logical step is to wait in scsi_remove_host until the async scan ends.
> @@ -172,6 +173,9 @@ void scsi_remove_host(struct Scsi_Host *shost)
>  	scsi_forget_host(shost);
>  	mutex_unlock(&shost->scan_mutex);
>  	scsi_proc_host_rm(shost);
> +	
> +	while (shost->async_scan)
> +		msleep(10);
>  
>  	spin_lock_irqsave(shost->host_lock, flags);
>  	if (scsi_host_set_state(shost, SHOST_DEL))
> 
> And a change in scsi_finish_async_scan is needed too - moving the 'async_scan = 0;' so it is
> after other functions which could touch a shost.
> @@ -1800,9 +1795,6 @@ static void scsi_finish_async_scan(struct async_scan_data *data)
>  
>  	scsi_sysfs_add_devices(shost);
>  
> -	spin_lock_irqsave(shost->host_lock, flags);
> -	shost->async_scan = 0;
> -	spin_unlock_irqrestore(shost->host_lock, flags);
>  
>  	mutex_unlock(&shost->scan_mutex);
>  
> @@ -1816,7 +1808,11 @@ static void scsi_finish_async_scan(struct async_scan_data *data)
>  	spin_unlock(&async_scan_lock);
>  
>  	scsi_autopm_put_host(shost);
> -	scsi_host_put(shost);
> +	
> +	spin_lock_irqsave(shost->host_lock, flags);
> +	shost->async_scan = 0;
> +	spin_unlock_irqrestore(shost->host_lock, flags);
> +	
>  	kfree(data);
>  }
> 
> When the async scan is protected this way a further protection via ref. count is no more needed
> so remove it
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 29c4c04..be9e6fe 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1743,10 +1743,9 @@ static struct async_scan_data *scsi_prep_async_scan(struct Scsi_Host *shost)
>  
>  	data = kmalloc(sizeof(*data), GFP_KERNEL);
>  	if (!data)
> -		goto err;
> -	data->shost = scsi_host_get(shost);
> -	if (!data->shost)
> -		goto err;
> +		return NULL;
> +
> +	data->shost = shost;
>  	init_completion(&data->prev_finished);
>  
>  	mutex_lock(&shost->scan_mutex);

Do you need to remove the matching scsi_host_put in
scsi_finish_async_scan too?

> 
> And tune the do_scsi_scan_host so it ends faster in case the host is being removed
> @@ -1827,8 +1823,14 @@ static void do_scsi_scan_host(struct Scsi_Host *shost)
>  		if (shost->hostt->scan_start)
>  			shost->hostt->scan_start(shost);
>  
> -		while (!shost->hostt->scan_finished(shost, jiffies - start))
> +		while (!shost->hostt->scan_finished(shost, jiffies - start)) {
>  			msleep(10);
> +			if (!scsi_host_scan_allowed(shost)) {
> +				printk (KERN_INFO "scsi: host not in proper a "
> +					"state, async scan aborted ...\n");
> +				break;
> +			}
> +		}
>  	} else {
>  		scsi_scan_host_selected(shost, SCAN_WILD_CARD, SCAN_WILD_CARD,
>  				SCAN_WILD_CARD, 0);
> ------------------
> Is the above solution good enough?

I think it will sort of work. It is like doing a complete/wait or flush
on the thread. scsi_scan_target users do something similar where they
will queue the scan work on the shost->work_q then wait on the flush in
functions like fc_remove_host.

I guess you could also just convert the code to use workqueues (do a
workqueue_struct for scsi_scan.c and a per host workqueue) then call
flush_work in scsi_remove_host.

The only problem I could think of is if the scan_finished times out,
then do_scsi_scan_host will return and do_scan_async will call
scsi_finish_async_scan and scsi_remove_host will continue to run.
However, if scsi_scan.c was still running something like the sequential
scan/report luns scanning code or if the scsi eh was running and that is
what caused the scan_finished to timeout then that could be accessing
the scsi_host_template and other structs.

I did not look at all the scan_finished callouts to see what they do.