[PATCH] scsi: lpfc: Fix potential memory leak

[PATCH] scsi: lpfc: Fix potential memory leak

[Liang He]

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@126.com>
---
 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

Re: [PATCH] scsi: lpfc: Fix potential memory leak

[Justin Tee]

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

On Thu, Mar 16, 2023 at 7:02 PM Liang He <windhl@126.com> 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@126.com>
> ---
>  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
>

Re:Re: [PATCH] scsi: lpfc: Fix potential memory leak

[Liang He]

At 2023-03-28 02:39:38, "Justin Tee" <justintee8345@gmail.com> 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@126.com> 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@126.com>
>> ---
>>  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
>>