linux-kernel - Re: [PATCH v19 2/3] scsi: ufs: L2P map management for HPB read

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]

Date:   Fri, 05 Feb 2021 12:15:39 +0100
From:   Bean Huo <huobean@...il.com>
To:     daejun7.park@...sung.com, Greg KH <gregkh@...uxfoundation.org>,
        "avri.altman@....com" <avri.altman@....com>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
        "stanley.chu@...iatek.com" <stanley.chu@...iatek.com>,
        "cang@...eaurora.org" <cang@...eaurora.org>,
        "bvanassche@....org" <bvanassche@....org>,
        ALIM AKHTAR <alim.akhtar@...sung.com>
Cc:     "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Sung-Jun Park <sungjun07.park@...sung.com>,
        yongmyung lee <ymhungry.lee@...sung.com>,
        Jinyoung CHOI <j-young.choi@...sung.com>,
        BoRam Shin <boram.shin@...sung.com>,
        SEUNGUK SHIN <seunguk.shin@...sung.com>
Subject: Re: [PATCH v19 2/3] scsi: ufs: L2P map management for HPB read

On Fri, 2021-01-29 at 14:30 +0900, Daejun Park wrote:
> +static void ufshpb_set_read_buf_cmd(unsigned char *cdb, int rgn_idx,
> +                                          int srgn_idx, int
> srgn_mem_size)
> +{
> +       cdb[0] = UFSHPB_READ_BUFFER;
> +       cdb[1] = UFSHPB_READ_BUFFER_ID;
> +
> +       put_unaligned_be16(rgn_idx, &cdb[2]);
> +       put_unaligned_be16(srgn_idx, &cdb[4]);
> +       put_unaligned_be24(srgn_mem_size, &cdb[6]);
> +
> +       cdb[9] = 0x00;
> +}
> +
> +static int ufshpb_execute_map_req(struct ufshpb_lu *hpb,
> +                                 struct ufshpb_req *map_req)
> +{
> +       struct request_queue *q;
> +       struct request *req;
> +       struct scsi_request *rq;
> +       int ret = 0;
> +       int i;
> +
> +       q = hpb->sdev_ufs_lu->request_queue;
> +       for (i = 0; i < hpb->pages_per_srgn; i++) {
> +               ret = bio_add_pc_page(q, map_req->bio, map_req->mctx-
> >m_page[i],
> +                                     PAGE_SIZE, 0);
> +               if (ret != PAGE_SIZE) {
> +                       dev_err(&hpb->sdev_ufs_lu->sdev_dev,
> +                                  "bio_add_pc_page fail %d - %d\n",
> +                                  map_req->rgn_idx, map_req-
> >srgn_idx);
> +                       return ret;
> +               }
> +       }
> +
> +       req = map_req->req;
> +
> +       blk_rq_append_bio(req, &map_req->bio);
> +
> +       req->end_io_data = map_req;
> +
> +       rq = scsi_req(req);
> +       ufshpb_set_read_buf_cmd(rq->cmd, map_req->rgn_idx,
> +                               map_req->srgn_idx, hpb-
> >srgn_mem_size);

Hi Daejun

Thanks for your hard-working on the HPB driver.

I found you didn't take into account of allocation Length of the last
sub-region of the last region.

UFS HPB spec: 

"If the requested field of the HPB Region or HPB Sub-Region is out of
range, then the device shall terminate the command by sending RESPONSE
UPIU with CHECK CONDITION status, with the SENSE KEY set to ILLEGAL
REQUEST, and the additional sense code set to INVALID FIELD IN CDB"


Below codes are from my RFC HPB patchset:

https://patchwork.kernel.org/project/linux-scsi/patch/20200504142032.16619-6-beanhuo@micron.com/

+	alloc_len = hpb->hba->hpb_geo.subregion_entry_sz;
+	/*
+	 * HPB Sub-Regions are equally sized except for the last one
which is
+	 * smaller if the last hpb Region is not an integer multiple of
+	 * bHPBSubRegionSize.
+	 */
+	if (map_req->region == (hpb->lu_region_cnt - 1) &&
+	    map_req->subregion == (hpb->subregions_in_last_region - 1))
+		alloc_len = hpb->last_subregion_entry_size;
+
+	ufshpb_prepare_read_buf_cmd(ureq->cmd, map_req->region,
+				    map_req->subregion, alloc_len);
+	if (!ureq->req) {
+		ureq->req = blk_get_request(q, REQ_OP_SCSI_IN, 0);
+		if (IS_ERR(ureq->req)) {
+			ret =  PTR_ERR(ureq->req);
+			goto free_mem;
+		}
+	}

please fix it in your next version patch. thanks.


Kind regards,
Bean