On Wed, 2014-02-05 at 14:02 -0800, Andy Grover wrote:
> Hi nab, I'm getting back to looking at this patchset, but wanted to just
> discuss and understand this one first because all the kref ones are
> similar. see below.
>
> On 12/16/2013 12:52 PM, Nicholas A. Bellinger wrote:
> > On Fri, 2013-12-13 at 15:58 -0800, Andy Grover wrote:
> >> Use kref to handle reference counting
> >>
> >> Signed-off-by: Andy Grover <agrover@xxxxxxxxxx>
> >> ---
> >> drivers/target/target_core_alua.c | 37 ++++++++++++++++++++-----------------
> >> include/target/target_core_base.h | 2 +-
> >> 2 files changed, 21 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/target/target_core_alua.c b/drivers/target/target_core_alua.c
> >> index 2ac2f11..8c01ade 100644
> >> --- a/drivers/target/target_core_alua.c
> >> +++ b/drivers/target/target_core_alua.c
> >> @@ -54,6 +54,16 @@ static LIST_HEAD(lu_gps_list);
> >>
> >> struct t10_alua_lu_gp *default_lu_gp;
> >>
> >> +static void release_alua_lu_gp(struct kref *ref)
> >> +{
> >> + struct t10_alua_lu_gp *lu_gp = container_of(ref, struct t10_alua_lu_gp, refcount);
> >> +
> >> + kmem_cache_free(t10_alua_lu_gp_cache, lu_gp);
> >> +}
> >> +
> >> +#define get_alua_lu_gp(x) kref_get(&x->refcount)
> >> +#define put_alua_lu_gp(x) kref_put(&x->refcount, release_alua_lu_gp)
> >> +
> >> /*
> >> * REPORT_TARGET_PORT_GROUPS
> >> *
> >> @@ -898,8 +908,7 @@ int core_alua_do_port_transition(
> >> local_lu_gp_mem = l_dev->dev_alua_lu_gp_mem;
> >> spin_lock(&local_lu_gp_mem->lu_gp_mem_lock);
> >> lu_gp = local_lu_gp_mem->lu_gp;
> >> - atomic_inc(&lu_gp->lu_gp_ref_cnt);
> >> - smp_mb__after_atomic_inc();
> >> + get_alua_lu_gp(lu_gp);
> >> spin_unlock(&local_lu_gp_mem->lu_gp_mem_lock);
> >> /*
> >> * For storage objects that are members of the 'default_lu_gp',
> >> @@ -913,8 +922,8 @@ int core_alua_do_port_transition(
> >> */
> >> core_alua_do_transition_tg_pt(l_tg_pt_gp, l_port, l_nacl,
> >> md_buf, new_state, explicit);
> >> - atomic_dec(&lu_gp->lu_gp_ref_cnt);
> >> - smp_mb__after_atomic_dec();
> >> +
> >> + put_alua_lu_gp(lu_gp);
> >> kfree(md_buf);
> >> return 0;
> >> }
> >> @@ -985,8 +994,7 @@ int core_alua_do_port_transition(
> >> l_tg_pt_gp->tg_pt_gp_id, (explicit) ? "explicit" : "implicit",
> >> core_alua_dump_state(new_state));
> >>
> >> - atomic_dec(&lu_gp->lu_gp_ref_cnt);
> >> - smp_mb__after_atomic_dec();
> >> + put_alua_lu_gp(lu_gp);
> >> kfree(md_buf);
> >> return 0;
> >> }
> >> @@ -1107,7 +1115,8 @@ core_alua_allocate_lu_gp(const char *name, int def_group)
> >> INIT_LIST_HEAD(&lu_gp->lu_gp_node);
> >> INIT_LIST_HEAD(&lu_gp->lu_gp_mem_list);
> >> spin_lock_init(&lu_gp->lu_gp_lock);
> >> - atomic_set(&lu_gp->lu_gp_ref_cnt, 0);
> >> +
> >> + kref_init(&lu_gp->refcount);
> >>
> >> if (def_group) {
> >> lu_gp->lu_gp_id = alua_lu_gps_counter++;
> >> @@ -1200,13 +1209,7 @@ void core_alua_free_lu_gp(struct t10_alua_lu_gp *lu_gp)
> >> list_del(&lu_gp->lu_gp_node);
> >> alua_lu_gps_count--;
> >> spin_unlock(&lu_gps_lock);
> >> - /*
> >> - * Allow struct t10_alua_lu_gp * referenced by core_alua_get_lu_gp_by_name()
> >> - * in target_core_configfs.c:target_core_store_alua_lu_gp() to be
> >> - * released with core_alua_put_lu_gp_from_name()
> >> - */
> >> - while (atomic_read(&lu_gp->lu_gp_ref_cnt))
> >> - cpu_relax();
> >> +
> >> /*
> >> * Release reference to struct t10_alua_lu_gp * from all associated
> >> * struct se_device.
> >> @@ -1241,7 +1244,7 @@ void core_alua_free_lu_gp(struct t10_alua_lu_gp *lu_gp)
> >> }
> >> spin_unlock(&lu_gp->lu_gp_lock);
> >>
> >> - kmem_cache_free(t10_alua_lu_gp_cache, lu_gp);
> >> + put_alua_lu_gp(lu_gp);
> >> }
> >>
>
> > The assumption that it's safe to 'Release reference to struct
> > t10_alua_lu_gp * from all associated struct device' below the original
> > cpu_relax(), while there are still other process contexts doing their
> > respective put_alua_lu_gp() is totally wrong.
>
> The only other spot is core_alua_do_port_transition, afaics. I think if
> it races with free_lu_gp, lu_gp will either be the old lu_gp (which no
> longer will have anything on lu_gp_mem_list) or will be default_lu_gp.
> If it's the old lu_gp then it iterates over an empty list, and then the
> lu_gp gets finally freed by the put() at the bottom.
>
> > Furthermore, allowing a configfs_group_ops->drop_item() to return while
> > there are still active references from other process contexts means that
> > the parent struct config_group is no longer referenced counted (eg:
> > configfs child is removed), and introduces a whole host of potential
> > bugs.
> >
> > So that said, NAK on this patch.
>
> I think some of the other patches used drop_item() and thus were bad,
> but in this one the existing code is already calling
> core_alua_free_lu_gp() from release().
>
> Thoughts?
>
The problem with this patch and all of the other patches that follow the
same logic is the false assumption that it's safe to return from
configfs_group_operations->drop_item() before all references to the
underlying data structure containing the associated struct config_group
have been dropped.
In this particular case, target_core_alua_drop_lu_gp() ->
config_item_put() -> target_core_alua_lu_gp_release() ->
core_alua_free_lu_gp() is still being called from ->drop_item() via
target_core_alua_lu_gp_ops->release(), so the same holds true here as
well.
--nab
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
