| Message ID | 20241105130835.4447-1-chenqiuji666@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: lpfc: Fix improper handling of refcount in lpfc_bsg_hba_set_event() | expand |
Hi Qiu-ji, This patch does not look logically correct. if (&evt->node == &phba->ct_ev_waiters) evaluates to true, then it is not possible that (evt->reg_id == event_req->ev_reg_id) is also true. Because if (evt->reg_id == event_req->ev_reg_id) evaluates to true, it means we have found an lpfc_bsg_event of event_req specified interest and therefore (&evt->node != &phba->ct_ev_waiters) must be true. Also, following this suggested patch’s logic, if after attempting to go through the phba->ct_ev_waiters list and the evt->node iterator is pointing at exactly the phba->ct_ev_waiters head, then this patch’s kref_put will be for the phba->ct_ev_waiters head which is not a preallocated lpfc_bsg_event object. So, this patch would be calling kref_put on an uninitialized memory region. Sorry, I cannot acknowledge this patch. Regards, Justin
diff --git a/drivers/scsi/lpfc/lpfc_bsg.c b/drivers/scsi/lpfc/lpfc_bsg.c index 85059b83ea6b..3a65270c5584 100644 --- a/drivers/scsi/lpfc/lpfc_bsg.c +++ b/drivers/scsi/lpfc/lpfc_bsg.c @@ -1200,6 +1200,9 @@ lpfc_bsg_hba_set_event(struct bsg_job *job) spin_unlock_irqrestore(&phba->ct_ev_lock, flags); if (&evt->node == &phba->ct_ev_waiters) { + spin_lock_irqsave(&phba->ct_ev_lock, flags); + lpfc_bsg_event_unref(evt); + spin_unlock_irqrestore(&phba->ct_ev_lock, flags); /* no event waiting struct yet - first call */ dd_data = kmalloc(sizeof(struct bsg_job_data), GFP_KERNEL); if (dd_data == NULL) {
This patch fixes a reference count handling issue in the function lpfc_bsg_hba_set_event(). In the branch if (evt->reg_id == event_req->ev_reg_id), the function calls lpfc_bsg_event_ref(), which increments the reference count of the associated resource. However, in the subsequent branch if (&evt->node == &phba->ct_ev_waiters), a new evt is allocated, but the old evt should be released at this point. Failing to do so could lead to issues. To resolve this issue, we added a release instruction at the beginning of the next branch if (&evt->node == &phba->ct_ev_waiters), ensuring that the resources allocated in the previous branch are properly released, thereby preventing a reference count leak. This bug was identified by an experimental static analysis tool developed by our team. The tool specializes in analyzing reference count operations and detecting potential issues where resources are not properly managed. In this case, the tool flagged the missing release operation as a potential problem, which led to the development of this patch. Fixes: 4cc0e56e977f ("[SCSI] lpfc 8.3.8: (BSG3) Modify BSG commands to operate asynchronously") Cc: stable@vger.kernel.org Signed-off-by: Qiu-ji Chen <chenqiuji666@gmail.com> --- drivers/scsi/lpfc/lpfc_bsg.c | 3 +++ 1 file changed, 3 insertions(+)