| Message ID | 20220308020725.16116-2-alice.chao@mediatek.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/1] scsi: Fix racing between dev init and dev reset | expand |
Hi Alice, Thanks for your patch. I have some questions. > Device reset thread uses kobject_uevent_env() to get kobj.parent > after scsi_evt_emit(), and it races with device init thread which > calls device_add() to create kobj.parent before kobject_uevent_env(). > > Device reset call trace: > fill_kobj_path > kobject_get_path > kobject_uevent_env > scsi_evt_emit <- add wait_event() > scsi_evt_thread > > Device init call trace: > fill_kobj_path > kobject_get_path > kobject_uevent_env > device_add <- create kobj.parent > scsi_target_add > scsi_sysfs_add_sdev > scsi_add_lun > scsi_probe_and_add_lun These are backtraces, and the things above looks like your solution, not the problem. It's better to describe the problem first and solution later: Device init call: Device reset call: scsi_probe_and_add_lun() scsi_evt_thread() scsi_add_lun() scsi_sysfs_add_sdev() scsi_evt_emit() scsi_target_add() kobject_uevent_env() device_add() kobject_get_path() // something wrong? fill_kobj_path() get_device_parent() // I see a get, not create parent kobj, could you show me that point? kobject_uevent() kobject_uevent_env() kobject_get_path() fill_kobj_path() > > These two jobs are scheduled asynchronously, we can't guaranteed that > kobj.parent will be created in device init thread before device reset > thread calls kobj_get_path(). What happens if device reset thread calls kobj_get_path() before kobj.parent's creation? a crash or something? > > To resolve the racing issue between device init thread and device > reset thread, we use wait_event() in scsi_evt_emit() to wait for > device_add() to complete the creation of kobj.parent. > > Signed-off-by: Alice Chao <alice.chao@mediatek.com> When we send patch N+1, we have to put the change log between patch v1 to vN under a "---", so others can understand the changes between patch N and N+1. e.g., --- Change since v1 - remove Change-Id > --- > drivers/scsi/scsi_lib.c | 1 + > drivers/scsi/scsi_scan.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 0a70aa763a96..abf9a71ed77c 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -2461,6 +2461,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt) > break; > case SDEV_EVT_POWER_ON_RESET_OCCURRED: > envp[idx++] = "SDEV_UA=POWER_ON_RESET_OCCURRED"; > + wait_event(sdev->host->host_wait, sdev->sdev_gendev.kobj.parent != NULL); > break; > default: > /* do nothing */ > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c > index f4e6c68ac99e..431f229ac435 100644 > --- a/drivers/scsi/scsi_scan.c > +++ b/drivers/scsi/scsi_scan.c > @@ -1904,6 +1904,7 @@ static void do_scsi_scan_host(struct Scsi_Host *shost) > } else { > scsi_scan_host_selected(shost, SCAN_WILD_CARD, SCAN_WILD_CARD, > SCAN_WILD_CARD, 0); > + wake_up(&shost->host_wait); do_scsi_scan_host() is not in the call trace above, could you show the relationship of do_scsi_scan_host in the call flow? thanks, Miles > } > }
Hi Miles, On Wed, 2022-03-09 at 07:23 +0800, Miles Chen wrote: > Hi Alice, > > Thanks for your patch. I have some questions. > > > Device reset thread uses kobject_uevent_env() to get kobj.parent > > after scsi_evt_emit(), and it races with device init thread which > > calls device_add() to create kobj.parent before > > kobject_uevent_env(). > > > > Device reset call trace: > > fill_kobj_path > > kobject_get_path > > kobject_uevent_env > > scsi_evt_emit <- add wait_event() > > scsi_evt_thread > > > > Device init call trace: > > fill_kobj_path > > kobject_get_path > > kobject_uevent_env > > device_add <- create kobj.parent > > scsi_target_add > > scsi_sysfs_add_sdev > > scsi_add_lun > > scsi_probe_and_add_lun > > These are backtraces, and the things above looks like your solution, > not > the problem. > It's better to describe the problem first and solution later: > > Device init call: Device reset call: > scsi_probe_and_add_lun() scsi_evt_thread() > scsi_add_lun() > scsi_sysfs_add_sdev() scsi_evt_emit() > scsi_target_add() kobject_uevent_env > () > device_add() kobject_get_path > () // something wrong? > fill_kobj_path() > get_device_parent() // I see a get, not create parent kobj, > could you show me that point? > kobject_uevent() > kobject_uevent_env() > kobject_get_path() > fill_kobj_path() > Device init call: Device reset call: scsi_probe_and_add_lun() scsi_evt_thread() scsi_add_lun() scsi_evt_emit() scsi_sysfs_add_sdev() kobject_uevent_env() //get kobj.parent scsi_target_add() kobject_get_path() //get wrong kobj.parent device_add() // add kobj.parent fill_kobj_path() kobject_uevent_env() kobject_get_path() fill_kobj_path() Above backtrace describes the problem, device reset thread will get wrong kobj.parent when device init thread didn’t add kobj.parent yet. > > > > These two jobs are scheduled asynchronously, we can't guaranteed > > that > > kobj.parent will be created in device init thread before device > > reset > > thread calls kobj_get_path(). > > What happens if device reset thread calls kobj_get_path() before > kobj.parent's > creation? a crash or something? > It will cause slab-out-of-bounds in kobject_get_path and kernel panic than crash. > > > > To resolve the racing issue between device init thread and device > > reset thread, we use wait_event() in scsi_evt_emit() to wait for > > device_add() to complete the creation of kobj.parent. > > > > Signed-off-by: Alice Chao <alice.chao@mediatek.com> > > When we send patch N+1, we have to put the change log between patch > v1 to vN > under a "---", so others can understand the changes between patch N > and N+1. > e.g., > > --- > > Change since v1 > - remove Change-Id > > > --- > > drivers/scsi/scsi_lib.c | 1 + > > drivers/scsi/scsi_scan.c | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > > index 0a70aa763a96..abf9a71ed77c 100644 > > --- a/drivers/scsi/scsi_lib.c > > +++ b/drivers/scsi/scsi_lib.c > > @@ -2461,6 +2461,7 @@ static void scsi_evt_emit(struct scsi_device > > *sdev, struct scsi_event *evt) > > break; > > case SDEV_EVT_POWER_ON_RESET_OCCURRED: > > envp[idx++] = "SDEV_UA=POWER_ON_RESET_OCCURRED"; > > + wait_event(sdev->host->host_wait, sdev- > > >sdev_gendev.kobj.parent != NULL); > > break; > > default: > > /* do nothing */ > > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c > > index f4e6c68ac99e..431f229ac435 100644 > > --- a/drivers/scsi/scsi_scan.c > > +++ b/drivers/scsi/scsi_scan.c > > @@ -1904,6 +1904,7 @@ static void do_scsi_scan_host(struct > > Scsi_Host *shost) > > } else { > > scsi_scan_host_selected(shost, SCAN_WILD_CARD, > > SCAN_WILD_CARD, > > SCAN_WILD_CARD, 0); > > + wake_up(&shost->host_wait); > > do_scsi_scan_host() is not in the call trace above, could you show > the relationship > of do_scsi_scan_host in the call flow? > ufshcd_async_scan() scsi_scan_host() do_scsi_scan_host() << here scsi_scan_host_selected() __scsi_scan_target() scsi_probe_and_add_lun() scsi_sysfs_add_sdev() device_add() kobject_uevent_env() kobject_get_path() fill_kobj_path() After we add wake_up here, we can ensure that device reset thread will get kobject after device init thread finishes adding parent. > thanks, > Miles > > > } > > }
Hi Alice, >Hi Miles, > >On Wed, 2022-03-09 at 07:23 +0800, Miles Chen wrote: >> Hi Alice, >> >> Thanks for your patch. I have some questions. >> >> > Device reset thread uses kobject_uevent_env() to get kobj.parent >> > after scsi_evt_emit(), and it races with device init thread which >> > calls device_add() to create kobj.parent before >> > kobject_uevent_env(). >> > >> > Device reset call trace: >> > fill_kobj_path >> > kobject_get_path >> > kobject_uevent_env >> > scsi_evt_emit <- add wait_event() >> > scsi_evt_thread >> > >> > Device init call trace: >> > fill_kobj_path >> > kobject_get_path >> > kobject_uevent_env >> > device_add <- create kobj.parent >> > scsi_target_add >> > scsi_sysfs_add_sdev >> > scsi_add_lun >> > scsi_probe_and_add_lun >> >> These are backtraces, and the things above looks like your solution, >> not >> the problem. >> It's better to describe the problem first and solution later: >> >> Device init call: Device reset call: >> scsi_probe_and_add_lun() scsi_evt_thread() >> scsi_add_lun() >> scsi_sysfs_add_sdev() scsi_evt_emit() >> scsi_target_add() kobject_uevent_env >> () >> device_add() kobject_get_path >> () // something wrong? >> fill_kobj_path() >> get_device_parent() // I see a get, not create parent kobj, >> could you show me that point? >> kobject_uevent() >> kobject_uevent_env() >> kobject_get_path() >> fill_kobj_path() >> > >Device init call: Device reset call: >scsi_probe_and_add_lun() scsi_evt_thread() > scsi_add_lun() scsi_evt_emit() > scsi_sysfs_add_sdev() kobject_uevent_env() //get kobj.parent > scsi_target_add() kobject_get_path() //get wrong >kobj.parent device_add() // add kobj.parent fill_kobj_path() > kobject_uevent_env() > kobject_get_path() > fill_kobj_path() >Above backtrace describes the problem, device reset thread will get >wrong kobj.parent when device init thread didn’t add kobj.parent yet. Thanks for sharing the error log, I think it is clearer this way: Device init call: Device reset call: scsi_probe_and_add_lun() scsi_evt_thread() scsi_add_lun() scsi_evt_emit() scsi_sysfs_add_sdev() kobject_uevent_env() //get kobj.parent scsi_target_add() kobject_get_path() len = get_kobj_path_length () // len=1 because parent is not create yet device_add() // add kobj.parent kobject_uevent_env() kobject_get_path() path = kzalloc() fill_kobj_path() fill_kobj_path() // --length; length -= cur is a negative value memcpy(path + length, kobject_name(parent), cur); // slab OOB! Now I understood this slab OOB issue, please add the kasan log and the analysis to the commit message so the maintainers can see it and help. > >> > >> > These two jobs are scheduled asynchronously, we can't guaranteed >> > that >> > kobj.parent will be created in device init thread before device >> > reset >> > thread calls kobj_get_path(). >> >> What happens if device reset thread calls kobj_get_path() before >> kobj.parent's >> creation? a crash or something? >> > >It will cause slab-out-of-bounds in kobject_get_path and kernel panic >than crash. Okay, please add the KASAN slab-out-of-bounds error in the commit message. Thanks, Miles > >> > >> > To resolve the racing issue between device init thread and device >> > reset thread, we use wait_event() in scsi_evt_emit() to wait for >> > device_add() to complete the creation of kobj.parent. >> > >> > Signed-off-by: Alice Chao <alice.chao@mediatek.com> >> >> When we send patch N+1, we have to put the change log between patch >> v1 to vN >> under a "---", so others can understand the changes between patch N >> and N+1. >> e.g., >> >> --- >> >> Change since v1 >> - remove Change-Id >> >> > --- >> > drivers/scsi/scsi_lib.c | 1 + >> > drivers/scsi/scsi_scan.c | 1 + >> > 2 files changed, 2 insertions(+) >> > >> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> > index 0a70aa763a96..abf9a71ed77c 100644 >> > --- a/drivers/scsi/scsi_lib.c >> > +++ b/drivers/scsi/scsi_lib.c >> > @@ -2461,6 +2461,7 @@ static void scsi_evt_emit(struct scsi_device >> > *sdev, struct scsi_event *evt) >> > break; >> > case SDEV_EVT_POWER_ON_RESET_OCCURRED: >> > envp[idx++] = "SDEV_UA=POWER_ON_RESET_OCCURRED"; >> > + wait_event(sdev->host->host_wait, sdev- >> > >sdev_gendev.kobj.parent != NULL); >> > break; >> > default: >> > /* do nothing */ >> > diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c >> > index f4e6c68ac99e..431f229ac435 100644 >> > --- a/drivers/scsi/scsi_scan.c >> > +++ b/drivers/scsi/scsi_scan.c >> > @@ -1904,6 +1904,7 @@ static void do_scsi_scan_host(struct >> > Scsi_Host *shost) >> > } else { >> > scsi_scan_host_selected(shost, SCAN_WILD_CARD, >> > SCAN_WILD_CARD, >> > SCAN_WILD_CARD, 0); >> > + wake_up(&shost->host_wait); >> >> do_scsi_scan_host() is not in the call trace above, could you show >> the relationship >> of do_scsi_scan_host in the call flow? >> > >ufshcd_async_scan() >scsi_scan_host() >do_scsi_scan_host() << here >scsi_scan_host_selected() > __scsi_scan_target() >scsi_probe_and_add_lun() >scsi_sysfs_add_sdev() >device_add() >kobject_uevent_env() >kobject_get_path() >fill_kobj_path() > >After we add wake_up here, we can ensure that device reset thread will >get kobject after device init thread finishes adding parent. > >> thanks, >> Miles >> >> > } >> > } > >
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 0a70aa763a96..abf9a71ed77c 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -2461,6 +2461,7 @@ static void scsi_evt_emit(struct scsi_device *sdev, struct scsi_event *evt) break; case SDEV_EVT_POWER_ON_RESET_OCCURRED: envp[idx++] = "SDEV_UA=POWER_ON_RESET_OCCURRED"; + wait_event(sdev->host->host_wait, sdev->sdev_gendev.kobj.parent != NULL); break; default: /* do nothing */ diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c index f4e6c68ac99e..431f229ac435 100644 --- a/drivers/scsi/scsi_scan.c +++ b/drivers/scsi/scsi_scan.c @@ -1904,6 +1904,7 @@ static void do_scsi_scan_host(struct Scsi_Host *shost) } else { scsi_scan_host_selected(shost, SCAN_WILD_CARD, SCAN_WILD_CARD, SCAN_WILD_CARD, 0); + wake_up(&shost->host_wait); } }
Device reset thread uses kobject_uevent_env() to get kobj.parent after scsi_evt_emit(), and it races with device init thread which calls device_add() to create kobj.parent before kobject_uevent_env(). Device reset call trace: fill_kobj_path kobject_get_path kobject_uevent_env scsi_evt_emit <- add wait_event() scsi_evt_thread Device init call trace: fill_kobj_path kobject_get_path kobject_uevent_env device_add <- create kobj.parent scsi_target_add scsi_sysfs_add_sdev scsi_add_lun scsi_probe_and_add_lun These two jobs are scheduled asynchronously, we can't guaranteed that kobj.parent will be created in device init thread before device reset thread calls kobj_get_path(). To resolve the racing issue between device init thread and device reset thread, we use wait_event() in scsi_evt_emit() to wait for device_add() to complete the creation of kobj.parent. Signed-off-by: Alice Chao <alice.chao@mediatek.com> --- drivers/scsi/scsi_lib.c | 1 + drivers/scsi/scsi_scan.c | 1 + 2 files changed, 2 insertions(+)