linux-kernel - Re: [PATCH v11 11/14] crypto: iaa - Add support for deflate-iaa compression algorithm

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: <82bf4e15a8aa31a63d6bdb070b81d9e2dc2c7a78.camel@linux.intel.com>
Date:   Tue, 05 Dec 2023 15:18:20 -0600
From:   Tom Zanussi <tom.zanussi@...ux.intel.com>
To:     Rex Zhang <rex.zhang@...el.com>
Cc:     dave.jiang@...el.com, davem@...emloft.net,
        dmaengine@...r.kernel.org, fenghua.yu@...el.com,
        giovanni.cabiddu@...el.com, herbert@...dor.apana.org.au,
        james.guilford@...el.com, kanchana.p.sridhar@...el.com,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        pavel@....cz, tony.luck@...el.com, vinodh.gopal@...el.com,
        vkoul@...nel.org, wajdi.k.feghali@...el.com
Subject: Re: [PATCH v11 11/14] crypto: iaa - Add support for deflate-iaa
 compression algorithm

Hi Rex,

On Tue, 2023-12-05 at 10:26 +0800, Rex Zhang wrote:
> Hi Tom,
> 
> On 2023-12-04 at 15:41:46 -0600, Tom Zanussi wrote:
> > Hi Rex,
> > 
> > On Mon, 2023-12-04 at 23:00 +0800, Rex Zhang wrote:
> > > Hi, Tom,
> > > 
> > > On 2023-12-01 at 14:10:32 -0600, Tom Zanussi wrote:
> > > 
> > > [snip]
> > > 
> > > > +static int iaa_wq_put(struct idxd_wq *wq)
> > > > +{
> > > > +       struct idxd_device *idxd = wq->idxd;
> > > > +       struct iaa_wq *iaa_wq;
> > > > +       bool free = false;
> > > > +       int ret = 0;
> > > > +
> > > > +       spin_lock(&idxd->dev_lock);
> > > > +       iaa_wq = idxd_wq_get_private(wq);
> > > > +       if (iaa_wq) {
> > > > +               iaa_wq->ref--;
> > > > +               if (iaa_wq->ref == 0 && iaa_wq->remove) {
> > > > +                       __free_iaa_wq(iaa_wq);
> > > > +                       idxd_wq_set_private(wq, NULL);
> > > > +                       free = true;
> > > > +               }
> > > > +               idxd_wq_put(wq);
> > > > +       } else {
> > > > +               ret = -ENODEV;
> > > > +       }
> > > > +       spin_unlock(&idxd->dev_lock);
> > > __free_iaa_wq() may cause schedule, whether it should be move out
> > > of
> > > the
> > > context between spin_lock() and spin_unlock()?
> > 
> > Yeah, I suppose it makes more sense to have it below anyway, will
> > move
> > it there.
> > 
> > > > +       if (free)
> > > > +               kfree(iaa_wq);
> > > > +
> > > > +       return ret;
> > > > +}
> > > 
> > > [snip]
> > > 
> > > > @@ -800,12 +1762,38 @@ static void iaa_crypto_remove(struct
> > > > idxd_dev *idxd_dev)
> > > >  
> > > >         remove_iaa_wq(wq);
> > > >  
> > > > +       spin_lock(&idxd->dev_lock);
> > > > +       iaa_wq = idxd_wq_get_private(wq);
> > > > +       if (!iaa_wq) {
> > > > +               spin_unlock(&idxd->dev_lock);
> > > > +               pr_err("%s: no iaa_wq available to remove\n",
> > > > __func__);
> > > > +               goto out;
> > > > +       }
> > > > +
> > > > +       if (iaa_wq->ref) {
> > > > +               iaa_wq->remove = true;
> > > > +       } else {
> > > > +               wq = iaa_wq->wq;
> > > > +               __free_iaa_wq(iaa_wq);
> > > > +               idxd_wq_set_private(wq, NULL);
> > > > +               free = true;
> > > > +       }
> > > > +       spin_unlock(&idxd->dev_lock);
> > > __free_iaa_wq() may cause schedule, whether it should be move out
> > > of
> > > the
> > > context between spin_lock() and spin_unlock()?
> > 
> > Same.
> > 
> > > > +
> > > > +       if (free)
> > > > +               kfree(iaa_wq);
> > > > +
> > > >         idxd_drv_disable_wq(wq);
> > > >         rebalance_wq_table();
> > > >  
> > > > -       if (nr_iaa == 0)
> > > > +       if (nr_iaa == 0) {
> > > > +               iaa_crypto_enabled = false;
> > > Is it necessary to add iaa_unregister_compression_device() here?
> > > All iaa devices are disabled cause the variable first_wq will be
> > > true,
> > > if enable wq, iaa_register_compression_device() will fail due to
> > > the
> > > algorithm is existed.
> > 
> > No, this is required by review input from a previous version - the
> > compression device can only be unregistered on module exit.
> Do it mean disabling all WQs followed by enabling WQ is unacceptable?
> User must do "rmmod iaa_crypto" before enabling WQ in this case.
> 

Right, as mentioned in the documentation:

+If a different configuration or set of driver attributes is required,
+the user must first disable the IAA devices and workqueues, reset the
+configuration, and then re-register the deflate-iaa algorithm with the
+crypto subsystem by removing and reinserting the iaa_crypto module.

Thanks,

Tom

> Thanks.
> > 
> > Thanks,
> > 
> > Tom
> > 
> > > >                 free_wq_table();
> > > > +               module_put(THIS_MODULE);
> > > >  
> > > > +               pr_info("iaa_crypto now DISABLED\n");
> > > > +       }
> > > > +out:
> > > >         mutex_unlock(&iaa_devices_lock);
> > > >         mutex_unlock(&wq->wq_lock);
> > > >  }
> > > 
> > > [snip]
> > > 
> > > Thanks,
> > > Rex Zhang
> > > > -- 
> > > > 2.34.1
> > > > 
> >