linux-kernel - RE: [PATCH v1 1/2] scsi: ufs: Support WriteBooster on Samsung UFS devices

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] [day] [month] [year] [list]
Message-ID: <1590996325.25636.30.camel@mtkswgap22>
Date:   Mon, 1 Jun 2020 15:25:25 +0800
From:   Stanley Chu <stanley.chu@...iatek.com>
To:     Avri Altman <Avri.Altman@....com>
CC:     "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "alim.akhtar@...sung.com" <alim.akhtar@...sung.com>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "beanhuo@...ron.com" <beanhuo@...ron.com>,
        "asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
        "cang@...eaurora.org" <cang@...eaurora.org>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "bvanassche@....org" <bvanassche@....org>,
        "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kuohong.wang@...iatek.com" <kuohong.wang@...iatek.com>,
        "peter.wang@...iatek.com" <peter.wang@...iatek.com>,
        "chun-hung.wu@...iatek.com" <chun-hung.wu@...iatek.com>,
        "andy.teng@...iatek.com" <andy.teng@...iatek.com>,
        "cc.chou@...iatek.com" <cc.chou@...iatek.com>,
        "chaotian.jing@...iatek.com" <chaotian.jing@...iatek.com>
Subject: RE: [PATCH v1 1/2] scsi: ufs: Support WriteBooster on Samsung UFS
 devices

Hi Avri,

On Sat, 2020-05-30 at 20:37 +0000, Avri Altman wrote:
> > @@ -2801,11 +2801,17 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum
> > query_opcode opcode,
> >  {
> >         struct ufs_query_req *request = NULL;
> >         struct ufs_query_res *response = NULL;
> > -       int err, selector = 0;
> > +       int err;
> >         int timeout = QUERY_REQ_TIMEOUT;
> > +       u8 selector = 0;
> > 
> >         BUG_ON(!hba);
> > 
> > +       if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) {
> > +               if (ufshcd_is_wb_flags(idn))
> > +                       selector = 1;
> > +       }
> > +
> Why not make the caller set the applicable selector,
> Instead of checking this for every flag?

This way have the minimum modification efforts and places compared to
other ways. However it looks a little wired because the selector control
is better assigned by users. I will submit next version with changing
the way selector assigned for comparison.

> 
> >         ufshcd_hold(hba, false);
> >         mutex_lock(&hba->dev_cmd.lock);
> >         ufshcd_init_query(hba, &request, &response, opcode, idn, index,
> > @@ -2882,6 +2888,11 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum
> > query_opcode opcode,
> >                 goto out;
> >         }
> > 
> > +       if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) {
> > +               if (ufshcd_is_wb_attrs(idn))
> > +                       selector = 1;
> > +       }
> > +
> Same here
> 
> >         mutex_lock(&hba->dev_cmd.lock);
> >         ufshcd_init_query(hba, &request, &response, opcode, idn, index,
> >                         selector);
> > @@ -3042,6 +3053,11 @@ int ufshcd_query_descriptor_retry(struct ufs_hba
> > *hba,
> >         int err;
> >         int retries;
> > 
> > +       if (hba->dev_quirks & UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR) {
> > +               if (ufshcd_is_wb_desc(idn, index))
> > +                       selector = 1;
> > +       }
> > +
> And here.
> But this can't be true - 
> Are you setting the selector = 1 for reading any field for those descriptors?
> Shouldn't it be for the wb specific fields?

Yes, thanks for remind this.
I shall assign selector = 1 for WB related fields only in descriptors.

>  
> 
> >         for (retries = QUERY_REQ_RETRIES; retries > 0; retries--) {
> >                 err = __ufshcd_query_descriptor(hba, opcode, idn, index,
> >                                                 selector, desc_buf, buf_len);
> > @@ -6907,8 +6923,10 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
> >         size_t buff_len;
> >         u8 model_index;
> >         u8 *desc_buf;
> > +       u8 retry_cnt = 0;
> >         struct ufs_dev_info *dev_info = &hba->dev_info;
> > 
> > +retry:
> >         buff_len = max_t(size_t, hba->desc_size.dev_desc,
> >                          QUERY_DESC_MAX_SIZE + 1);
> >         desc_buf = kmalloc(buff_len, GFP_KERNEL);
> > @@ -6948,6 +6966,29 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
> > 
> >         ufs_fixup_device_setup(hba);
> > 
> > +       if (!retry_cnt && (hba->dev_quirks &
> > +               UFS_DEVICE_QUIRK_WB_SPECIAL_SELECTOR)) {
> If you only want to enter this clause once - you should use something other than retry_cnt,
> Which the reader expects to performs retries....

OK! I will fix this label by using another more comprehensible name.
> 
> Also, this is becoming too wired - 
> From your commit log I get that for specific Samsung devices,
> You need to query wb descriptor fields/attributes/flags using selectore = 1.
> But what it has to do with descriptor sizes?

Sorry to not mention clearly in the commit log.

Here driver needs to update the descriptor size to a "longer size" which
includes the "hidden WB related fields" which can be "found" by selector
= 1.

If descriptor size is not updated, any query can only get the fields
offset within current descriptor size even if selector = 1, and
out-of-boundary desc_buf[] access will happen in
ufshcd_read_desc_param().

PS. The check of "param_offset" to prevent possible out-of-boundary
desc_buf[] access can be patched as well.


Thanks,
Stanley Chu