Hello Justin Tee,
Commit 4281f44ea8bf ("scsi: lpfc: Prevent NDLP reference count
underflow in dev_loss_tmo callback") from Oct 31, 2024 (linux-next),
leads to the following Smatch static checker warning:
drivers/scsi/lpfc/lpfc_hbadisc.c:220 lpfc_dev_loss_tmo_callbk() error: dereferencing freed memory 'ndlp' (line 211)
// presumably these are fine
drivers/scsi/lpfc/lpfc_els.c:1021 lpfc_cmpl_els_flogi() error: dereferencing freed memory 'ndlp' (line 1019)
drivers/scsi/lpfc/lpfc_els.c:1021 lpfc_cmpl_els_flogi() error: dereferencing freed memory 'ndlp' (line 1019)
drivers/scsi/lpfc/lpfc_els.c:4993 lpfc_els_retry() error: we previously assumed 'ndlp' could be null (see line 4970)
drivers/scsi/lpfc/lpfc_els.c:5278 lpfc_mbx_cmpl_dflt_rpi() error: dereferencing freed memory 'ndlp' (line 5277)
drivers/scsi/lpfc/lpfc_els.c:8634 lpfc_els_rsp_rls_acc() error: dereferencing freed memory 'ndlp' (line 8602)
drivers/scsi/lpfc/lpfc_nvme.c:2654 lpfc_nvme_unregister_port() error: dereferencing freed memory 'ndlp' (line 2642)
drivers/scsi/lpfc/lpfc_nportdisc.c:1875 lpfc_rcv_logo_reglogin_issue() error: dereferencing freed memory 'ndlp' (line 1867)
// this one has a comment saying it's fine
drivers/scsi/lpfc/lpfc_hbadisc.c:4335 lpfc_mbx_cmpl_ns_reg_login() error: dereferencing freed memory 'ndlp' (line 4327)
drivers/scsi/lpfc/lpfc_hbadisc.c
156 void
157 lpfc_dev_loss_tmo_callbk(struct fc_rport *rport)
158 {
159 struct lpfc_nodelist *ndlp;
160 struct lpfc_vport *vport;
161 struct lpfc_hba *phba;
162 struct lpfc_work_evt *evtp;
163 unsigned long iflags;
164 bool nvme_reg = false;
165
166 ndlp = ((struct lpfc_rport_data *)rport->dd_data)->pnode;
167 if (!ndlp)
168 return;
169
170 vport = ndlp->vport;
171 phba = vport->phba;
172
173 lpfc_debugfs_disc_trc(vport, LPFC_DISC_TRC_RPORT,
174 "rport devlosscb: sid:x%x did:x%x flg:x%lx",
175 ndlp->nlp_sid, ndlp->nlp_DID, ndlp->nlp_flag);
176
177 lpfc_printf_vlog(ndlp->vport, KERN_INFO, LOG_NODE,
178 "3181 dev_loss_callbk x%06x, rport x%px flg x%lx "
179 "load_flag x%lx refcnt %u state %d xpt x%x\n",
180 ndlp->nlp_DID, ndlp->rport, ndlp->nlp_flag,
181 vport->load_flag, kref_read(&ndlp->kref),
182 ndlp->nlp_state, ndlp->fc4_xpt_flags);
183
184 /* Don't schedule a worker thread event if the vport is going down. */
185 if (test_bit(FC_UNLOADING, &vport->load_flag) ||
186 !test_bit(HBA_SETUP, &phba->hba_flag)) {
187
188 spin_lock_irqsave(&ndlp->lock, iflags);
189 ndlp->rport = NULL;
190
191 if (ndlp->fc4_xpt_flags & NVME_XPT_REGD)
192 nvme_reg = true;
193
194 /* The scsi_transport is done with the rport so lpfc cannot
195 * call to unregister.
196 */
197 if (ndlp->fc4_xpt_flags & SCSI_XPT_REGD) {
198 ndlp->fc4_xpt_flags &= ~SCSI_XPT_REGD;
199
200 /* If NLP_XPT_REGD was cleared in lpfc_nlp_unreg_node,
201 * unregister calls were made to the scsi and nvme
202 * transports and refcnt was already decremented. Clear
203 * the NLP_XPT_REGD flag only if the NVME Rport is
204 * confirmed unregistered.
205 */
206 if (!nvme_reg && ndlp->fc4_xpt_flags & NLP_XPT_REGD) {
207 ndlp->fc4_xpt_flags &= ~NLP_XPT_REGD;
208 spin_unlock_irqrestore(&ndlp->lock, iflags);
209 lpfc_nlp_put(ndlp); /* may free ndlp */
If nvme_reg is false then sometimes we free ndlp. I would normally
ignore this warning because obviously the code is doing something
subtle, but there is a comment which says that it does free ndlp.
210 } else {
211 spin_unlock_irqrestore(&ndlp->lock, iflags);
212 }
213 } else {
214 spin_unlock_irqrestore(&ndlp->lock, iflags);
215 }
216
217 /* Only 1 thread can drop the initial node reference. If
218 * another thread has set NLP_DROPPED, this thread is done.
219 */
--> 220 if (nvme_reg || test_bit(NLP_DROPPED, &ndlp->nlp_flag))
^^^^
Here we know nvme_reg is false but we're still dereferencing "ndlp".
221 return;
222
223 set_bit(NLP_DROPPED, &ndlp->nlp_flag);
^^^^
Here nvme_reg is false again but this is even more dereferencing.
224 lpfc_nlp_put(ndlp);
^^^^
And here.
225 return;
226 }
227
regards,
dan carpenter
