| Message ID | 20230317015602.1748372-1-windhl@126.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: lpfc: Fix potential memory leak | expand |
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
>
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 >>
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;
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(+)