Re: [PATCH 7/7] target: core: check RTPI uniquity for enabled TPG

[Dmitry Bogdanov <d.bogdanov@yadro.com>]

On Thu, Sep 29, 2022 at 07:02:12PM -0500, Mike Christie wrote:
> 
> On 9/6/22 10:45 AM, Dmitry Bogdanov wrote:
> > Garantee uniquity of RTPI only for enabled target port groups.
> > Allow any RPTI for disabled tpg until it is enabled.
> >
> > Signed-off-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
> > ---
> >  drivers/target/target_core_fabric_configfs.c | 29 +++++++++++++-
> >  drivers/target/target_core_internal.h        |  2 +
> >  drivers/target/target_core_tpg.c             | 40 +++++++++++++-------
> >  3 files changed, 56 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
> > index a34b5db4eec5..fc1b8f54fb54 100644
> > --- a/drivers/target/target_core_fabric_configfs.c
> > +++ b/drivers/target/target_core_fabric_configfs.c
> > @@ -857,6 +857,7 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
> >                                                  size_t count)
> >  {
> >       struct se_portal_group *se_tpg = to_tpg(item);
> > +     struct se_portal_group *tpg;
> >       int ret;
> >       bool op;
> >
> > @@ -867,11 +868,37 @@ static ssize_t target_fabric_tpg_base_enable_store(struct config_item *item,
> >       if (se_tpg->enabled == op)
> >               return count;
> >
> > +     spin_lock(&g_tpg_lock);
> > +
> > +     if (op) {
> > +             tpg = core_get_tpg_by_rtpi(se_tpg->tpg_rtpi);
> > +             if (tpg) {
> > +                     spin_unlock(&g_tpg_lock);
> > +
> > +                     pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
> > +                            se_tpg->se_tpg_tfo->fabric_name,
> > +                            se_tpg->se_tpg_tfo->tpg_get_tag(tpg),
> > +                            se_tpg->tpg_rtpi,
> > +                            tpg->se_tpg_tfo->fabric_name,
> > +                            tpg->se_tpg_tfo->tpg_get_tag(tpg));
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     se_tpg->enabled |= 0x10; /* transient state */
> 
> Just use a mutex and hold it the entire time if you can and
> drop this. Or add a proper enum/define for the states and make a real
> API since this is exported to userspace, and it's going to be difficult
> to explain to users that state.

Yes, it looks wierd.
After rewriting to xarray I decided (according to SAM-5) to not allow
to change RTPI on the enabled TPG. And that part will be dropped.
4.6.5.2 Relative Port Identifier attribute
  The relative port identifier for a SCSI port shall not change once
  assigned unless reconfiguration of the SCSI target device occurs.
> 
> > +     spin_unlock(&g_tpg_lock);
> > +
> >       ret = se_tpg->se_tpg_tfo->fabric_enable_tpg(se_tpg, op);
> > -     if (ret)
> > +
> > +     spin_lock(&g_tpg_lock);
> > +     if (ret < 0) {
> > +             se_tpg->enabled &= ~0x10; /* clear transient state */
> > +             spin_unlock(&g_tpg_lock);
> >               return ret;
> > +     }
> >
> >       se_tpg->enabled = op;
> > +     spin_unlock(&g_tpg_lock);
> >
> >       return count;
> >  }
> > diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
> > index d662cdc9a04c..d4ace697edb0 100644
> > --- a/drivers/target/target_core_internal.h
> > +++ b/drivers/target/target_core_internal.h
> > @@ -116,6 +116,7 @@ int       core_tmr_lun_reset(struct se_device *, struct se_tmr_req *,
> >               struct list_head *, struct se_cmd *);
> >
> >  /* target_core_tpg.c */
> > +extern struct spinlock g_tpg_lock;
> >  extern struct se_device *g_lun0_dev;
> >  extern struct configfs_attribute *core_tpg_attrib_attrs[];
> >
> > @@ -131,6 +132,7 @@ void core_tpg_remove_lun(struct se_portal_group *, struct se_lun *);
> >  struct se_node_acl *core_tpg_add_initiator_node_acl(struct se_portal_group *tpg,
> >               const char *initiatorname);
> >  void core_tpg_del_initiator_node_acl(struct se_node_acl *acl);
> > +struct se_portal_group *core_get_tpg_by_rtpi(u16 rtpi);
> >
> >  /* target_core_transport.c */
> >  extern struct kmem_cache *se_tmr_req_cache;
> > diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
> > index 85c9473a38fd..ef2adbe628e0 100644
> > --- a/drivers/target/target_core_tpg.c
> > +++ b/drivers/target/target_core_tpg.c
> > @@ -34,7 +34,7 @@ extern struct se_device *g_lun0_dev;
> >  static u16 g_tpg_count;
> >  static u16 g_tpg_rtpi_counter = 1;
> >  static LIST_HEAD(g_tpg_list);
> > -static DEFINE_SPINLOCK(g_tpg_lock);
> > +DEFINE_SPINLOCK(g_tpg_lock);
> >
> >  /*   __core_tpg_get_initiator_node_acl():
> >   *
> > @@ -476,7 +476,7 @@ static int core_tpg_register_rtpi(struct se_portal_group *se_tpg)
> >                * Make sure RELATIVE TARGET PORT IDENTIFIER is unique
> >                * for 16-bit wrap..
> >                */
> > -             if (se_tpg->tpg_rtpi == tpg->tpg_rtpi)
> > +             if (tpg->enabled && se_tpg->tpg_rtpi == tpg->tpg_rtpi)
> >                       goto again;
> >       }
> >       list_add(&se_tpg->tpg_list, &g_tpg_list);
> > @@ -738,12 +738,26 @@ static ssize_t core_tpg_rtpi_show(struct config_item *item, char *page)
> >       return sprintf(page, "%#x\n", se_tpg->tpg_rtpi);
> >  }
> >
> > +struct se_portal_group *
> > +core_get_tpg_by_rtpi(u16 rtpi)
> > +{
> > +     struct se_portal_group *tpg;
> > +
> > +     lockdep_assert_held(&g_tpg_lock);
> > +
> > +     list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
> > +             if (tpg->enabled && rtpi == tpg->tpg_rtpi)
> > +                     return tpg;
> > +     }
> > +
> > +     return NULL;
> > +}
> > +
> >  static ssize_t core_tpg_rtpi_store(struct config_item *item,
> >                                  const char *page, size_t count)
> >  {
> >       struct se_portal_group *se_tpg = attrib_to_tpg(item);
> >       struct se_portal_group *tpg;
> > -     bool rtpi_changed = false;
> >       u16 val;
> >       int ret;
> >
> > @@ -753,11 +767,14 @@ static ssize_t core_tpg_rtpi_store(struct config_item *item,
> >       if (val == 0)
> >               return -EINVAL;
> >
> > -     /* RTPI shouldn't conflict with values of existing ports */
> > +     if (se_tpg->tpg_rtpi == val)
> > +             return count;
> 
> This test above and the chunk at the end looks like it should have been
> in patch 6.
> 
This patch will be completely rewritten.

> > +
> >       spin_lock(&g_tpg_lock);
> >
> > -     list_for_each_entry(tpg, &g_tpg_list, tpg_list) {
> > -             if (se_tpg != tpg && val == tpg->tpg_rtpi) {
> > +     if (se_tpg->enabled) {
> > +             tpg = core_get_tpg_by_rtpi(val);
> > +             if (tpg) {
> >                       spin_unlock(&g_tpg_lock);
> >                       pr_err("TARGET_CORE[%s]->TPG[%u] - RTPI %#x conflicts with TARGET_CORE[%s]->TPG[%u]\n",
> >                              se_tpg->se_tpg_tfo->fabric_name,
> > @@ -769,17 +786,12 @@ static ssize_t core_tpg_rtpi_store(struct config_item *item,
> >               }
> >       }
> >
> > -     if (se_tpg->tpg_rtpi != val) {
> > -             se_tpg->tpg_rtpi = val;
> > -             rtpi_changed = true;
> > -     }
> > +     se_tpg->tpg_rtpi = val;
> >       spin_unlock(&g_tpg_lock);
> >
> > -     if (rtpi_changed)
> > -             core_tpg_ua(se_tpg, 0x3f, ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
> > -     ret = count;
> > +     core_tpg_ua(se_tpg, 0x3f, ASCQ_3FH_INQUIRY_DATA_HAS_CHANGED);
> >
> > -     return ret;
> > +     return count;
> >  }
> >
> >  CONFIGFS_ATTR(core_tpg_, rtpi);
>