Re: [PATCH v3 1/2] mpt2sas: Refcount sas_device objects and fix unsafe list usage
On Monday 08/10 at 18:45 +0530, Sreekanth Reddy wrote:
> On Sat, Aug 1, 2015 at 10:32 AM, Calvin Owens wrote:
Sreekanth,
Thanks for the review, responses below. I'll have a v4 out shortly.
Calvin
> > These objects can be referenced concurrently throughout the driver, we
> > need a way to make sure threads can't delete them out from under each
> > other. This patch adds the refcount, and refactors the code to use it.
> >
> > Additionally, we cannot iterate over the sas_device_list without
> > holding the lock, or we risk corrupting random memory if items are
> > added or deleted as we iterate. This patch refactors _scsih_probe_sas()
> > to use the sas_device_list in a safe way.
> >
> > Cc: Christoph Hellwig
> > Cc: Bart Van Assche
> > Cc: Joe Lawrence
> > Signed-off-by: Calvin Owens
> > ---
> > Changes in v3:
> > * Drop the sas_device_lock while enabling devices, and leave the
> > sas_device object on the list, since it may need to be looked up
> > there while it is being enabled.
> > * Drop put() in _scsih_add_device(), because the ->hostdata now
> > keeps a
> > reference (this was an oversight in v2).
> > * Be consistent about calling sas_device_put() while holding the
> > sas_device_lock where feasible.
> > * Take and assert_spin_locked() on the sas_device_lock from the
> > newly
> > added __get_sdev_from_target(), add wrapper similar to other
> > lookups
> > for callers which do not explicitly take the lock.
> >
> > Changes in v2:
> > * Squished patches 1-3 into this one
> > * s/BUG_ON(!spin_is_locked/assert_spin_locked/g
> > * Store a pointer to the sas_device object in ->hostdata, to
> > eliminate
> > the need for several lookups on the lists.
> >
> > drivers/scsi/mpt2sas/mpt2sas_base.h | 22 +-
> > drivers/scsi/mpt2sas/mpt2sas_scsih.c | 467
> > +--
> > drivers/scsi/mpt2sas/mpt2sas_transport.c | 12 +-
> > 3 files changed, 348 insertions(+), 153 deletions(-)
> >
> > diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h
> > b/drivers/scsi/mpt2sas/mpt2sas_base.h
> > index caff8d1..78f41ac 100644
> > --- a/drivers/scsi/mpt2sas/mpt2sas_base.h
> > +++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
> > @@ -238,6 +238,7 @@
> > * @flags: MPT_TARGET_FLAGS_XXX flags
> > * @deleted: target flaged for deletion
> > * @tm_busy: target is busy with TM request.
> > + * @sdev: The sas_device associated with this target
> > */
> > struct MPT2SAS_TARGET {
> > struct scsi_target *starget;
> > @@ -248,6 +249,7 @@ struct MPT2SAS_TARGET {
> > u32 flags;
> > u8 deleted;
> > u8 tm_busy;
> > + struct _sas_device *sdev;
> > };
> >
> >
> > @@ -376,8 +378,24 @@ struct _sas_device {
> > u8 phy;
> > u8 responding;
> > u8 pfa_led_on;
> > + struct kref refcount;
> > };
> >
> > +static inline void sas_device_get(struct _sas_device *s)
> > +{
> > + kref_get(>refcount);
> > +}
> > +
> > +static inline void sas_device_free(struct kref *r)
> > +{
> > + kfree(container_of(r, struct _sas_device, refcount));
> > +}
> > +
> > +static inline void sas_device_put(struct _sas_device *s)
> > +{
> > + kref_put(>refcount, sas_device_free);
> > +}
> > +
> > /**
> > * struct _raid_device - raid volume link list
> > * @list: sas device list
> > @@ -1095,7 +1113,9 @@ struct _sas_node
> > *mpt2sas_scsih_expander_find_by_handle(struct MPT2SAS_ADAPTER *
> > u16 handle);
> > struct _sas_node *mpt2sas_scsih_expander_find_by_sas_address(struct
> > MPT2SAS_ADAPTER
> > *ioc, u64 sas_address);
> > -struct _sas_device *mpt2sas_scsih_sas_device_find_by_sas_address(
> > +struct _sas_device *mpt2sas_get_sdev_by_addr(
> > +struct MPT2SAS_ADAPTER *ioc, u64 sas_address);
> > +struct _sas_device *__mpt2sas_get_sdev_by_addr(
> > struct MPT2SAS_ADAPTER *ioc, u64 sas_address);
> >
> > void mpt2sas_port_enable_complete(struct MPT2SAS_ADAPTER *ioc);
> > diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > index 3f26147..a2af9a5 100644
> > --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> > @@ -526,8 +526,61 @@ _scsih_determine_boot_device(struct MPT2SAS_ADAPTER
> > *ioc,
> > }
> > }
> >
> > +static struct _sas_device *
> > +__mpt2sas_get_sdev_from_target(struct MPT2SAS_ADAPTER *ioc,
> > + struct MPT2SAS_TARGET *tgt_priv)
> > +{
> > + struct _sas_device *ret;
> > +
> > + assert_spin_locked(>sas_device_lock);
> > +
> > + ret = tgt_priv->sdev;
> > + if (ret)
> > + sas_device_get(ret);
> > +
> > + return ret;
> > +}
> > +
> > +static struct _sas_device *
> > +mpt2sas_get_sdev_from_target(struct MPT2SAS_ADAPTER *ioc,
> > + struct MPT2SAS_TARGET *tgt_priv)
> > +{
> > + struct _sas_device
Re: [PATCH v3 1/2] mpt2sas: Refcount sas_device objects and fix unsafe list usage
On Monday 08/10 at 18:45 +0530, Sreekanth Reddy wrote:
On Sat, Aug 1, 2015 at 10:32 AM, Calvin Owens calvinow...@fb.com wrote:
Sreekanth,
Thanks for the review, responses below. I'll have a v4 out shortly.
Calvin
These objects can be referenced concurrently throughout the driver, we
need a way to make sure threads can't delete them out from under each
other. This patch adds the refcount, and refactors the code to use it.
Additionally, we cannot iterate over the sas_device_list without
holding the lock, or we risk corrupting random memory if items are
added or deleted as we iterate. This patch refactors _scsih_probe_sas()
to use the sas_device_list in a safe way.
Cc: Christoph Hellwig h...@infradead.org
Cc: Bart Van Assche bart.vanass...@sandisk.com
Cc: Joe Lawrence joe.lawre...@stratus.com
Signed-off-by: Calvin Owens calvinow...@fb.com
---
Changes in v3:
* Drop the sas_device_lock while enabling devices, and leave the
sas_device object on the list, since it may need to be looked up
there while it is being enabled.
* Drop put() in _scsih_add_device(), because the -hostdata now
keeps a
reference (this was an oversight in v2).
* Be consistent about calling sas_device_put() while holding the
sas_device_lock where feasible.
* Take and assert_spin_locked() on the sas_device_lock from the
newly
added __get_sdev_from_target(), add wrapper similar to other
lookups
for callers which do not explicitly take the lock.
Changes in v2:
* Squished patches 1-3 into this one
* s/BUG_ON(!spin_is_locked/assert_spin_locked/g
* Store a pointer to the sas_device object in -hostdata, to
eliminate
the need for several lookups on the lists.
drivers/scsi/mpt2sas/mpt2sas_base.h | 22 +-
drivers/scsi/mpt2sas/mpt2sas_scsih.c | 467
+--
drivers/scsi/mpt2sas/mpt2sas_transport.c | 12 +-
3 files changed, 348 insertions(+), 153 deletions(-)
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h
b/drivers/scsi/mpt2sas/mpt2sas_base.h
index caff8d1..78f41ac 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
@@ -238,6 +238,7 @@
* @flags: MPT_TARGET_FLAGS_XXX flags
* @deleted: target flaged for deletion
* @tm_busy: target is busy with TM request.
+ * @sdev: The sas_device associated with this target
*/
struct MPT2SAS_TARGET {
struct scsi_target *starget;
@@ -248,6 +249,7 @@ struct MPT2SAS_TARGET {
u32 flags;
u8 deleted;
u8 tm_busy;
+ struct _sas_device *sdev;
};
@@ -376,8 +378,24 @@ struct _sas_device {
u8 phy;
u8 responding;
u8 pfa_led_on;
+ struct kref refcount;
};
+static inline void sas_device_get(struct _sas_device *s)
+{
+ kref_get(s-refcount);
+}
+
+static inline void sas_device_free(struct kref *r)
+{
+ kfree(container_of(r, struct _sas_device, refcount));
+}
+
+static inline void sas_device_put(struct _sas_device *s)
+{
+ kref_put(s-refcount, sas_device_free);
+}
+
/**
* struct _raid_device - raid volume link list
* @list: sas device list
@@ -1095,7 +1113,9 @@ struct _sas_node
*mpt2sas_scsih_expander_find_by_handle(struct MPT2SAS_ADAPTER *
u16 handle);
struct _sas_node *mpt2sas_scsih_expander_find_by_sas_address(struct
MPT2SAS_ADAPTER
*ioc, u64 sas_address);
-struct _sas_device *mpt2sas_scsih_sas_device_find_by_sas_address(
+struct _sas_device *mpt2sas_get_sdev_by_addr(
+struct MPT2SAS_ADAPTER *ioc, u64 sas_address);
+struct _sas_device *__mpt2sas_get_sdev_by_addr(
struct MPT2SAS_ADAPTER *ioc, u64 sas_address);
void mpt2sas_port_enable_complete(struct MPT2SAS_ADAPTER *ioc);
diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index 3f26147..a2af9a5 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -526,8 +526,61 @@ _scsih_determine_boot_device(struct MPT2SAS_ADAPTER
*ioc,
}
}
+static struct _sas_device *
+__mpt2sas_get_sdev_from_target(struct MPT2SAS_ADAPTER *ioc,
+ struct MPT2SAS_TARGET *tgt_priv)
+{
+ struct _sas_device *ret;
+
+ assert_spin_locked(ioc-sas_device_lock);
+
+ ret = tgt_priv-sdev;
+ if (ret)
+ sas_device_get(ret);
+
+ return ret;
+}
+
+static struct _sas_device *
+mpt2sas_get_sdev_from_target(struct MPT2SAS_ADAPTER *ioc,
+ struct MPT2SAS_TARGET *tgt_priv)
+{
+ struct _sas_device *ret;
+ unsigned long flags;
+
+ spin_lock_irqsave(ioc-sas_device_lock, flags);
+ ret = __mpt2sas_get_sdev_from_target(ioc, tgt_priv);
Re: [PATCH v3 1/2] mpt2sas: Refcount sas_device objects and fix unsafe list usage
On Sat, Aug 1, 2015 at 10:32 AM, Calvin Owens wrote:
> These objects can be referenced concurrently throughout the driver, we
> need a way to make sure threads can't delete them out from under each
> other. This patch adds the refcount, and refactors the code to use it.
>
> Additionally, we cannot iterate over the sas_device_list without
> holding the lock, or we risk corrupting random memory if items are
> added or deleted as we iterate. This patch refactors _scsih_probe_sas()
> to use the sas_device_list in a safe way.
>
> Cc: Christoph Hellwig
> Cc: Bart Van Assche
> Cc: Joe Lawrence
> Signed-off-by: Calvin Owens
> ---
> Changes in v3:
> * Drop the sas_device_lock while enabling devices, and leave the
> sas_device object on the list, since it may need to be looked up
> there while it is being enabled.
> * Drop put() in _scsih_add_device(), because the ->hostdata now keeps
> a
> reference (this was an oversight in v2).
> * Be consistent about calling sas_device_put() while holding the
> sas_device_lock where feasible.
> * Take and assert_spin_locked() on the sas_device_lock from the newly
> added __get_sdev_from_target(), add wrapper similar to other lookups
> for callers which do not explicitly take the lock.
>
> Changes in v2:
> * Squished patches 1-3 into this one
> * s/BUG_ON(!spin_is_locked/assert_spin_locked/g
> * Store a pointer to the sas_device object in ->hostdata, to eliminate
> the need for several lookups on the lists.
>
> drivers/scsi/mpt2sas/mpt2sas_base.h | 22 +-
> drivers/scsi/mpt2sas/mpt2sas_scsih.c | 467
> +--
> drivers/scsi/mpt2sas/mpt2sas_transport.c | 12 +-
> 3 files changed, 348 insertions(+), 153 deletions(-)
>
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h
> b/drivers/scsi/mpt2sas/mpt2sas_base.h
> index caff8d1..78f41ac 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_base.h
> +++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
> @@ -238,6 +238,7 @@
> * @flags: MPT_TARGET_FLAGS_XXX flags
> * @deleted: target flaged for deletion
> * @tm_busy: target is busy with TM request.
> + * @sdev: The sas_device associated with this target
> */
> struct MPT2SAS_TARGET {
> struct scsi_target *starget;
> @@ -248,6 +249,7 @@ struct MPT2SAS_TARGET {
> u32 flags;
> u8 deleted;
> u8 tm_busy;
> + struct _sas_device *sdev;
> };
>
>
> @@ -376,8 +378,24 @@ struct _sas_device {
> u8 phy;
> u8 responding;
> u8 pfa_led_on;
> + struct kref refcount;
> };
>
> +static inline void sas_device_get(struct _sas_device *s)
> +{
> + kref_get(>refcount);
> +}
> +
> +static inline void sas_device_free(struct kref *r)
> +{
> + kfree(container_of(r, struct _sas_device, refcount));
> +}
> +
> +static inline void sas_device_put(struct _sas_device *s)
> +{
> + kref_put(>refcount, sas_device_free);
> +}
> +
> /**
> * struct _raid_device - raid volume link list
> * @list: sas device list
> @@ -1095,7 +1113,9 @@ struct _sas_node
> *mpt2sas_scsih_expander_find_by_handle(struct MPT2SAS_ADAPTER *
> u16 handle);
> struct _sas_node *mpt2sas_scsih_expander_find_by_sas_address(struct
> MPT2SAS_ADAPTER
> *ioc, u64 sas_address);
> -struct _sas_device *mpt2sas_scsih_sas_device_find_by_sas_address(
> +struct _sas_device *mpt2sas_get_sdev_by_addr(
> +struct MPT2SAS_ADAPTER *ioc, u64 sas_address);
> +struct _sas_device *__mpt2sas_get_sdev_by_addr(
> struct MPT2SAS_ADAPTER *ioc, u64 sas_address);
>
> void mpt2sas_port_enable_complete(struct MPT2SAS_ADAPTER *ioc);
> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> index 3f26147..a2af9a5 100644
> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> @@ -526,8 +526,61 @@ _scsih_determine_boot_device(struct MPT2SAS_ADAPTER *ioc,
> }
> }
>
> +static struct _sas_device *
> +__mpt2sas_get_sdev_from_target(struct MPT2SAS_ADAPTER *ioc,
> + struct MPT2SAS_TARGET *tgt_priv)
> +{
> + struct _sas_device *ret;
> +
> + assert_spin_locked(>sas_device_lock);
> +
> + ret = tgt_priv->sdev;
> + if (ret)
> + sas_device_get(ret);
> +
> + return ret;
> +}
> +
> +static struct _sas_device *
> +mpt2sas_get_sdev_from_target(struct MPT2SAS_ADAPTER *ioc,
> + struct MPT2SAS_TARGET *tgt_priv)
> +{
> + struct _sas_device *ret;
> + unsigned long flags;
> +
> + spin_lock_irqsave(>sas_device_lock, flags);
> + ret = __mpt2sas_get_sdev_from_target(ioc, tgt_priv);
> + spin_unlock_irqrestore(>sas_device_lock, flags);
> +
> + return ret;
> +}
> +
> +
> +struct _sas_device *
> +__mpt2sas_get_sdev_by_addr(struct MPT2SAS_ADAPTER *ioc,
> +u64 sas_address)
> +{
> + struct _sas_device *sas_device;
> +
> +
Re: [PATCH v3 1/2] mpt2sas: Refcount sas_device objects and fix unsafe list usage
On Sat, Aug 1, 2015 at 10:32 AM, Calvin Owens calvinow...@fb.com wrote:
These objects can be referenced concurrently throughout the driver, we
need a way to make sure threads can't delete them out from under each
other. This patch adds the refcount, and refactors the code to use it.
Additionally, we cannot iterate over the sas_device_list without
holding the lock, or we risk corrupting random memory if items are
added or deleted as we iterate. This patch refactors _scsih_probe_sas()
to use the sas_device_list in a safe way.
Cc: Christoph Hellwig h...@infradead.org
Cc: Bart Van Assche bart.vanass...@sandisk.com
Cc: Joe Lawrence joe.lawre...@stratus.com
Signed-off-by: Calvin Owens calvinow...@fb.com
---
Changes in v3:
* Drop the sas_device_lock while enabling devices, and leave the
sas_device object on the list, since it may need to be looked up
there while it is being enabled.
* Drop put() in _scsih_add_device(), because the -hostdata now keeps
a
reference (this was an oversight in v2).
* Be consistent about calling sas_device_put() while holding the
sas_device_lock where feasible.
* Take and assert_spin_locked() on the sas_device_lock from the newly
added __get_sdev_from_target(), add wrapper similar to other lookups
for callers which do not explicitly take the lock.
Changes in v2:
* Squished patches 1-3 into this one
* s/BUG_ON(!spin_is_locked/assert_spin_locked/g
* Store a pointer to the sas_device object in -hostdata, to eliminate
the need for several lookups on the lists.
drivers/scsi/mpt2sas/mpt2sas_base.h | 22 +-
drivers/scsi/mpt2sas/mpt2sas_scsih.c | 467
+--
drivers/scsi/mpt2sas/mpt2sas_transport.c | 12 +-
3 files changed, 348 insertions(+), 153 deletions(-)
diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.h
b/drivers/scsi/mpt2sas/mpt2sas_base.h
index caff8d1..78f41ac 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_base.h
+++ b/drivers/scsi/mpt2sas/mpt2sas_base.h
@@ -238,6 +238,7 @@
* @flags: MPT_TARGET_FLAGS_XXX flags
* @deleted: target flaged for deletion
* @tm_busy: target is busy with TM request.
+ * @sdev: The sas_device associated with this target
*/
struct MPT2SAS_TARGET {
struct scsi_target *starget;
@@ -248,6 +249,7 @@ struct MPT2SAS_TARGET {
u32 flags;
u8 deleted;
u8 tm_busy;
+ struct _sas_device *sdev;
};
@@ -376,8 +378,24 @@ struct _sas_device {
u8 phy;
u8 responding;
u8 pfa_led_on;
+ struct kref refcount;
};
+static inline void sas_device_get(struct _sas_device *s)
+{
+ kref_get(s-refcount);
+}
+
+static inline void sas_device_free(struct kref *r)
+{
+ kfree(container_of(r, struct _sas_device, refcount));
+}
+
+static inline void sas_device_put(struct _sas_device *s)
+{
+ kref_put(s-refcount, sas_device_free);
+}
+
/**
* struct _raid_device - raid volume link list
* @list: sas device list
@@ -1095,7 +1113,9 @@ struct _sas_node
*mpt2sas_scsih_expander_find_by_handle(struct MPT2SAS_ADAPTER *
u16 handle);
struct _sas_node *mpt2sas_scsih_expander_find_by_sas_address(struct
MPT2SAS_ADAPTER
*ioc, u64 sas_address);
-struct _sas_device *mpt2sas_scsih_sas_device_find_by_sas_address(
+struct _sas_device *mpt2sas_get_sdev_by_addr(
+struct MPT2SAS_ADAPTER *ioc, u64 sas_address);
+struct _sas_device *__mpt2sas_get_sdev_by_addr(
struct MPT2SAS_ADAPTER *ioc, u64 sas_address);
void mpt2sas_port_enable_complete(struct MPT2SAS_ADAPTER *ioc);
diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index 3f26147..a2af9a5 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -526,8 +526,61 @@ _scsih_determine_boot_device(struct MPT2SAS_ADAPTER *ioc,
}
}
+static struct _sas_device *
+__mpt2sas_get_sdev_from_target(struct MPT2SAS_ADAPTER *ioc,
+ struct MPT2SAS_TARGET *tgt_priv)
+{
+ struct _sas_device *ret;
+
+ assert_spin_locked(ioc-sas_device_lock);
+
+ ret = tgt_priv-sdev;
+ if (ret)
+ sas_device_get(ret);
+
+ return ret;
+}
+
+static struct _sas_device *
+mpt2sas_get_sdev_from_target(struct MPT2SAS_ADAPTER *ioc,
+ struct MPT2SAS_TARGET *tgt_priv)
+{
+ struct _sas_device *ret;
+ unsigned long flags;
+
+ spin_lock_irqsave(ioc-sas_device_lock, flags);
+ ret = __mpt2sas_get_sdev_from_target(ioc, tgt_priv);
+ spin_unlock_irqrestore(ioc-sas_device_lock, flags);
+
+ return ret;
+}
+
+
+struct _sas_device *
+__mpt2sas_get_sdev_by_addr(struct MPT2SAS_ADAPTER *ioc,
+u64 sas_address)
+{
+ struct _sas_device *sas_device;
+
+
