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?
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.
>
> - 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.
> * 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);
> }
