> On Jun 8, 2020, at 10:21 AM, Mike Christie <michael.christie@xxxxxxxxxx> wrote: > > On 6/8/20 1:14 AM, Hannes Reinecke wrote: >>> + se_sess->target_name = kstrdup(se_tpg->se_tpg_wwn->wwn_group.cg_item.ci_name, >>> + GFP_KERNEL); >>> + if (!se_sess->target_name) >>> + goto free_acl; >>> + >>> + if (se_sess->tfo->fabric_alias) >>> + se_sess->fabric_name = kstrdup(se_sess->tfo->fabric_alias, >>> + GFP_KERNEL); >>> + else >>> + se_sess->fabric_name = kstrdup(se_sess->tfo->fabric_name, >>> + GFP_KERNEL); >>> + if (!se_sess->fabric_name) >>> + goto free_target; >>> + >>> + se_sess->tpg_name = kstrdup(se_tpg->tpg_group.cg_item.ci_name, >>> + GFP_KERNEL); >>> + if (!se_sess->tpg_name) >>> + goto free_fabric; >>> + >>> + return 0; >> I wonder if we need to copy these variables. >> Why can't we display them directly, returning an error if the respective >> link is not available? >> If one is worried about the sysfs/configfs reference counting we can get the reference in the _show() functions; wouldn't that be a better solution? > > Do you mean in the sysfs show function do a configfs_depend_item() on the tpg, www, acl, etc? If so, I'm not sure that's safe, because for the tpg for example, we could do: > > 1. Userspace starts tpg removal with a rmdir. > 2. Userspace also opens sysfs session file and has a ref to the session, but not the tpg yet. > 3. kernel tears down tpg and sessions under it. The tpg is freed. The session is not because of the ref taken in #2. > 4. sysfs session show function starts to reference se_sess->se_tpg so it can do a configfs_depend_item on the tpg_group.cg_item, but the se_tpg is freed in #3. > > We either need to: > 1. cp like above. > 2. Handle both configfs and device struct refcounts for the same struct. So add a device struct to the www, tpg, acl and then coordinate the refcounting. > 3. take a reference to the se_tpg when the session is created then drop it in the session release. Ignore #3. That does not work. Userspace expects to be able to rmdir on a tpg and that will remove the underlying sessions. If the session takes a ref to the tpg, then it breaks userspace because something now has to do the session removal before the tpg rmdir. > 4. add some code and end up mix locking and state checks with refcounts. For example the tpg would have its configfs refcount like it does today (no new device struct in it), and when it deletes the sessions under it make sure the se_sess->se_tpg is NULL'd in a way that the session show function can do > > lock() > if (!se_sess->se_tpg) > return > > se_sess->se_tpg access > unlock() > > And then you have to do that up the stack for the other structs we want to ref.
