Message ID | 20190927161923.24915-2-connor.kuehl@canonical.com |
---|---|
State | New |
Headers | show |
Series | SAS DoS | expand |
On 2019-09-27 09:19:23, Connor Kuehl wrote: > From: Jason Yan <yanaijie@huawei.com> > > CVE-2017-18232 > > In commit 87c8331fcf72 ("[SCSI] libsas: prevent domain rediscovery > competing with ata error handling") introduced disco mutex to prevent > rediscovery competing with ata error handling and put the whole > revalidation in the mutex. But the rphy add/remove needs to wait for the > error handling which also grabs the disco mutex. This may leads to dead > lock.So the probe and destruct event were introduce to do the rphy > add/remove asynchronously and out of the lock. > > The asynchronously processed workers makes the whole discovery process > not atomic, the other events may interrupt the process. For example, > if a loss of signal event inserted before the probe event, the > sas_deform_port() is called and the port will be deleted. > > And sas_port_delete() may run before the destruct event, but the > port-x:x is the top parent of end device or expander. This leads to > a kernel WARNING such as: > > [ 82.042979] sysfs group 'power' not found for kobject 'phy-1:0:22' > [ 82.042983] ------------[ cut here ]------------ > [ 82.042986] WARNING: CPU: 54 PID: 1714 at fs/sysfs/group.c:237 > sysfs_remove_group+0x94/0xa0 > [ 82.043059] Call trace: > [ 82.043082] [<ffff0000082e7624>] sysfs_remove_group+0x94/0xa0 > [ 82.043085] [<ffff00000864e320>] dpm_sysfs_remove+0x60/0x70 > [ 82.043086] [<ffff00000863ee10>] device_del+0x138/0x308 > [ 82.043089] [<ffff00000869a2d0>] sas_phy_delete+0x38/0x60 > [ 82.043091] [<ffff00000869a86c>] do_sas_phy_delete+0x6c/0x80 > [ 82.043093] [<ffff00000863dc20>] device_for_each_child+0x58/0xa0 > [ 82.043095] [<ffff000008696f80>] sas_remove_children+0x40/0x50 > [ 82.043100] [<ffff00000869d1bc>] sas_destruct_devices+0x64/0xa0 > [ 82.043102] [<ffff0000080e93bc>] process_one_work+0x1fc/0x4b0 > [ 82.043104] [<ffff0000080e96c0>] worker_thread+0x50/0x490 > [ 82.043105] [<ffff0000080f0364>] kthread+0xfc/0x128 > [ 82.043107] [<ffff0000080836c0>] ret_from_fork+0x10/0x50 > > Make probe and destruct a direct call in the disco and revalidate function, > but put them outside the lock. The whole discovery or revalidate won't > be interrupted by other events. And the DISCE_PROBE and DISCE_DESTRUCT > event are deleted as a result of the direct call. > > Introduce a new list to destruct the sas_port and put the port delete after > the destruct. This makes sure the right order of destroying the sysfs > kobject and fix the warning above. > > In sas_ex_revalidate_domain() have a loop to find all broadcasted > device, and sometimes we have a chance to find the same expander twice. > Because the sas_port will be deleted at the end of the whole revalidate > process, sas_port with the same name cannot be added before this. > Otherwise the sysfs will complain of creating duplicate filename. Since > the LLDD will send broadcast for every device change, we can only > process one expander's revalidation. > > [mkp: kbuild test robot warning] > > Signed-off-by: Jason Yan <yanaijie@huawei.com> > CC: John Garry <john.garry@huawei.com> > CC: Johannes Thumshirn <jthumshirn@suse.de> > CC: Ewan Milne <emilne@redhat.com> > CC: Christoph Hellwig <hch@lst.de> > CC: Tomas Henzl <thenzl@redhat.com> > CC: Dan Williams <dan.j.williams@intel.com> > Reviewed-by: Hannes Reinecke <hare@suse.com> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> > (backported from commit 0558f33c06bb910e2879e355192227a8e8f0219d) > [ Connor Kuehl: The hunk that removed variants from 'enum > discover_event' required manual placement. I did take the liberty of > removing the hardcoded enum values from this enum as is done in > upstream commit 0d78f969b10f "scsi: libsas: remove the numbering for > each event enum" but only for this enum to reduce confusion over > renumbering. It seemed overkill to take in the entire patch alongside > this backport. ] > Signed-off-by: Connor Kuehl <connor.kuehl@canonical.com> The changes to the discover_event enum look fine to me. Acked-by: Tyler Hicks <tyhicks@canonical.com> Thanks! Tyler > --- > drivers/scsi/libsas/sas_ata.c | 1 - > drivers/scsi/libsas/sas_discover.c | 32 +++++++++++++++++------------- > drivers/scsi/libsas/sas_expander.c | 8 +++----- > drivers/scsi/libsas/sas_internal.h | 1 + > drivers/scsi/libsas/sas_port.c | 3 +++ > include/scsi/libsas.h | 13 ++++++------ > include/scsi/scsi_transport_sas.h | 1 + > 7 files changed, 32 insertions(+), 27 deletions(-) > > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c > index 6f5e2720ffad..e018e76b156b 100644 > --- a/drivers/scsi/libsas/sas_ata.c > +++ b/drivers/scsi/libsas/sas_ata.c > @@ -732,7 +732,6 @@ int sas_discover_sata(struct domain_device *dev) > if (res) > return res; > > - sas_discover_event(dev->port, DISCE_PROBE); > return 0; > } > > diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c > index 60de66252fa2..487d7345f515 100644 > --- a/drivers/scsi/libsas/sas_discover.c > +++ b/drivers/scsi/libsas/sas_discover.c > @@ -212,13 +212,9 @@ void sas_notify_lldd_dev_gone(struct domain_device *dev) > } > } > > -static void sas_probe_devices(struct work_struct *work) > +static void sas_probe_devices(struct asd_sas_port *port) > { > struct domain_device *dev, *n; > - struct sas_discovery_event *ev = to_sas_discovery_event(work); > - struct asd_sas_port *port = ev->port; > - > - clear_bit(DISCE_PROBE, &port->disc.pending); > > /* devices must be domain members before link recovery and probe */ > list_for_each_entry(dev, &port->disco_list, disco_list_node) { > @@ -294,7 +290,6 @@ int sas_discover_end_dev(struct domain_device *dev) > res = sas_notify_lldd_dev_found(dev); > if (res) > return res; > - sas_discover_event(dev->port, DISCE_PROBE); > > return 0; > } > @@ -353,13 +348,9 @@ static void sas_unregister_common_dev(struct asd_sas_port *port, struct domain_d > sas_put_device(dev); > } > > -static void sas_destruct_devices(struct work_struct *work) > +void sas_destruct_devices(struct asd_sas_port *port) > { > struct domain_device *dev, *n; > - struct sas_discovery_event *ev = to_sas_discovery_event(work); > - struct asd_sas_port *port = ev->port; > - > - clear_bit(DISCE_DESTRUCT, &port->disc.pending); > > list_for_each_entry_safe(dev, n, &port->destroy_list, disco_list_node) { > list_del_init(&dev->disco_list_node); > @@ -370,6 +361,16 @@ static void sas_destruct_devices(struct work_struct *work) > } > } > > +static void sas_destruct_ports(struct asd_sas_port *port) > +{ > + struct sas_port *sas_port, *p; > + > + list_for_each_entry_safe(sas_port, p, &port->sas_port_del_list, del_list) { > + list_del_init(&sas_port->del_list); > + sas_port_delete(sas_port); > + } > +} > + > void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *dev) > { > if (!test_bit(SAS_DEV_DESTROY, &dev->state) && > @@ -384,7 +385,6 @@ void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *dev) > if (!test_and_set_bit(SAS_DEV_DESTROY, &dev->state)) { > sas_rphy_unlink(dev->rphy); > list_move_tail(&dev->disco_list_node, &port->destroy_list); > - sas_discover_event(dev->port, DISCE_DESTRUCT); > } > } > > @@ -490,6 +490,8 @@ static void sas_discover_domain(struct work_struct *work) > port->port_dev = NULL; > } > > + sas_probe_devices(port); > + > SAS_DPRINTK("DONE DISCOVERY on port %d, pid:%d, result:%d\n", port->id, > task_pid_nr(current), error); > } > @@ -523,6 +525,10 @@ static void sas_revalidate_domain(struct work_struct *work) > port->id, task_pid_nr(current), res); > out: > mutex_unlock(&ha->disco_mutex); > + > + sas_destruct_devices(port); > + sas_destruct_ports(port); > + sas_probe_devices(port); > } > > /* ---------- Events ---------- */ > @@ -578,10 +584,8 @@ void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *port) > static const work_func_t sas_event_fns[DISC_NUM_EVENTS] = { > [DISCE_DISCOVER_DOMAIN] = sas_discover_domain, > [DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain, > - [DISCE_PROBE] = sas_probe_devices, > [DISCE_SUSPEND] = sas_suspend_devices, > [DISCE_RESUME] = sas_resume_devices, > - [DISCE_DESTRUCT] = sas_destruct_devices, > }; > > disc->pending = 0; > diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c > index 400eee9d7783..3d104be4076e 100644 > --- a/drivers/scsi/libsas/sas_expander.c > +++ b/drivers/scsi/libsas/sas_expander.c > @@ -1908,7 +1908,8 @@ static void sas_unregister_devs_sas_addr(struct domain_device *parent, > sas_port_delete_phy(phy->port, phy->phy); > sas_device_set_phy(found, phy->port); > if (phy->port->num_phys == 0) > - sas_port_delete(phy->port); > + list_add_tail(&phy->port->del_list, > + &parent->port->sas_port_del_list); > phy->port = NULL; > } > } > @@ -2121,7 +2122,7 @@ int sas_ex_revalidate_domain(struct domain_device *port_dev) > struct domain_device *dev = NULL; > > res = sas_find_bcast_dev(port_dev, &dev); > - while (res == 0 && dev) { > + if (res == 0 && dev) { > struct expander_device *ex = &dev->ex_dev; > int i = 0, phy_id; > > @@ -2133,9 +2134,6 @@ int sas_ex_revalidate_domain(struct domain_device *port_dev) > res = sas_rediscover(dev, phy_id); > i = phy_id + 1; > } while (i < ex->num_phys); > - > - dev = NULL; > - res = sas_find_bcast_dev(port_dev, &dev); > } > return res; > } > diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h > index 9cf0bc260b0e..2cbbd113d898 100644 > --- a/drivers/scsi/libsas/sas_internal.h > +++ b/drivers/scsi/libsas/sas_internal.h > @@ -98,6 +98,7 @@ int sas_try_ata_reset(struct asd_sas_phy *phy); > void sas_hae_reset(struct work_struct *work); > > void sas_free_device(struct kref *kref); > +void sas_destruct_devices(struct asd_sas_port *port); > > #ifdef CONFIG_SCSI_SAS_HOST_SMP > extern int sas_smp_host_handler(struct Scsi_Host *shost, struct request *req, > diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c > index d3c5297c6c89..5d3244c8f280 100644 > --- a/drivers/scsi/libsas/sas_port.c > +++ b/drivers/scsi/libsas/sas_port.c > @@ -66,6 +66,7 @@ static void sas_resume_port(struct asd_sas_phy *phy) > rc = sas_notify_lldd_dev_found(dev); > if (rc) { > sas_unregister_dev(port, dev); > + sas_destruct_devices(port); > continue; > } > > @@ -219,6 +220,7 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone) > > if (port->num_phys == 1) { > sas_unregister_domain_devices(port, gone); > + sas_destruct_devices(port); > sas_port_delete(port->port); > port->port = NULL; > } else { > @@ -323,6 +325,7 @@ static void sas_init_port(struct asd_sas_port *port, > INIT_LIST_HEAD(&port->dev_list); > INIT_LIST_HEAD(&port->disco_list); > INIT_LIST_HEAD(&port->destroy_list); > + INIT_LIST_HEAD(&port->sas_port_del_list); > spin_lock_init(&port->phy_list_lock); > INIT_LIST_HEAD(&port->phy_list); > port->ha = sas_ha; > diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h > index 706a7017885c..c9226edaca08 100644 > --- a/include/scsi/libsas.h > +++ b/include/scsi/libsas.h > @@ -85,13 +85,11 @@ enum phy_event { > > enum discover_event { > DISCE_DISCOVER_DOMAIN = 0U, > - DISCE_REVALIDATE_DOMAIN = 1, > - DISCE_PORT_GONE = 2, > - DISCE_PROBE = 3, > - DISCE_SUSPEND = 4, > - DISCE_RESUME = 5, > - DISCE_DESTRUCT = 6, > - DISC_NUM_EVENTS = 7, > + DISCE_REVALIDATE_DOMAIN, > + DISCE_PORT_GONE, > + DISCE_SUSPEND, > + DISCE_RESUME, > + DISC_NUM_EVENTS, > }; > > /* ---------- Expander Devices ---------- */ > @@ -269,6 +267,7 @@ struct asd_sas_port { > struct list_head dev_list; > struct list_head disco_list; > struct list_head destroy_list; > + struct list_head sas_port_del_list; > enum sas_linkrate linkrate; > > struct sas_work work; > diff --git a/include/scsi/scsi_transport_sas.h b/include/scsi/scsi_transport_sas.h > index 31ae074dad9d..d088d450de61 100644 > --- a/include/scsi/scsi_transport_sas.h > +++ b/include/scsi/scsi_transport_sas.h > @@ -160,6 +160,7 @@ struct sas_port { > > struct mutex phy_list_mutex; > struct list_head phy_list; > + struct list_head del_list; /* libsas only */ > }; > > #define dev_to_sas_port(d) \ > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index 6f5e2720ffad..e018e76b156b 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -732,7 +732,6 @@ int sas_discover_sata(struct domain_device *dev) if (res) return res; - sas_discover_event(dev->port, DISCE_PROBE); return 0; } diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c index 60de66252fa2..487d7345f515 100644 --- a/drivers/scsi/libsas/sas_discover.c +++ b/drivers/scsi/libsas/sas_discover.c @@ -212,13 +212,9 @@ void sas_notify_lldd_dev_gone(struct domain_device *dev) } } -static void sas_probe_devices(struct work_struct *work) +static void sas_probe_devices(struct asd_sas_port *port) { struct domain_device *dev, *n; - struct sas_discovery_event *ev = to_sas_discovery_event(work); - struct asd_sas_port *port = ev->port; - - clear_bit(DISCE_PROBE, &port->disc.pending); /* devices must be domain members before link recovery and probe */ list_for_each_entry(dev, &port->disco_list, disco_list_node) { @@ -294,7 +290,6 @@ int sas_discover_end_dev(struct domain_device *dev) res = sas_notify_lldd_dev_found(dev); if (res) return res; - sas_discover_event(dev->port, DISCE_PROBE); return 0; } @@ -353,13 +348,9 @@ static void sas_unregister_common_dev(struct asd_sas_port *port, struct domain_d sas_put_device(dev); } -static void sas_destruct_devices(struct work_struct *work) +void sas_destruct_devices(struct asd_sas_port *port) { struct domain_device *dev, *n; - struct sas_discovery_event *ev = to_sas_discovery_event(work); - struct asd_sas_port *port = ev->port; - - clear_bit(DISCE_DESTRUCT, &port->disc.pending); list_for_each_entry_safe(dev, n, &port->destroy_list, disco_list_node) { list_del_init(&dev->disco_list_node); @@ -370,6 +361,16 @@ static void sas_destruct_devices(struct work_struct *work) } } +static void sas_destruct_ports(struct asd_sas_port *port) +{ + struct sas_port *sas_port, *p; + + list_for_each_entry_safe(sas_port, p, &port->sas_port_del_list, del_list) { + list_del_init(&sas_port->del_list); + sas_port_delete(sas_port); + } +} + void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *dev) { if (!test_bit(SAS_DEV_DESTROY, &dev->state) && @@ -384,7 +385,6 @@ void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *dev) if (!test_and_set_bit(SAS_DEV_DESTROY, &dev->state)) { sas_rphy_unlink(dev->rphy); list_move_tail(&dev->disco_list_node, &port->destroy_list); - sas_discover_event(dev->port, DISCE_DESTRUCT); } } @@ -490,6 +490,8 @@ static void sas_discover_domain(struct work_struct *work) port->port_dev = NULL; } + sas_probe_devices(port); + SAS_DPRINTK("DONE DISCOVERY on port %d, pid:%d, result:%d\n", port->id, task_pid_nr(current), error); } @@ -523,6 +525,10 @@ static void sas_revalidate_domain(struct work_struct *work) port->id, task_pid_nr(current), res); out: mutex_unlock(&ha->disco_mutex); + + sas_destruct_devices(port); + sas_destruct_ports(port); + sas_probe_devices(port); } /* ---------- Events ---------- */ @@ -578,10 +584,8 @@ void sas_init_disc(struct sas_discovery *disc, struct asd_sas_port *port) static const work_func_t sas_event_fns[DISC_NUM_EVENTS] = { [DISCE_DISCOVER_DOMAIN] = sas_discover_domain, [DISCE_REVALIDATE_DOMAIN] = sas_revalidate_domain, - [DISCE_PROBE] = sas_probe_devices, [DISCE_SUSPEND] = sas_suspend_devices, [DISCE_RESUME] = sas_resume_devices, - [DISCE_DESTRUCT] = sas_destruct_devices, }; disc->pending = 0; diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c index 400eee9d7783..3d104be4076e 100644 --- a/drivers/scsi/libsas/sas_expander.c +++ b/drivers/scsi/libsas/sas_expander.c @@ -1908,7 +1908,8 @@ static void sas_unregister_devs_sas_addr(struct domain_device *parent, sas_port_delete_phy(phy->port, phy->phy); sas_device_set_phy(found, phy->port); if (phy->port->num_phys == 0) - sas_port_delete(phy->port); + list_add_tail(&phy->port->del_list, + &parent->port->sas_port_del_list); phy->port = NULL; } } @@ -2121,7 +2122,7 @@ int sas_ex_revalidate_domain(struct domain_device *port_dev) struct domain_device *dev = NULL; res = sas_find_bcast_dev(port_dev, &dev); - while (res == 0 && dev) { + if (res == 0 && dev) { struct expander_device *ex = &dev->ex_dev; int i = 0, phy_id; @@ -2133,9 +2134,6 @@ int sas_ex_revalidate_domain(struct domain_device *port_dev) res = sas_rediscover(dev, phy_id); i = phy_id + 1; } while (i < ex->num_phys); - - dev = NULL; - res = sas_find_bcast_dev(port_dev, &dev); } return res; } diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h index 9cf0bc260b0e..2cbbd113d898 100644 --- a/drivers/scsi/libsas/sas_internal.h +++ b/drivers/scsi/libsas/sas_internal.h @@ -98,6 +98,7 @@ int sas_try_ata_reset(struct asd_sas_phy *phy); void sas_hae_reset(struct work_struct *work); void sas_free_device(struct kref *kref); +void sas_destruct_devices(struct asd_sas_port *port); #ifdef CONFIG_SCSI_SAS_HOST_SMP extern int sas_smp_host_handler(struct Scsi_Host *shost, struct request *req, diff --git a/drivers/scsi/libsas/sas_port.c b/drivers/scsi/libsas/sas_port.c index d3c5297c6c89..5d3244c8f280 100644 --- a/drivers/scsi/libsas/sas_port.c +++ b/drivers/scsi/libsas/sas_port.c @@ -66,6 +66,7 @@ static void sas_resume_port(struct asd_sas_phy *phy) rc = sas_notify_lldd_dev_found(dev); if (rc) { sas_unregister_dev(port, dev); + sas_destruct_devices(port); continue; } @@ -219,6 +220,7 @@ void sas_deform_port(struct asd_sas_phy *phy, int gone) if (port->num_phys == 1) { sas_unregister_domain_devices(port, gone); + sas_destruct_devices(port); sas_port_delete(port->port); port->port = NULL; } else { @@ -323,6 +325,7 @@ static void sas_init_port(struct asd_sas_port *port, INIT_LIST_HEAD(&port->dev_list); INIT_LIST_HEAD(&port->disco_list); INIT_LIST_HEAD(&port->destroy_list); + INIT_LIST_HEAD(&port->sas_port_del_list); spin_lock_init(&port->phy_list_lock); INIT_LIST_HEAD(&port->phy_list); port->ha = sas_ha; diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index 706a7017885c..c9226edaca08 100644 --- a/include/scsi/libsas.h +++ b/include/scsi/libsas.h @@ -85,13 +85,11 @@ enum phy_event { enum discover_event { DISCE_DISCOVER_DOMAIN = 0U, - DISCE_REVALIDATE_DOMAIN = 1, - DISCE_PORT_GONE = 2, - DISCE_PROBE = 3, - DISCE_SUSPEND = 4, - DISCE_RESUME = 5, - DISCE_DESTRUCT = 6, - DISC_NUM_EVENTS = 7, + DISCE_REVALIDATE_DOMAIN, + DISCE_PORT_GONE, + DISCE_SUSPEND, + DISCE_RESUME, + DISC_NUM_EVENTS, }; /* ---------- Expander Devices ---------- */ @@ -269,6 +267,7 @@ struct asd_sas_port { struct list_head dev_list; struct list_head disco_list; struct list_head destroy_list; + struct list_head sas_port_del_list; enum sas_linkrate linkrate; struct sas_work work; diff --git a/include/scsi/scsi_transport_sas.h b/include/scsi/scsi_transport_sas.h index 31ae074dad9d..d088d450de61 100644 --- a/include/scsi/scsi_transport_sas.h +++ b/include/scsi/scsi_transport_sas.h @@ -160,6 +160,7 @@ struct sas_port { struct mutex phy_list_mutex; struct list_head phy_list; + struct list_head del_list; /* libsas only */ }; #define dev_to_sas_port(d) \