Re: [PATCH 1/2] scsi :bnx2fc :bnx2fc_els :fix bnx2fc_cmd refcount imbalance in send_rec

[Saurav Kashyap <skashyap@xxxxxxxxxxx>]

Hi Lin,

On 23/06/19, 7:27 AM, "linux-scsi-owner@xxxxxxxxxxxxxxx on behalf of Lin
Yi" <linux-scsi-owner@xxxxxxxxxxxxxxx on behalf of teroincn@xxxxxxx> wrote:

>if cb_arg alloc failed, we can't release the struct orig_io_req refcount
>before we take it's refcount. call kref_get before malloc, so as to pair
>with kref_put on rec_err path.
>
>Signed-off-by: Lin Yi <teroincn@xxxxxxx>
>---
> drivers/scsi/bnx2fc/bnx2fc_els.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/scsi/bnx2fc/bnx2fc_els.c
>b/drivers/scsi/bnx2fc/bnx2fc_els.c
>index 76e65a3..709bb92 100644
>--- a/drivers/scsi/bnx2fc/bnx2fc_els.c
>+++ b/drivers/scsi/bnx2fc/bnx2fc_els.c
>@@ -592,13 +592,13 @@ int bnx2fc_send_rec(struct bnx2fc_cmd *orig_io_req)
> 	BNX2FC_IO_DBG(orig_io_req, "Sending REC\n");
> 	memset(&rec, 0, sizeof(rec));
> 
>+	kref_get(&orig_io_req->refcount);
> 	cb_arg = kzalloc(sizeof(struct bnx2fc_els_cb_arg), GFP_ATOMIC);
> 	if (!cb_arg) {
> 		printk(KERN_ERR PFX "Unable to allocate cb_arg for REC\n");
> 		rc = -ENOMEM;
> 		goto rec_err;
> 	}
>-	kref_get(&orig_io_req->refcount);
> 
> 	cb_arg->aborted_io_req = orig_io_req;
> 
>-- 
>1.9.1

Thanks for the patch, but this is not the correct fix. If kzalloc fails
code will go to rec_err and try to free cb_arg as well.
Correct way is to move the rec_err label down.

diff --git a/bnx2fc/driver/bnx2fc_els.c b/bnx2fc/driver/bnx2fc_els.c
index 2287008..1b816af 100644
--- a/bnx2fc/driver/bnx2fc_els.c
+++ b/bnx2fc/driver/bnx2fc_els.c
@@ -635,7 +635,6 @@ int bnx2fc_send_rec(struct bnx2fc_cmd *orig_io_req)
        rc = bnx2fc_initiate_els(tgt, ELS_REC, &rec, sizeof(rec),
                                 bnx2fc_rec_compl, cb_arg,
                                 r_a_tov);
-rec_err:
        if (rc) {
                BNX2FC_IO_DBG(orig_io_req, "REC failed - release\n");
                spin_lock_bh(&tgt->tgt_lock);
@@ -643,6 +642,7 @@ rec_err:
                spin_unlock_bh(&tgt->tgt_lock);
                kfree(cb_arg);
        }
+rec_err:
        return rc;
 }

Kindly submit the updated patch.

Thanks,
~Saurav

>