linux-kernel - Re: crypto: ccp - Release dma channels before dmaengine unrgister

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: <d5d1785d-b2a8-6913-78ba-d686d70f2a64@amd.com>
Date:   Wed, 31 Aug 2022 07:54:02 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Koba Ko <koba.ko@...onical.com>
Cc:     John Allen <john.allen@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: crypto: ccp - Release dma channels before dmaengine unrgister

On 8/31/22 00:39, Koba Ko wrote:
> On Tue, Aug 30, 2022 at 11:32 PM Tom Lendacky <thomas.lendacky@....com> wrote:
>>
>> On 8/30/22 04:34, Koba Ko wrote:
>>> A warning is shown during shutdown,
>>>
>>> __dma_async_device_channel_unregister called while 2 clients hold a reference
>>> WARNING: CPU: 15 PID: 1 at drivers/dma/dmaengine.c:1110
>>> __dma_async_device_channel_unregister
>>>
>>> Call dma_release_channel for occupied channles
>>> before dma_async_device_unregister.
>>>
>>> Fixes: 4cbe9bc34ed0 ("crypto: ccp - ccp_dmaengine_unregister release dma channels")
>>> Signed-off-by: Koba Ko <koba.ko@...onical.com>
>>> ---
>>>    drivers/crypto/ccp/ccp-dmaengine.c | 8 +++++++-
>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/crypto/ccp/ccp-dmaengine.c b/drivers/crypto/ccp/ccp-dmaengine.c
>>> index 7d4b4ad1db1f3..ba3eb1ac3b55d 100644
>>> --- a/drivers/crypto/ccp/ccp-dmaengine.c
>>> +++ b/drivers/crypto/ccp/ccp-dmaengine.c
>>> @@ -641,6 +641,10 @@ static void ccp_dma_release(struct ccp_device *ccp)
>>>        for (i = 0; i < ccp->cmd_q_count; i++) {
>>>                chan = ccp->ccp_dma_chan + i;
>>>                dma_chan = &chan->dma_chan;
>>> +
>>> +             if (dma_chan->client_count)
>>> +                     dma_release_channel(dma_chan);
>>> +
>>>                tasklet_kill(&chan->cleanup_tasklet);
>>>                list_del_rcu(&dma_chan->device_node);
>>>        }
>>> @@ -762,12 +766,14 @@ int ccp_dmaengine_register(struct ccp_device *ccp)
>>>    void ccp_dmaengine_unregister(struct ccp_device *ccp)
>>>    {
>>>        struct dma_device *dma_dev = &ccp->dma_dev;
>>> +     struct dma_chan *dma_chan;
>>> +     unsigned int i;
>>>
>>>        if (!dmaengine)
>>>                return;
>>>
>>> -     dma_async_device_unregister(dma_dev);
>>>        ccp_dma_release(ccp);
>>> +     dma_async_device_unregister(dma_dev);
>>
>> Seems the right way to get rid of the warn, but can there still be a race
>> where the channel is grabbed again after ccp_dma_release() and before
>> dma_async_device_unregister() is called?
> 
> After tracing the ccp driver,
> dma_device is held by the ccp device.
> the only function called ccp_dmaengine_register is ccp5_init/ccp_init,
> ccp_dma_release is called in ccp_dmaengine_unregister/ccp_dmaengine_register,
> so there's no race condition in current codes.

I'm referring to some other piece of software claiming the channel, like 
the raid driver or the AMD NTB driver. This just seems racy to me, but it 
does address the WARNing.

Clean up the kernel test robot complaints and submit a v2.

Thanks,
Tom

> 
> 
>>
>> Thanks,
>> Tom
>>
>>>
>>>        kmem_cache_destroy(ccp->dma_desc_cache);
>>>        kmem_cache_destroy(ccp->dma_cmd_cache);