linux-kernel - Re: [PATCH v3 2/4] nvme-fc: reorganize ctrl ref-counting and cleanup code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3b3bf8fc-592c-4732-8985-629e3baeaa34@flourine.local>
Date: Wed, 3 Sep 2025 09:34:26 +0200
From: Daniel Wagner <dwagner@...e.de>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: oe-kbuild@...ts.linux.dev, Daniel Wagner <wagi@...nel.org>, 
	Keith Busch <kbusch@...nel.org>, Christoph Hellwig <hch@....de>, Sagi Grimberg <sagi@...mberg.me>, 
	James Smart <james.smart@...adcom.com>, lkp@...el.com, oe-kbuild-all@...ts.linux.dev, 
	Shinichiro Kawasaki <shinichiro.kawasaki@....com>, Hannes Reinecke <hare@...e.de>, linux-nvme@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/4] nvme-fc: reorganize ctrl ref-counting and cleanup
 code

Hi Dan,

On Mon, Sep 01, 2025 at 02:46:07PM +0300, Dan Carpenter wrote:
> Hi Daniel,
> 
> kernel test robot noticed the following build warnings:
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Daniel-Wagner/nvme-fabrics-introduce-ref-counting-for-nvmf_ctrl_options/20250829-234513
> base:   c17b750b3ad9f45f2b6f7e6f7f4679844244f0b9
> patch link:    https://lore.kernel.org/r/20250829-nvme-fc-sync-v3-2-d69c87e63aee%40kernel.org
> patch subject: [PATCH v3 2/4] nvme-fc: reorganize ctrl ref-counting and cleanup code
> config: x86_64-randconfig-161-20250830 (https://download.01.org/0day-ci/archive/20250831/202508310442.lBWBZ5AC-lkp@intel.com/config)
> compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@...el.com>
> | Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> | Closes: https://lore.kernel.org/r/202508310442.lBWBZ5AC-lkp@intel.com/
> 
> New smatch warnings:
> drivers/nvme/host/fc.c:1492 nvme_fc_match_disconn_ls() warn: iterator used outside loop: 'ctrl'
> 
> Old smatch warnings:
> drivers/nvme/host/fc.c:3180 nvme_fc_delete_association() warn: mixing irqsave and irq
> 
> vim +/ctrl +1492 drivers/nvme/host/fc.c
> 
> 14fd1e98afafc0 James Smart 2020-03-31  1465  static struct nvme_fc_ctrl *
> 14fd1e98afafc0 James Smart 2020-03-31  1466  nvme_fc_match_disconn_ls(struct nvme_fc_rport *rport,
> 14fd1e98afafc0 James Smart 2020-03-31  1467  		      struct nvmefc_ls_rcv_op *lsop)
> 14fd1e98afafc0 James Smart 2020-03-31  1468  {
> 14fd1e98afafc0 James Smart 2020-03-31  1469  	struct fcnvme_ls_disconnect_assoc_rqst *rqst =
> 14fd1e98afafc0 James Smart 2020-03-31  1470  					&lsop->rqstbuf->rq_dis_assoc;
> 14fd1e98afafc0 James Smart 2020-03-31  1471  	struct nvme_fc_ctrl *ctrl, *ret = NULL;
> 14fd1e98afafc0 James Smart 2020-03-31  1472  	struct nvmefc_ls_rcv_op *oldls = NULL;
> 14fd1e98afafc0 James Smart 2020-03-31  1473  	u64 association_id = be64_to_cpu(rqst->associd.association_id);
> 14fd1e98afafc0 James Smart 2020-03-31  1474  	unsigned long flags;
> 14fd1e98afafc0 James Smart 2020-03-31  1475  
> 14fd1e98afafc0 James Smart 2020-03-31  1476  	spin_lock_irqsave(&rport->lock, flags);
> 14fd1e98afafc0 James Smart 2020-03-31  1477  
> 14fd1e98afafc0 James Smart 2020-03-31  1478  	list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
> 14fd1e98afafc0 James Smart 2020-03-31  1479  		spin_lock(&ctrl->lock);
> 14fd1e98afafc0 James Smart 2020-03-31  1480  		if (association_id == ctrl->association_id) {
> 14fd1e98afafc0 James Smart 2020-03-31  1481  			oldls = ctrl->rcv_disconn;
> 14fd1e98afafc0 James Smart 2020-03-31  1482  			ctrl->rcv_disconn = lsop;
> 14fd1e98afafc0 James Smart 2020-03-31  1483  			ret = ctrl;
> 
> There should maybe be a break statement here?

Hmm, the commit 14fd1e98afafc0 has a break after the spin_unlock.

+               spin_unlock(&ctrl->lock);
+               if (ret)
+                       /* leave the ctrl get reference */
+                       break;

And since then there aren't any changes applied.

> 14fd1e98afafc0 James Smart 2020-03-31  1484  		}
> 14fd1e98afafc0 James Smart 2020-03-31  1485  		spin_unlock(&ctrl->lock);
> 14fd1e98afafc0 James Smart 2020-03-31  1486  	}
> 14fd1e98afafc0 James Smart 2020-03-31  1487  
> 14fd1e98afafc0 James Smart 2020-03-31  1488  	spin_unlock_irqrestore(&rport->lock, flags);
> 14fd1e98afafc0 James Smart 2020-03-31  1489  
> 14fd1e98afafc0 James Smart 2020-03-31  1490  	/* transmit a response for anything that was pending */
> 14fd1e98afafc0 James Smart 2020-03-31  1491  	if (oldls) {
> 14fd1e98afafc0 James Smart 2020-03-31 @1492  		dev_info(rport->lport->dev,
> 14fd1e98afafc0 James Smart 2020-03-31  1493  			"NVME-FC{%d}: Multiple Disconnect Association "
> 14fd1e98afafc0 James Smart 2020-03-31  1494  			"LS's received\n", ctrl->cnum);
> 
> ctrl->cnum is always an invalid pointer on this line.  Another
> option would be to print ret->cnum instead ctrl->nmum.

Yes, this makes sense. I'll send a patch for this. Thanks!

Thanks,
Daniel