[PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when the device is shut down

[PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when the device is shut down

[Dennis Dalessandro]

From: Kaike Wan <kaike.wan@intel.com>

The workqueue hfi1_wq is destroyed in function shutdown_device(), which
is called by either shutdown_one() or remove_one(). The function
shutdown_one() is called when the kernel is rebooted while remove_one()
is called when the hfi1 driver is unloaded. When the kernel is rebooted,
hfi1_wq is destroyed while all qps are still active, leading to a
kernel crash:

[ 198.891986] BUG: unable to handle kernel NULL pointer dereference at 0000000000000102
[ 198.892072] IP: [<ffffffff94cb7b02>] __queue_work+0x32/0x3e0
[ 198.892130] PGD 0
[ 198.892156] Oops: 0000 [#1] SMP
[ 198.892193] Modules linked in: dm_round_robin nvme_rdma(OE) nvme_fabrics(OE) nvme_core(OE) ib_isert iscsi_target_mod target_core_mod ib_ucm mlx4_ib iTCO_wdt iTCO_vendor_support mxm_wmi sb_edac intel_powerclamp coretemp intel_rapl iosf_mbi kvm rpcrdma sunrpc irqbypass crc32_pclmul ghash_clmulni_intel rdma_ucm aesni_intel ib_uverbs lrw gf128mul opa_vnic glue_helper ablk_helper ib_iser cryptd ib_umad rdma_cm iw_cm ses enclosure libiscsi scsi_transport_sas pcspkr joydev ib_ipoib(OE) scsi_transport_iscsi ib_cm sg ipmi_ssif mei_me lpc_ich i2c_i801 mei ioatdma ipmi_si dm_multipath ipmi_devintf ipmi_msghandler wmi acpi_pad acpi_power_meter hangcheck_timer ip_tables ext4 mbcache jbd2 mlx4_en sd_mod crc_t10dif crct10dif_generic mgag200 drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops ttm hfi1(OE)
[ 198.892997] crct10dif_pclmul crct10dif_common crc32c_intel drm ahci mlx4_core libahci rdmavt(OE) igb megaraid_sas ib_core libata drm_panel_orientation_quirks ptp pps_core devlink dca i2c_algo_bit dm_mirror dm_region_hash dm_log dm_mod
[ 198.893233] CPU: 19 PID: 0 Comm: swapper/19 Kdump: loaded Tainted: G OE ------------ 3.10.0-957.el7.x86_64 #1
[ 198.893317] Hardware name: Phegda X2226A/S2600CW, BIOS SE5C610.86B.01.01.0024.021320181901 02/13/2018
[ 198.893388] task: ffff8a799ba0d140 ti: ffff8a799bad8000 task.ti: ffff8a799bad8000
[ 198.893447] RIP: 0010:[<ffffffff94cb7b02>] [<ffffffff94cb7b02>] __queue_work+0x32/0x3e0
[ 198.893518] RSP: 0018:ffff8a90dde43d80 EFLAGS: 00010046
[ 198.893561] RAX: 0000000000000082 RBX: 0000000000000086 RCX: 0000000000000000
[ 198.893617] RDX: ffff8a90b924fcb8 RSI: 0000000000000000 RDI: 000000000000001b
[ 198.893674] RBP: ffff8a90dde43db8 R08: ffff8a799ba0d6d8 R09: ffff8a90dde53900
[ 198.893730] R10: 0000000000000002 R11: ffff8a90dde43de8 R12: ffff8a90b924fcb8
[ 198.893786] R13: 000000000000001b R14: 0000000000000000 R15: ffff8a90d2890000
[ 198.893843] FS: 0000000000000000(0000) GS:ffff8a90dde40000(0000) knlGS:0000000000000000
[ 198.893905] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 198.893953] CR2: 0000000000000102 CR3: 0000001a70410000 CR4: 00000000001607e0
[ 198.894009] Call Trace:
[ 198.894058] [<ffffffff94cb8105>] queue_work_on+0x45/0x50
[ 198.894161] [<ffffffffc03f781e>] _hfi1_schedule_send+0x6e/0xc0 [hfi1]
[ 198.894244] [<ffffffffc03f78a2>] hfi1_schedule_send+0x32/0x70 [hfi1]
[ 198.894311] [<ffffffffc02cf2d9>] rvt_rc_timeout+0xe9/0x130 [rdmavt]
[ 198.894366] [<ffffffff94ce563a>] ? trigger_load_balance+0x6a/0x280
[ 198.894426] [<ffffffffc02cf1f0>] ? rvt_free_qpn+0x40/0x40 [rdmavt]
[ 198.894482] [<ffffffff94ca7f58>] call_timer_fn+0x38/0x110
[ 198.894537] [<ffffffffc02cf1f0>] ? rvt_free_qpn+0x40/0x40 [rdmavt]
[ 198.894589] [<ffffffff94caa3bd>] run_timer_softirq+0x24d/0x300
[ 198.894641] [<ffffffff94ca0f05>] __do_softirq+0xf5/0x280
[ 198.894689] [<ffffffff9537832c>] call_softirq+0x1c/0x30
[ 198.894736] [<ffffffff94c2e675>] do_softirq+0x65/0xa0
[ 198.894780] [<ffffffff94ca1285>] irq_exit+0x105/0x110
[ 198.894826] [<ffffffff953796c8>] smp_apic_timer_interrupt+0x48/0x60
[ 198.894881] [<ffffffff95375df2>] apic_timer_interrupt+0x162/0x170
[ 198.894930] <EOI>
[ 198.894955] [<ffffffff951adfb7>] ? cpuidle_enter_state+0x57/0xd0
[ 198.895012] [<ffffffff951ae10e>] cpuidle_idle_call+0xde/0x230
[ 198.895063] [<ffffffff94c366de>] arch_cpu_idle+0xe/0xc0
[ 198.896984] [<ffffffff94cfc3ba>] cpu_startup_entry+0x14a/0x1e0
[ 198.898879] [<ffffffff94c57db7>] start_secondary+0x1f7/0x270
[ 198.900750] [<ffffffff94c000d5>] start_cpu+0x5/0x14

The solution is to destroy the workqueue only when the hfi1 driver is
unloaded, not when the device is shut down.

Fixes: 8d3e71136a08 ("IB/{hfi1, qib}: Add handling of kernel restart")
Cc: <stable@vger.kernel.org>
Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Kaike Wan <kaike.wan@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/infiniband/hw/hfi1/init.c |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/init.c b/drivers/infiniband/hw/hfi1/init.c
index 3759d92..10fb5c6 100644
--- a/drivers/infiniband/hw/hfi1/init.c
+++ b/drivers/infiniband/hw/hfi1/init.c
@@ -829,6 +829,25 @@ static int create_workqueues(struct hfi1_devdata *dd)
 }
 
 /**
+ * destroy_workqueues - destroy per port workqueues
+ * @dd: the hfi1_ib device
+ */
+static void destroy_workqueues(struct hfi1_devdata *dd)
+{
+	int pidx;
+	struct hfi1_pportdata *ppd;
+
+	for (pidx = 0; pidx < dd->num_pports; ++pidx) {
+		ppd = dd->pport + pidx;
+
+		if (ppd->hfi1_wq) {
+			destroy_workqueue(ppd->hfi1_wq);
+			ppd->hfi1_wq = NULL;
+		}
+	}
+}
+
+/**
  * enable_general_intr() - Enable the IRQs that will be handled by the
  * general interrupt handler.
  * @dd: valid devdata
@@ -1102,10 +1121,6 @@ static void shutdown_device(struct hfi1_devdata *dd)
 		 */
 		hfi1_quiet_serdes(ppd);
 
-		if (ppd->hfi1_wq) {
-			destroy_workqueue(ppd->hfi1_wq);
-			ppd->hfi1_wq = NULL;
-		}
 		if (ppd->link_wq) {
 			destroy_workqueue(ppd->link_wq);
 			ppd->link_wq = NULL;
@@ -1757,6 +1772,7 @@ static void remove_one(struct pci_dev *pdev)
 	 * clear dma engines, etc.
 	 */
 	shutdown_device(dd);
+	destroy_workqueues(dd);
 
 	stop_timers(dd);
 

Re: [PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when the device is shut down

[Leon Romanovsky]

On Mon, May 11, 2020 at 11:13:15PM -0400, Dennis Dalessandro wrote:
> From: Kaike Wan <kaike.wan@intel.com>
>
> The workqueue hfi1_wq is destroyed in function shutdown_device(), which
> is called by either shutdown_one() or remove_one(). The function
> shutdown_one() is called when the kernel is rebooted while remove_one()
> is called when the hfi1 driver is unloaded. When the kernel is rebooted,
> hfi1_wq is destroyed while all qps are still active, leading to a
> kernel crash:

I was under impression that kernel reboot should follow same logic as
module removal. This is what graceful reboot will do anyway. Can you
please give me a link where I can read about difference in those flows?

Thanks

RE: [PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when the device is shut down

[Wan, Kaike]

> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> owner@vger.kernel.org> On Behalf Of Leon Romanovsky
> Sent: Tuesday, May 12, 2020 1:55 AM
> To: Dalessandro, Dennis <dennis.dalessandro@intel.com>
> Cc: jgg@ziepe.ca; dledford@redhat.com; linux-rdma@vger.kernel.org;
> Marciniszyn, Mike <mike.marciniszyn@intel.com>; stable@vger.kernel.org;
> Wan, Kaike <kaike.wan@intel.com>
> Subject: Re: [PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when
> the device is shut down
> 
> On Mon, May 11, 2020 at 11:13:15PM -0400, Dennis Dalessandro wrote:
> > From: Kaike Wan <kaike.wan@intel.com>
> >
> > The workqueue hfi1_wq is destroyed in function shutdown_device(),
> > which is called by either shutdown_one() or remove_one(). The function
> > shutdown_one() is called when the kernel is rebooted while
> > remove_one() is called when the hfi1 driver is unloaded. When the
> > kernel is rebooted, hfi1_wq is destroyed while all qps are still
> > active, leading to a kernel crash:
> 
> I was under impression that kernel reboot should follow same logic as
> module removal. This is what graceful reboot will do anyway. Can you please
> give me a link where I can read about difference in those flows?
> 
I used to think the same. However, by adding traces to the hfi driver, I found out that the shutdown function of the pci_driver was called when typing "reboot"  while the remove function  of the pci_driver was called when typing "modprobe -r hfi1". 

I am not an expert on kernel reboot and can someone give some hints?

Kaike

Re: [PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when the device is shut down

[Leon Romanovsky]

On Tue, May 12, 2020 at 11:52:34AM +0000, Wan, Kaike wrote:
>
>
> > -----Original Message-----
> > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > owner@vger.kernel.org> On Behalf Of Leon Romanovsky
> > Sent: Tuesday, May 12, 2020 1:55 AM
> > To: Dalessandro, Dennis <dennis.dalessandro@intel.com>
> > Cc: jgg@ziepe.ca; dledford@redhat.com; linux-rdma@vger.kernel.org;
> > Marciniszyn, Mike <mike.marciniszyn@intel.com>; stable@vger.kernel.org;
> > Wan, Kaike <kaike.wan@intel.com>
> > Subject: Re: [PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when
> > the device is shut down
> >
> > On Mon, May 11, 2020 at 11:13:15PM -0400, Dennis Dalessandro wrote:
> > > From: Kaike Wan <kaike.wan@intel.com>
> > >
> > > The workqueue hfi1_wq is destroyed in function shutdown_device(),
> > > which is called by either shutdown_one() or remove_one(). The function
> > > shutdown_one() is called when the kernel is rebooted while
> > > remove_one() is called when the hfi1 driver is unloaded. When the
> > > kernel is rebooted, hfi1_wq is destroyed while all qps are still
> > > active, leading to a kernel crash:
> >
> > I was under impression that kernel reboot should follow same logic as
> > module removal. This is what graceful reboot will do anyway. Can you please
> > give me a link where I can read about difference in those flows?
> >
> I used to think the same. However, by adding traces to the hfi driver, I found out that the shutdown function of the pci_driver was called when typing "reboot"  while the remove function  of the pci_driver was called when typing "modprobe -r hfi1".

I took a look on what mlx5_core is doing in shutdown flow and it can be
summarized in the following:
1. Drain workqueues
2. Close PCI
3. Don't release anything.

So maybe you didn't flush the hfi1_wq?

>
> I am not an expert on kernel reboot and can someone give some hints?
>
> Kaike
>
>
>
>

RE: [PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when the device is shut down

[Wan, Kaike]

> -----Original Message-----
> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> owner@vger.kernel.org> On Behalf Of Leon Romanovsky
> Sent: Wednesday, May 13, 2020 3:59 AM
> To: Wan, Kaike <kaike.wan@intel.com>
> Cc: Dalessandro, Dennis <dennis.dalessandro@intel.com>; jgg@ziepe.ca;
> dledford@redhat.com; linux-rdma@vger.kernel.org; Marciniszyn, Mike
> <mike.marciniszyn@intel.com>; stable@vger.kernel.org
> Subject: Re: [PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when
> the device is shut down
> 
> On Tue, May 12, 2020 at 11:52:34AM +0000, Wan, Kaike wrote:
> >
> >
> > > -----Original Message-----
> > > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > > owner@vger.kernel.org> On Behalf Of Leon Romanovsky
> > > Sent: Tuesday, May 12, 2020 1:55 AM
> > > To: Dalessandro, Dennis <dennis.dalessandro@intel.com>
> > > Cc: jgg@ziepe.ca; dledford@redhat.com; linux-rdma@vger.kernel.org;
> > > Marciniszyn, Mike <mike.marciniszyn@intel.com>;
> > > stable@vger.kernel.org; Wan, Kaike <kaike.wan@intel.com>
> > > Subject: Re: [PATCH for-rc or next 1/3] IB/hfi1: Do not destroy
> > > hfi1_wq when the device is shut down
> > >
> > > On Mon, May 11, 2020 at 11:13:15PM -0400, Dennis Dalessandro wrote:
> > > > From: Kaike Wan <kaike.wan@intel.com>
> > > >
> > > > The workqueue hfi1_wq is destroyed in function shutdown_device(),
> > > > which is called by either shutdown_one() or remove_one(). The
> > > > function
> > > > shutdown_one() is called when the kernel is rebooted while
> > > > remove_one() is called when the hfi1 driver is unloaded. When the
> > > > kernel is rebooted, hfi1_wq is destroyed while all qps are still
> > > > active, leading to a kernel crash:
> > >
> > > I was under impression that kernel reboot should follow same logic
> > > as module removal. This is what graceful reboot will do anyway. Can
> > > you please give me a link where I can read about difference in those
> flows?
> > >
> > I used to think the same. However, by adding traces to the hfi driver, I
> found out that the shutdown function of the pci_driver was called when
> typing "reboot"  while the remove function  of the pci_driver was called
> when typing "modprobe -r hfi1".
> 
> I took a look on what mlx5_core is doing in shutdown flow and it can be
> summarized in the following:
> 1. Drain workqueues
> 2. Close PCI
> 3. Don't release anything.
> 
> So maybe you didn't flush the hfi1_wq?
Will add the flush.

Thanks,

Kaike
> >
> >
> >

Re: [PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when the device is shut down

[Jason Gunthorpe]

On Mon, May 11, 2020 at 11:13:15PM -0400, Dennis Dalessandro wrote:
> From: Kaike Wan <kaike.wan@intel.com>
> 
> The workqueue hfi1_wq is destroyed in function shutdown_device(), which
> is called by either shutdown_one() or remove_one(). The function
> shutdown_one() is called when the kernel is rebooted while remove_one()
> is called when the hfi1 driver is unloaded. When the kernel is rebooted,
> hfi1_wq is destroyed while all qps are still active, leading to a
> kernel crash:

AFAIK the purpose of shutdown is to stop all in progress DMAs.

If devices are wildly doing DMA during the shutdown process then all
manner of things can fail, including kexecing into another kernel.

Do you achive that with these shutdown handlers?

It does make sense that the work queue would not be destroyed in
shutdown, but I'm surprised it doesn't flush it?

Jason

RE: [PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when the device is shut down

[Wan, Kaike]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, May 19, 2020 8:27 PM
> To: Dalessandro, Dennis <dennis.dalessandro@intel.com>
> Cc: dledford@redhat.com; linux-rdma@vger.kernel.org; Marciniszyn, Mike
> <mike.marciniszyn@intel.com>; stable@vger.kernel.org; Wan, Kaike
> <kaike.wan@intel.com>
> Subject: Re: [PATCH for-rc or next 1/3] IB/hfi1: Do not destroy hfi1_wq when
> the device is shut down
> 
> On Mon, May 11, 2020 at 11:13:15PM -0400, Dennis Dalessandro wrote:
> > From: Kaike Wan <kaike.wan@intel.com>
> >
> > The workqueue hfi1_wq is destroyed in function shutdown_device(),
> > which is called by either shutdown_one() or remove_one(). The function
> > shutdown_one() is called when the kernel is rebooted while
> > remove_one() is called when the hfi1 driver is unloaded. When the
> > kernel is rebooted, hfi1_wq is destroyed while all qps are still
> > active, leading to a kernel crash:
> 
> AFAIK the purpose of shutdown is to stop all in progress DMAs.
> 
> If devices are wildly doing DMA during the shutdown process then all manner
> of things can fail, including kexecing into another kernel.
> 
> Do you achive that with these shutdown handlers?

We did try to shut down the hardware and will address some software issues in next revision.

> 
> It does make sense that the work queue would not be destroyed in
> shutdown, but I'm surprised it doesn't flush it?
Will do.

Kaike

[PATCH for-rc or next 2/3] IB/hfi1: Do not destroy link_wq when the device is shut down

[Dennis Dalessandro]

From: Kaike Wan <kaike.wan@intel.com>

The workqueue link_wq should only be destroyed when the hfi1 driver
is unloaded, not when the device is shut down.

Fixes: 71d47008ca1b ("IB/hfi1: Create workqueue for link events")
Cc: <stable@vger.kernel.org>
Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Kaike Wan <kaike.wan@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/infiniband/hw/hfi1/init.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/init.c b/drivers/infiniband/hw/hfi1/init.c
index 10fb5c6..49a4566 100644
--- a/drivers/infiniband/hw/hfi1/init.c
+++ b/drivers/infiniband/hw/hfi1/init.c
@@ -844,6 +844,10 @@ static void destroy_workqueues(struct hfi1_devdata *dd)
 			destroy_workqueue(ppd->hfi1_wq);
 			ppd->hfi1_wq = NULL;
 		}
+		if (ppd->link_wq) {
+			destroy_workqueue(ppd->link_wq);
+			ppd->link_wq = NULL;
+		}
 	}
 }
 
@@ -1120,11 +1124,6 @@ static void shutdown_device(struct hfi1_devdata *dd)
 		 * We can't count on interrupts since we are stopping.
 		 */
 		hfi1_quiet_serdes(ppd);
-
-		if (ppd->link_wq) {
-			destroy_workqueue(ppd->link_wq);
-			ppd->link_wq = NULL;
-		}
 	}
 	sdma_exit(dd);
 }

Re: [PATCH for-rc or next 2/3] IB/hfi1: Do not destroy link_wq when the device is shut down

[Leon Romanovsky]

On Mon, May 11, 2020 at 11:13:22PM -0400, Dennis Dalessandro wrote:
> From: Kaike Wan <kaike.wan@intel.com>
>
> The workqueue link_wq should only be destroyed when the hfi1 driver
> is unloaded, not when the device is shut down.

It really doesn't make sense to keep workqueue if no devices exist.

Thanks

RE: [PATCH for-rc or next 2/3] IB/hfi1: Do not destroy link_wq when the device is shut down

[Wan, Kaike]

> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Tuesday, May 12, 2020 1:57 AM
> To: Dalessandro, Dennis <dennis.dalessandro@intel.com>
> Cc: jgg@ziepe.ca; dledford@redhat.com; linux-rdma@vger.kernel.org;
> Marciniszyn, Mike <mike.marciniszyn@intel.com>; stable@vger.kernel.org;
> Wan, Kaike <kaike.wan@intel.com>
> Subject: Re: [PATCH for-rc or next 2/3] IB/hfi1: Do not destroy link_wq when
> the device is shut down
> 
> On Mon, May 11, 2020 at 11:13:22PM -0400, Dennis Dalessandro wrote:
> > From: Kaike Wan <kaike.wan@intel.com>
> >
> > The workqueue link_wq should only be destroyed when the hfi1 driver is
> > unloaded, not when the device is shut down.
> 
> It really doesn't make sense to keep workqueue if no devices exist.
> 
Only to keep consistent with the other workqueue: hfi1_wq. Both workqueues are created when the probe function of the pci_driver is called and destroyed when the remove function is called. In addition, we try not to free any data structure in the shutdown function, which is intended to change the power state of a device before reboot (include/linux/pci.h comment).

Kaike

[PATCH for-rc or next 3/3] IB/qib: Call kobject_put() when kobject_init_and_add() fails

[Dennis Dalessandro]

From: Kaike Wan <kaike.wan@intel.com>

When kobject_init_and_add() returns an error in the function
qib_create_port_files(), the function kobject_put() is not called for
the corresponding kobject, which potentially leads to memory leak.

This patch fixes the issue by calling kobject_put() even if
kobject_init_and_add() fails. In addition, the ppd->diagc_kobj is
released along with other kobjects when the sysfs is unregistered.

Fixes: f931551bafe1 ("IB/qib: Add new qib driver for QLogic PCIe InfiniBand adapters")
Cc: <stable@vger.kernel.org>
Suggested-by: Lin Yi <teroincn@gmail.com>
Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Signed-off-by: Kaike Wan <kaike.wan@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
---
 drivers/infiniband/hw/qib/qib_sysfs.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_sysfs.c b/drivers/infiniband/hw/qib/qib_sysfs.c
index 568b21e..021df06 100644
--- a/drivers/infiniband/hw/qib/qib_sysfs.c
+++ b/drivers/infiniband/hw/qib/qib_sysfs.c
@@ -760,7 +760,7 @@ int qib_create_port_files(struct ib_device *ibdev, u8 port_num,
 		qib_dev_err(dd,
 			"Skipping linkcontrol sysfs info, (err %d) port %u\n",
 			ret, port_num);
-		goto bail;
+		goto bail_link;
 	}
 	kobject_uevent(&ppd->pport_kobj, KOBJ_ADD);
 
@@ -770,7 +770,7 @@ int qib_create_port_files(struct ib_device *ibdev, u8 port_num,
 		qib_dev_err(dd,
 			"Skipping sl2vl sysfs info, (err %d) port %u\n",
 			ret, port_num);
-		goto bail_link;
+		goto bail_sl;
 	}
 	kobject_uevent(&ppd->sl2vl_kobj, KOBJ_ADD);
 
@@ -780,7 +780,7 @@ int qib_create_port_files(struct ib_device *ibdev, u8 port_num,
 		qib_dev_err(dd,
 			"Skipping diag_counters sysfs info, (err %d) port %u\n",
 			ret, port_num);
-		goto bail_sl;
+		goto bail_diagc;
 	}
 	kobject_uevent(&ppd->diagc_kobj, KOBJ_ADD);
 
@@ -793,7 +793,7 @@ int qib_create_port_files(struct ib_device *ibdev, u8 port_num,
 		qib_dev_err(dd,
 		 "Skipping Congestion Control sysfs info, (err %d) port %u\n",
 		 ret, port_num);
-		goto bail_diagc;
+		goto bail_cc;
 	}
 
 	kobject_uevent(&ppd->pport_cc_kobj, KOBJ_ADD);
@@ -854,6 +854,7 @@ void qib_verbs_unregister_sysfs(struct qib_devdata *dd)
 				&cc_table_bin_attr);
 			kobject_put(&ppd->pport_cc_kobj);
 		}
+		kobject_put(&ppd->diagc_kobj);
 		kobject_put(&ppd->sl2vl_kobj);
 		kobject_put(&ppd->pport_kobj);
 	}

Re: [PATCH for-rc or next 3/3] IB/qib: Call kobject_put() when kobject_init_and_add() fails

[Leon Romanovsky]

On Mon, May 11, 2020 at 11:13:28PM -0400, Dennis Dalessandro wrote:
> From: Kaike Wan <kaike.wan@intel.com>
>
> When kobject_init_and_add() returns an error in the function
> qib_create_port_files(), the function kobject_put() is not called for
> the corresponding kobject, which potentially leads to memory leak.
>
> This patch fixes the issue by calling kobject_put() even if
> kobject_init_and_add() fails. In addition, the ppd->diagc_kobj is
> released along with other kobjects when the sysfs is unregistered.
>
> Fixes: f931551bafe1 ("IB/qib: Add new qib driver for QLogic PCIe InfiniBand adapters")
> Cc: <stable@vger.kernel.org>
> Suggested-by: Lin Yi <teroincn@gmail.com>
> Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
> Signed-off-by: Kaike Wan <kaike.wan@intel.com>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
> ---
>  drivers/infiniband/hw/qib/qib_sysfs.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>

It is not "even if", the kobject_put() must be called if kobject_init_and_add() fails.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@mellanox.com>

Re: [PATCH for-rc or next 3/3] IB/qib: Call kobject_put() when kobject_init_and_add() fails

[Jason Gunthorpe]

On Mon, May 11, 2020 at 11:13:28PM -0400, Dennis Dalessandro wrote:
> From: Kaike Wan <kaike.wan@intel.com>
> 
> When kobject_init_and_add() returns an error in the function
> qib_create_port_files(), the function kobject_put() is not called for
> the corresponding kobject, which potentially leads to memory leak.
> 
> This patch fixes the issue by calling kobject_put() even if
> kobject_init_and_add() fails. In addition, the ppd->diagc_kobj is
> released along with other kobjects when the sysfs is unregistered.
> 
> Fixes: f931551bafe1 ("IB/qib: Add new qib driver for QLogic PCIe InfiniBand adapters")
> Cc: <stable@vger.kernel.org>
> Suggested-by: Lin Yi <teroincn@gmail.com>
> Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
> Signed-off-by: Kaike Wan <kaike.wan@intel.com>
> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
> Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/infiniband/hw/qib/qib_sysfs.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Applied to for-rc

Are you respinning the other two patches?

Thanks,
Jason

Re: [PATCH for-rc or next 3/3] IB/qib: Call kobject_put() when kobject_init_and_add() fails

[Dennis Dalessandro]

On 5/19/2020 8:00 PM, Jason Gunthorpe wrote:
> On Mon, May 11, 2020 at 11:13:28PM -0400, Dennis Dalessandro wrote:
>> From: Kaike Wan <kaike.wan@intel.com>
>>
>> When kobject_init_and_add() returns an error in the function
>> qib_create_port_files(), the function kobject_put() is not called for
>> the corresponding kobject, which potentially leads to memory leak.
>>
>> This patch fixes the issue by calling kobject_put() even if
>> kobject_init_and_add() fails. In addition, the ppd->diagc_kobj is
>> released along with other kobjects when the sysfs is unregistered.
>>
>> Fixes: f931551bafe1 ("IB/qib: Add new qib driver for QLogic PCIe InfiniBand adapters")
>> Cc: <stable@vger.kernel.org>
>> Suggested-by: Lin Yi <teroincn@gmail.com>
>> Reviewed-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
>> Signed-off-by: Kaike Wan <kaike.wan@intel.com>
>> Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
>> Reviewed-by: Leon Romanovsky <leonro@mellanox.com>
>> ---
>>   drivers/infiniband/hw/qib/qib_sysfs.c |    9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> Applied to for-rc
> 
> Are you respinning the other two patches?

Yes, Kaike is working on getting updates to those two out.

-Denny