[bug report] scsi: elx: efct: LIO backend interface routines

[bug report] scsi: elx: efct: LIO backend interface routines

[Dan Carpenter]

Hello James Smart,

The patch 692e5d73a811: "scsi: elx: efct: LIO backend interface
routines" from Jun 1, 2021, leads to the following static checker
warning:

	drivers/scsi/elx/efct/efct_lio.c:851 efct_lio_npiv_make_nport()
	warn: '&vport_list->list_entry' not removed from list

drivers/scsi/elx/efct/efct_lio.c
   828          vport_list = kzalloc(sizeof(*vport_list), GFP_KERNEL);
   829          if (!vport_list) {
   830                  kfree(lio_vport);
   831                  return ERR_PTR(-ENOMEM);
   832          }
   833  
   834          vport_list->lio_vport = lio_vport;
   835          spin_lock_irqsave(&efct->tgt_efct.efct_lio_lock, flags);
   836          INIT_LIST_HEAD(&vport_list->list_entry);
   837          list_add_tail(&vport_list->list_entry, &efct->tgt_efct.vport_list);
                               ^^^^^^^^^^^^^^^^^^^^^^
Is it possible to add this to the list after fc_vport_create() succeeds?

   838          spin_unlock_irqrestore(&efct->tgt_efct.efct_lio_lock, flags);
   839  
   840          memset(&vport_id, 0, sizeof(vport_id));
   841          vport_id.port_name = npiv_wwpn;
   842          vport_id.node_name = npiv_wwnn;
   843          vport_id.roles = FC_PORT_ROLE_FCP_INITIATOR;
   844          vport_id.vport_type = FC_PORTTYPE_NPIV;
   845          vport_id.disable = false;
   846  
   847          new_fc_vport = fc_vport_create(efct->shost, 0, &vport_id);
   848          if (!new_fc_vport) {
   849                  efc_log_err(efct, "fc_vport_create failed\n");
   850                  kfree(lio_vport);
   851                  kfree(vport_list);

In the corrent code we free it without removing it from the list which
leads to a use after free.

   852                  return ERR_PTR(-ENOMEM);
   853          }
   854  
   855          lio_vport->fc_vport = new_fc_vport;
   856  
   857          return &lio_vport->vport_wwn;
   858  }

regards,
dan carpenter

[bug report] scsi: elx: efct: LIO backend interface routines

[Dan Carpenter]

Hello James Smart,

The patch 692e5d73a811: "scsi: elx: efct: LIO backend interface
routines" from Jun 1, 2021, leads to the following
Smatch static checker warning:

	drivers/base/power/sysfs.c:833 dpm_sysfs_remove()
	warn: sleeping in atomic context

drivers/base/power/sysfs.c
    829 void dpm_sysfs_remove(struct device *dev)
    830 {
    831         if (device_pm_not_required(dev))
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^
If this is true then the warning is a false positive.

    832                 return;
--> 833         sysfs_unmerge_group(&dev->kobj, &pm_qos_latency_tolerance_attr_group);

It's the down_read() in kernfs_find_and_get_ns() that sleeps.

    834         dev_pm_qos_constraints_destroy(dev);
    835         rpm_sysfs_remove(dev);
    836         sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
    837         sysfs_remove_group(&dev->kobj, &pm_attr_group);
    838 }

The call tree is:

efct_lio_npiv_drop_nport() <- disables preempt
-> fc_vport_terminate()
   -> device_del()
      -> dpm_sysfs_remove()

drivers/scsi/elx/efct/efct_lio.c
   875  efct_lio_npiv_drop_nport(struct se_wwn *wwn)
   876  {
   877          struct efct_lio_vport *lio_vport =
   878                  container_of(wwn, struct efct_lio_vport, vport_wwn);
   879          struct efct_lio_vport_list_t *vport, *next_vport;
   880          struct efct *efct = lio_vport->efct;
   881          unsigned long flags = 0;
   882  
   883          spin_lock_irqsave(&efct->tgt_efct.efct_lio_lock, flags);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Holding a lock.

   884  
   885          if (lio_vport->fc_vport)
   886                  fc_vport_terminate(lio_vport->fc_vport);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Sleeps on the success path unless device_pm_not_required() is true in
dpm_sysfs_remove().

   887  
   888          list_for_each_entry_safe(vport, next_vport, &efct->tgt_efct.vport_list,
   889                                   list_entry) {
   890                  if (vport->lio_vport == lio_vport) {
   891                          list_del(&vport->list_entry);
   892                          kfree(vport->lio_vport);
   893                          kfree(vport);
   894                          break;
   895                  }
   896          }
   897          spin_unlock_irqrestore(&efct->tgt_efct.efct_lio_lock, flags);
   898  }

regards,
dan carpenter