[PATCH 0/2] libfc: Handling of extra kref.

[PATCH 0/2] libfc: Handling of extra kref.

[Javed Hasan]

Kindly apply this series to scsi tree at your earliest convenience.

Thanks,
Javed

Javed Hasan (2):
  scsi: libfc: Handling of extra kref.
  scsi: libfc: Skip additional kref updating work event.

 drivers/scsi/libfc/fc_rport.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] scsi: libfc: Handling of extra kref.

[Javed Hasan]

  Handling of extra kref which is done by lookup table in
  case rdata is already present in list.

  This issue was leading to memory leak. Below is the trace from
  KMEMLEAK tool.
  unreferenced object 0xffff8888259e8780 (size 512):
    comm "kworker/2:1", pid 182614, jiffies 4433237386 (age 113021.971s)
    hex dump (first 32 bytes):
    58 0a ec cf 83 88 ff ff 00 00 00 00 00 00 00 00
    01 00 00 00 08 00 00 00 13 7d f0 1e 0e 00 00 10
  backtrace:
	[<000000006b25760f>] fc_rport_recv_req+0x3c6/0x18f0 [libfc]
	[<00000000f208d994>] fc_lport_recv_els_req+0x120/0x8a0 [libfc]
	[<00000000a9c437b8>] fc_lport_recv+0xb9/0x130 [libfc]
	[<00000000ad5be37b>] qedf_ll2_process_skb+0x73d/0xad0 [qedf]
	[<00000000e0eb6893>] process_one_work+0x382/0x6c0
	[<000000002dfd9e21>] worker_thread+0x57/0x5c0
	[<00000000b648204f>] kthread+0x1a0/0x1c0
	[<0000000072f5ab20>] ret_from_fork+0x35/0x40
	[<000000001d5c05d8>] 0xffffffffffffffff

 Below is the logs sequence which leads to memory leak.
 Here we get the nested "Received PLOGI request" for same port
 and this request leads to call the fc_rport_create() twice for the same rport.
	kernel: host1: rport fffce5: Received PLOGI request
	kernel: host1: rport fffce5: Received PLOGI in INIT state
	kernel: host1: rport fffce5: Port is Ready
	kernel: host1: rport fffce5: Received PRLI request while in state Ready
	kernel: host1: rport fffce5: PRLI rspp type 8 active 1 passive 0
	kernel: host1: rport fffce5: Received LOGO request while in state Ready
	kernel: host1: rport fffce5: Delete port
	kernel: host1: rport fffce5: Received PLOGI request
	kernel: host1: rport fffce5: Received PLOGI in state Delete - send busy

Reviewed-by: Girish Basrur <gbasrur@marvell.com>
Reviewed-by: Saurav Kashyap <skashyap@marvell.com>
Reviewed-by: Shyam Sundar <ssundar@marvell.com>
Signed-off-by: Javed Hasan <jhasan@marvell.com>
---
 drivers/scsi/libfc/fc_rport.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index 83636ef..ca39b4b 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -133,8 +133,10 @@ struct fc_rport_priv *fc_rport_create(struct fc_lport *lport, u32 port_id)
 	lockdep_assert_held(&lport->disc.disc_mutex);
 
 	rdata = fc_rport_lookup(lport, port_id);
-	if (rdata)
+	if (rdata) {
+		kref_put(&rdata->kref, fc_rport_destroy);
 		return rdata;
+	}
 
 	if (lport->rport_priv_size > 0)
 		rport_priv_size = lport->rport_priv_size;
-- 
1.8.3.1

[PATCH 2/2] scsi: libfc: Skip additional kref updating work event.

[Javed Hasan]

  When an rport event(RPORT_EV_READY) is updated without work being
  queued, avoid taking an additional reference.

  This issue was leading to memory leak. Below is the trace from
  KMEMLEAK tool.
  unreferenced object 0xffff8888259e8780 (size 512):
  comm "kworker/2:1", pid 182614, jiffies 4433237386 (age 113021.971s)
    hex dump (first 32 bytes):
	58 0a ec cf 83 88 ff ff 00 00 00 00 00 00 00 00
	01 00 00 00 08 00 00 00 13 7d f0 1e 0e 00 00 10
  backtrace:
    [<000000006b25760f>] fc_rport_recv_req+0x3c6/0x18f0 [libfc]
    [<00000000f208d994>] fc_lport_recv_els_req+0x120/0x8a0 [libfc]
    [<00000000a9c437b8>] fc_lport_recv+0xb9/0x130 [libfc]
    [<00000000a9c437b8>] fc_lport_recv+0xb9/0x130 [libfc]
    [<00000000ad5be37b>] qedf_ll2_process_skb+0x73d/0xad0 [qedf]
    [<00000000e0eb6893>] process_one_work+0x382/0x6c0
    [<000000002dfd9e21>] worker_thread+0x57/0x5c0
    [<00000000b648204f>] kthread+0x1a0/0x1c0
    [<0000000072f5ab20>] ret_from_fork+0x35/0x40
    [<000000001d5c05d8>] 0xffffffffffffffff

  Below is the logs sequence which leads to memory leak.
  Here we get the RPORT_EV_READY and RPORT_EV_STOP back to back, which lead to
  overwrite the event RPORT_EV_READY by event RPORT_EV_STOP. Because of this
  kref_count get incremented by 1.
	kernel: host0: rport fffce5: Received PLOGI request
	kernel: host0: rport fffce5: Received PLOGI in INIT state
	kernel: host0: rport fffce5: Port is Ready
	kernel: host0: rport fffce5: Received PRLI request while in state Ready
	kernel: host0: rport fffce5: PRLI rspp type 8 active 1 passive 0
	kernel: host0: rport fffce5: Received LOGO request while in state Ready
	kernel: host0: rport fffce5: Delete port
	kernel: host0: rport fffce5: Received PLOGI request
	kernel: host0: rport fffce5: Received PLOGI in state Delete - send busy
	kernel: host0: rport fffce5: work event 3
	kernel: host0: rport fffce5: lld callback ev 3
	kernel: host0: rport fffce5: work delete

  Reviewed-by: Girish Basrur <gbasrur@marvell.com>
  Reviewed-by: Saurav Kashyap <skashyap@marvell.com>
  Reviewed-by: Shyam Sundar <ssundar@marvell.com>
  Signed-off-by: Javed Hasan <jhasan@marvell.com>
---
 drivers/scsi/libfc/fc_rport.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/libfc/fc_rport.c b/drivers/scsi/libfc/fc_rport.c
index ca39b4b..830df32f 100644
--- a/drivers/scsi/libfc/fc_rport.c
+++ b/drivers/scsi/libfc/fc_rport.c
@@ -483,10 +483,11 @@ static void fc_rport_enter_delete(struct fc_rport_priv *rdata,
 
 	fc_rport_state_enter(rdata, RPORT_ST_DELETE);
 
-	kref_get(&rdata->kref);
-	if (rdata->event == RPORT_EV_NONE &&
-	    !queue_work(rport_event_queue, &rdata->event_work))
-		kref_put(&rdata->kref, fc_rport_destroy);
+	if (rdata->event == RPORT_EV_NONE) {
+		kref_get(&rdata->kref);
+	    if(!queue_work(rport_event_queue, &rdata->event_work))
+			kref_put(&rdata->kref, fc_rport_destroy);
+	}
 
 	rdata->event = event;
 }
-- 
1.8.3.1

Re: [PATCH 2/2] scsi: libfc: Skip additional kref updating work event.

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2242 bytes --]

Hi Javed,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on scsi/for-next v5.8-rc2 next-20200625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Javed-Hasan/libfc-Handling-of-extra-kref/20200622-181643
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: h8300-randconfig-m031-20200624 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

smatch warnings:
drivers/scsi/libfc/fc_rport.c:488 fc_rport_enter_delete() warn: inconsistent indenting

vim +488 drivers/scsi/libfc/fc_rport.c

   459	
   460	/**
   461	 * fc_rport_enter_delete() - Schedule a remote port to be deleted
   462	 * @rdata: The remote port to be deleted
   463	 * @event: The event to report as the reason for deletion
   464	 *
   465	 * Allow state change into DELETE only once.
   466	 *
   467	 * Call queue_work only if there's no event already pending.
   468	 * Set the new event so that the old pending event will not occur.
   469	 * Since we have the mutex, even if fc_rport_work() is already started,
   470	 * it'll see the new event.
   471	 *
   472	 * Reference counting: does not modify kref
   473	 */
   474	static void fc_rport_enter_delete(struct fc_rport_priv *rdata,
   475					  enum fc_rport_event event)
   476	{
   477		lockdep_assert_held(&rdata->rp_mutex);
   478	
   479		if (rdata->rp_state == RPORT_ST_DELETE)
   480			return;
   481	
   482		FC_RPORT_DBG(rdata, "Delete port\n");
   483	
   484		fc_rport_state_enter(rdata, RPORT_ST_DELETE);
   485	
   486		if (rdata->event == RPORT_EV_NONE) {
   487			kref_get(&rdata->kref);
 > 488		    if(!queue_work(rport_event_queue, &rdata->event_work))
   489				kref_put(&rdata->kref, fc_rport_destroy);
   490		}
   491	
   492		rdata->event = event;
   493	}
   494	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29729 bytes --]

Re: [PATCH 0/2] libfc: Handling of extra kref.

[Martin K. Petersen]

On Mon, 22 Jun 2020 03:12:10 -0700, Javed Hasan wrote:

> Kindly apply this series to scsi tree at your earliest convenience.
> 
> Thanks,
> Javed
> 
> Javed Hasan (2):
>   scsi: libfc: Handling of extra kref.
>   scsi: libfc: Skip additional kref updating work event.
> 
> [...]

Applied to 5.8/scsi-fixes, thanks!

[1/2] scsi: libfc: Handling of extra kref
      https://git.kernel.org/mkp/scsi/c/71f2bf85e90d
[2/2] scsi: libfc: Skip additional kref updating work event
      https://git.kernel.org/mkp/scsi/c/823a65409c89

-- 
Martin K. Petersen	Oracle Linux Engineering