At 2023-03-28 02:39:38, "Justin Tee" <justintee8345@xxxxxxxxx> wrote:
>Hi Liang,
>
>@@ -1200,6 +1200,11 @@ 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) {
>
>if (&evt->node == &phba->ct_ev_waiters) was true, then that means the
>ct_ev_waiters list was empty so the *evt ptr is not pointing to a real
>lpfc_bsg_event structure. I think we would even crash trying to
>decrement evt’s kref because evt is an uninitialized lpfc_bsg_event
>structure and not pointing to anything real. We’re about to kzalloc a
>new evt with lpfc_bsg_event_new in this same if statement clause
>anyways. I’m not clear on the reasoning behind the suggestion to call
>lpfc_bsg_event_unref(evt) there.
>
>
>@@ -1292,6 +1297,9 @@ lpfc_bsg_hba_get_event(struct bsg_job *job)
> * an error indicating that there isn't anymore
> */
> if (evt_dat == NULL) {
>+ spin_lock_irqsave(&phba->ct_ev_lock, flags);
>+ lpfc_bsg_event_unref(evt);
>+ spin_unlock_irqrestore(&phba->ct_ev_lock, flags);
> bsg_reply->reply_payload_rcv_len = 0;
> rc = -ENOENT;
> goto job_error;
>
>Similar argument here. If (evt_dat == NULL) was true, then that means
>there was nothing of interest in the ct_ev_waiters list or that the
>ct_ev_waiters list was empty. In either case, we shouldn’t call
>lpfc_bsg_event_unref(evt) because we would be decrementing the wrong
>reg_id evt’s kref or even crash if the ct_ev_waiters list was empty
>trying to kref_put an uninitialized lpfc_bsg_event structure.
>
>Regards,
>Justin
Hi, Justin,
Thanks for your kindly explaination.
My patch is indeed wrong, as I have not realized the semantic of "&evt->node == &phba->ct_ev_waiters".
Thanks and sorry for my trouble.
Liang
>
>On Thu, Mar 16, 2023 at 7:02 PM Liang He <windhl@xxxxxxx> wrote:
>>
>> In lpfc_bsg_hba_get_event() and lpfc_bsg_hba_set_event(), we
>> should keep the refcount balance when there is some error or
>> the *evt* will be replaced.
>>
>> Fixes: f1c3b0fcbb81 ("[SCSI] lpfc 8.3.4: Add bsg (SGIOv4) support for ELS/CT support")
>> Signed-off-by: Liang He <windhl@xxxxxxx>
>> ---
>> drivers/scsi/lpfc/lpfc_bsg.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/scsi/lpfc/lpfc_bsg.c b/drivers/scsi/lpfc/lpfc_bsg.c
>> index 852b025e2fec..aa535bc14758 100644
>> --- a/drivers/scsi/lpfc/lpfc_bsg.c
>> +++ b/drivers/scsi/lpfc/lpfc_bsg.c
>> @@ -1200,6 +1200,11 @@ 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) {
>> @@ -1292,6 +1297,9 @@ lpfc_bsg_hba_get_event(struct bsg_job *job)
>> * an error indicating that there isn't anymore
>> */
>> if (evt_dat == NULL) {
>> + spin_lock_irqsave(&phba->ct_ev_lock, flags);
>> + lpfc_bsg_event_unref(evt);
>> + spin_unlock_irqrestore(&phba->ct_ev_lock, flags);
>> bsg_reply->reply_payload_rcv_len = 0;
>> rc = -ENOENT;
>> goto job_error;
>> --
>> 2.25.1
>>
