Hello Dan,
Please find my comments inline.
-----Original Message-----
From: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Sent: Friday, August 21, 2020 6:22 PM
To: Javed Hasan <jhasan@xxxxxxxxxxx>
Cc: linux-scsi@xxxxxxxxxxxxxxx
Subject: [EXT] [bug report] scsi: libfc: Free skb in fc_disc_gpn_id_resp() for valid cases
Hello Javed Hasan,
The patch ec007ef40abb: "scsi: libfc: Free skb in
fc_disc_gpn_id_resp() for valid cases" from Jul 29, 2020, leads to the following static checker warning:
drivers/scsi/libfc/fc_disc.c:638 fc_disc_gpn_id_resp()
warn: '&fp->skb' double freed
drivers/scsi/libfc/fc_disc.c
568 static void fc_disc_gpn_id_resp(struct fc_seq *sp, struct fc_frame *fp,
569 void *rdata_arg)
570 {
571 struct fc_rport_priv *rdata = rdata_arg;
572 struct fc_rport_priv *new_rdata;
573 struct fc_lport *lport;
574 struct fc_disc *disc;
575 struct fc_ct_hdr *cp;
576 struct fc_ns_gid_pn *pn;
577 u64 port_name;
578
579 lport = rdata->local_port;
580 disc = &lport->disc;
581
582 if (PTR_ERR(fp) == -FC_EX_CLOSED)
583 goto out;
584 if (IS_ERR(fp)) {
585 mutex_lock(&disc->disc_mutex);
586 fc_disc_restart(disc);
587 mutex_unlock(&disc->disc_mutex);
This call to fc_disc_restart(disc); was added in the commit, but it wasn't mentioned in the commit message so I suspect it was committed by mistake.
[JH] : This is the part of the fix. I just moved the fc_disc_restart here for case IS_ERR(), where we need to do fc_disc_restart() without doing free of "fp".
588 goto out;
589 }
590
591 cp = fc_frame_payload_get(fp, sizeof(*cp));
592 if (!cp)
593 goto redisc;
594 if (ntohs(cp->ct_cmd) == FC_FS_ACC) {
595 if (fr_len(fp) < sizeof(struct fc_frame_header) +
596 sizeof(*cp) + sizeof(*pn))
597 goto redisc;
598 pn = (struct fc_ns_gid_pn *)(cp + 1);
599 port_name = get_unaligned_be64(&pn->fn_wwpn);
600 mutex_lock(&rdata->rp_mutex);
601 if (rdata->ids.port_name == -1)
602 rdata->ids.port_name = port_name;
603 else if (rdata->ids.port_name != port_name) {
604 FC_DISC_DBG(disc, "GPN_ID accepted. WWPN changed. "
605 "Port-id %6.6x wwpn %16.16llx\n",
606 rdata->ids.port_id, port_name);
607 mutex_unlock(&rdata->rp_mutex);
608 fc_rport_logoff(rdata);
609 mutex_lock(&lport->disc.disc_mutex);
610 new_rdata = fc_rport_create(lport, rdata->ids.port_id);
611 mutex_unlock(&lport->disc.disc_mutex);
612 if (new_rdata) {
613 new_rdata->disc_id = disc->disc_id;
614 fc_rport_login(new_rdata);
615 }
616 goto free_fp;
617 }
618 rdata->disc_id = disc->disc_id;
619 mutex_unlock(&rdata->rp_mutex);
620 fc_rport_login(rdata);
621 } else if (ntohs(cp->ct_cmd) == FC_FS_RJT) {
622 FC_DISC_DBG(disc, "GPN_ID rejected reason %x exp %x\n",
623 cp->ct_reason, cp->ct_explan);
624 fc_rport_logoff(rdata);
625 } else {
626 FC_DISC_DBG(disc, "GPN_ID unexpected response code %x\n",
627 ntohs(cp->ct_cmd));
628 redisc:
629 mutex_lock(&disc->disc_mutex);
630 fc_disc_restart(disc);
631 mutex_unlock(&disc->disc_mutex);
632 }
633 free_fp:
634 fc_frame_free(fp);
^^^^^^^^^^^^^^^^^
Then this free was added.
635 out:
636 kref_put(&rdata->kref, fc_rport_destroy);
637 if (!IS_ERR(fp))
638 fc_frame_free(fp);
^^^^^^^^^^^^^^^^^ But there was already a free here so it was a double free.
[JH] : I removed this if() section. Now there is only one fc_frame_free() which is present just after free_fp:
639 }
[JH] : Thank you for pointing out.
regards,
dan carpenter
