linux-kernel - RE: [PATCH v2] scsi: ufs: Zero utp_upiu_req at the beginning of each command

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:
 <DM6PR04MB657549C63703AECEA2169CC4FC612@DM6PR04MB6575.namprd04.prod.outlook.com>
Date: Tue, 17 Sep 2024 06:52:26 +0000
From: Avri Altman <Avri.Altman@....com>
To: Bart Van Assche <bvanassche@....org>, "Martin K . Petersen"
	<martin.petersen@...cle.com>
CC: "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] scsi: ufs: Zero utp_upiu_req at the beginning of each
 command

> 
> On 9/15/24 12:48 AM, Avri Altman wrote:
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index 8ea5a82503a9..1f6575afc1c5 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -2761,7 +2761,6 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct
> ufshcd_lrb *lrbp, u8 upiu_flags)
> >       ucd_req_ptr->sc.exp_data_transfer_len =
> > cpu_to_be32(cmd->sdb.length);
> >
> >       cdb_len = min_t(unsigned short, cmd->cmd_len, UFS_CDB_SIZE);
> > -     memset(ucd_req_ptr->sc.cdb, 0, UFS_CDB_SIZE);
> >       memcpy(ucd_req_ptr->sc.cdb, cmd->cmnd, cdb_len);
> >
> >       memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp)); @@
> > -2834,6 +2833,8 @@ static int ufshcd_compose_devman_upiu(struct ufs_hba
> *hba,
> >       u8 upiu_flags;
> >       int ret = 0;
> >
> > +     memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr));
> > +
> >       ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE,
> > 0);
> >
> >       if (hba->dev_cmd.type == DEV_CMD_TYPE_QUERY) @@ -2858,6 +2859,8
> > @@ static void ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb
> *lrbp)
> >       unsigned int ioprio_class = IOPRIO_PRIO_CLASS(req_get_ioprio(rq));
> >       u8 upiu_flags;
> >
> > +     memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr));
> > +
> >       ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, lrbp->cmd-
> >sc_data_direction, 0);
> >       if (ioprio_class == IOPRIO_CLASS_RT)
> >               upiu_flags |= UPIU_CMD_FLAGS_CP; @@ -7165,6 +7168,8 @@
> > static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
> >
> >       ufshcd_setup_dev_cmd(hba, lrbp, cmd_type, 0, tag);
> >
> > +     memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr));
> > +
> >       ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE,
> > 0);
> >
> >       /* update the task tag in the request upiu */ @@ -7317,6 +7322,8
> > @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct
> > utp_upiu_req *r
> >
> >       ufshcd_setup_dev_cmd(hba, lrbp, DEV_CMD_TYPE_RPMB,
> > UFS_UPIU_RPMB_WLUN, tag);
> >
> > +     memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr));
> > +
> >       ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE,
> > ehs);
> >
> >       /* update the task tag */
> 
> Something unfortunate about the above patch is that it spreads the initialization
> of *lrbp->ucd_req_ptr over two functions. Wouldn't it be better to have all the
> code that initializes *lrbp->ucd_req_ptr in the same function, e.g. as in the
> untested patch below?
It does that for the 4 different required instances: command, query, raw query, and rpmb extended header.
Is the below proposal evidently better? e.g. with respect of efficiency, simplicity, readability etc.?

Thanks,
Avri

> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 8d90af6434da..9d826e5d610b 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2770,13 +2770,14 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct
> ufshcd_lrb *lrbp, u8 upiu_flags)
>                 .task_tag = lrbp->task_tag,
>                 .command_set_type = UPIU_COMMAND_SET_TYPE_SCSI,
>         };
> +       memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) -
> +              sizeof(ucd_req_ptr->header));
> 
>         WARN_ON_ONCE(ucd_req_ptr->header.task_tag != lrbp->task_tag);
> 
>         ucd_req_ptr->sc.exp_data_transfer_len = cpu_to_be32(cmd->sdb.length);
> 
>         cdb_len = min_t(unsigned short, cmd->cmd_len, UFS_CDB_SIZE);
> -       memset(ucd_req_ptr->sc.cdb, 0, UFS_CDB_SIZE);
>         memcpy(ucd_req_ptr->sc.cdb, cmd->cmnd, cdb_len);
> 
>         memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp)); @@ -2809,6
> +2810,8 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba
> *hba,
>                                 cpu_to_be16(len) :
>                                 0,
>         };
> +       memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) -
> +              sizeof(ucd_req_ptr->header));
> 
>         /* Copy the Query Request buffer as is */
>         memcpy(&ucd_req_ptr->qr, &query->request.upiu_req, @@ -2825,12
> +2828,12 @@ static inline void ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb
> *lrbp)
>   {
>         struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr;
> 
> -       memset(ucd_req_ptr, 0, sizeof(struct utp_upiu_req));
> -
>         ucd_req_ptr->header = (struct utp_upiu_header){
>                 .transaction_code = UPIU_TRANSACTION_NOP_OUT,
>                 .task_tag = lrbp->task_tag,
>         };
> +       memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) -
> +              sizeof(ucd_req_ptr->header));
> 
>         memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
>   }
> 
> Thanks,
> 
> Bart.