On Tue, Jul 26, 2022 at 05:38:52PM -0500, Mike Christie wrote:
>
> On 7/20/22 6:28 AM, Dmitry Bogdanov wrote:
> > @@ -1437,34 +1435,26 @@ static void core_scsi3_nodeacl_undepend_item(struct se_node_acl *nacl)
> >
> > static int core_scsi3_lunacl_depend_item(struct se_dev_entry *se_deve)
> > {
> > - struct se_lun_acl *lun_acl;
> > -
> > /*
> > * For nacl->dynamic_node_acl=1
> > */
> > - lun_acl = rcu_dereference_check(se_deve->se_lun_acl,
> > - kref_read(&se_deve->pr_kref) != 0);
> > - if (!lun_acl)
> > + if (!se_deve->se_lun_acl)
> > return 0;
>
> Will se_lun_acl and se_lun ever be NULL now? Should this maybe be a
> WARN_ON or return a -EXYZ error code like when target_depend_item fails
> at the bottom of the function?
After the first patch from the patchset deve->se_lun_acl is always NULL
for deve on a dynamyc ACL, and always NOT NULL for deve on a configured
ACL, and deve->se_lun is never NULL.
>
> I don't think it's a problem with your patch. I think we had issues
> before it. If they were NULL then I think we can hit issues like:
>
> 1. __core_scsi3_alloc_registration calls core_scsi3_lunacl_depend_item.
> If se_lun_acl is NULL and we return 0.
>
> 2. __core_scsi3_alloc_registration and other callers treat 0 as success.
>
> 3. It then does:
>
> dest_lun = deve_tmp->se_lun;
>
> pr_reg_atp = __core_scsi3_do_alloc_registration(dev,
> nacl_tmp, dest_lun, deve_tmp,
>
> 4. So if se_lun is also NULL then for __core_scsi3_do_alloc_registration's
> args we have:
>
> dest_deve, as non-NULL
> lun as NULL
>
> 5. __core_scsi3_do_alloc_registration will then do a
>
> pr_reg->pr_aptpl_target_lun = lun->unpacked_lun;
> pr_reg->tg_pt_sep_rtpi = lun->lun_rtpi;
>
> and since lun is NULL we will crash.
I think it was not possible even before my patches becasue deve->se_lun
NULLing was after waiting for deve->pr_kref == 0, which is incremented
in PR OUT handling under spinlock for lun->lun_deve_list. There
everything is safe.
>
>
>
> >
> > - return target_depend_item(&lun_acl->se_lun_group.cg_item);
> > + return target_depend_item(&se_deve->se_lun_acl->se_lun_group.cg_item);
> > }
> >
> > static void core_scsi3_lunacl_undepend_item(struct se_dev_entry *se_deve)
> > {
> > - struct se_lun_acl *lun_acl;
> > -
> > - /*
> > +/*
>
> Just drop the misformatting bit above.
oops, will drop it.
> > * For nacl->dynamic_node_acl=1
> > */
> > - lun_acl = rcu_dereference_check(se_deve->se_lun_acl,
> > - kref_read(&se_deve->pr_kref) != 0);
> > - if (!lun_acl) {
> > + if (!se_deve->se_lun_acl) {
> > kref_put(&se_deve->pr_kref, target_pr_kref_release);
> > return;
> > }
> >
> > - target_undepend_item(&lun_acl->se_lun_group.cg_item);
> > + target_undepend_item(&se_deve->se_lun_acl->se_lun_group.cg_item);
> > kref_put(&se_deve->pr_kref, target_pr_kref_release);
> > }
