| 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
| ||
|
Date: Tue, 7 Jul 2020 08:45:00 -0700
From: Dave Jiang <dave.jiang@...el.com>
To: Peter Ujfalusi <peter.ujfalusi@...com>,
Jiri Slaby <jirislaby@...nel.org>, vkoul@...nel.org
Cc: Swathi Kovvuri <swathi.kovvuri@...el.com>,
dmaengine@...r.kernel.org,
Linux kernel mailing list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] dmaengine: check device and channel list for empty
On 7/7/2020 1:50 AM, Peter Ujfalusi wrote:
>
>
> On 07/07/2020 9.05, Jiri Slaby wrote:
>> On 26. 06. 20, 20:09, Dave Jiang wrote:
>>> Check dma device list and channel list for empty before iterate as the
>>> iteration function assume the list to be not empty. With devices and
>>> channels now being hot pluggable this is a condition that needs to be
>>> checked. Otherwise it can cause the iterator to spin forever.
>>
>> Could you be a little bit more specific how this can spin forever? I.e.
>> can you attach a stacktrace of such a behaviour?
>>
>> As in the empty case, "&pos->member" is "head" (look into
>> list_for_each_entry) and the for loop should loop exactly zero times.
>
> This is my understanding as well.
>
> Isn't it more plausible that you have race between
> dma_async_device_register() / dma_async_device_unregister() /
> dma_async_device_channel_register() /
> dma_async_device_channel_unregister() ?
>
> It looks like that there is unbalanced locking between
> dma_async_device_channel_register() and
> dma_async_device_channel_unregister().
>
> The later locks the dma_list_mutex for a short while, while the former
> does not.
> Both device_register/unregister locks the same dma_list_mutex in some point.
It is possible there's a race as well in addition to the issue that the patch
fixes. I'll take a look and see if there's an additional fix for the unbalanced
locking. Thanks for checking Peter.
>
>>> Fixes: e81274cd6b52 ("dmaengine: add support to dynamic register/unregister of channels")
>>> Reported-by: Swathi Kovvuri <swathi.kovvuri@...el.com>
>>> Signed-off-by: Dave Jiang <dave.jiang@...el.com>
>>> Tested-by: Swathi Kovvuri <swathi.kovvuri@...el.com>
>>> ---
>>>
>>> Rebased to dmaengine next tree
>>>
>>> drivers/dma/dmaengine.c | 119 +++++++++++++++++++++++++++++++++++++----------
>>> 1 file changed, 94 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>>> index 2b06a7a8629d..0d6529eff66f 100644
>>> --- a/drivers/dma/dmaengine.c>> +++ b/drivers/dma/dmaengine.c
>
> ...
>
>>> +static int dma_channel_enumeration(struct dma_device *device)
>>> +{
>>> + struct dma_chan *chan;
>>> + int rc;
>>> +
>>> + if (list_empty(&device->channels))
>>> + return 0;
>>> +
>>> + /* represent channels in sysfs. Probably want devs too */
>>> + list_for_each_entry(chan, &device->channels, device_node) {
>>> + rc = __dma_async_device_channel_register(device, chan);
>>> + if (rc < 0)
>>> + return rc;
>>> + }
>>> +
>>> + /* take references on public channels */
>>> + if (dmaengine_ref_count && !dma_has_cap(DMA_PRIVATE, device->cap_mask))
>>> + list_for_each_entry(chan, &device->channels, device_node) {
>>> + /* if clients are already waiting for channels we need
>>> + * to take references on their behalf
>>> + */
>>> + if (dma_chan_get(chan) == -ENODEV) {
>>> + /* note we can only get here for the first
>>> + * channel as the remaining channels are
>>> + * guaranteed to get a reference
>>> + */
>>> + return -ENODEV;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> /**
>>> * dma_async_device_register - registers DMA devices found
>>> * @device: pointer to &struct dma_device
>>> @@ -1247,33 +1330,15 @@ int dma_async_device_register(struct dma_device *device)
>>> if (rc != 0)
>>> return rc;
>>>
>>> + mutex_lock(&dma_list_mutex);
>>> mutex_init(&device->chan_mutex);
>>> ida_init(&device->chan_ida);
>>> -
>>> - /* represent channels in sysfs. Probably want devs too */
>>> - list_for_each_entry(chan, &device->channels, device_node) {
>>> - rc = __dma_async_device_channel_register(device, chan);
>>> - if (rc < 0)
>>> - goto err_out;
>>>
>>> + rc = dma_channel_enumeration(device);
>>> + if (rc < 0) {
>>> + mutex_unlock(&dma_list_mutex);
>>> + goto err_out;
>>> }
>
> Here you effectively moved the __dma_async_device_channel_register()
> under dma_list_mutex.
>
>
>>>
>>> - mutex_lock(&dma_list_mutex);
>>> - /* take references on public channels */
>>> - if (dmaengine_ref_count && !dma_has_cap(DMA_PRIVATE, device->cap_mask))
>>> - list_for_each_entry(chan, &device->channels, device_node) {
>>> - /* if clients are already waiting for channels we need
>>> - * to take references on their behalf
>>> - */
>>> - if (dma_chan_get(chan) == -ENODEV) {
>>> - /* note we can only get here for the first
>>> - * channel as the remaining channels are
>>> - * guaranteed to get a reference
>>> - */
>>> - rc = -ENODEV;
>>> - mutex_unlock(&dma_list_mutex);
>>> - goto err_out;
>>> - }
>>> - }
>>> list_add_tail_rcu(&device->global_node, &dma_device_list);
>>> if (dma_has_cap(DMA_PRIVATE, device->cap_mask))
>>> device->privatecnt++; /* Always private */
>>> @@ -1291,6 +1356,9 @@ int dma_async_device_register(struct dma_device *device)
>>> return rc;
>>> }
>>>
>>> + if (list_empty(&device->channels))
>>> + return rc;
>>> +
>>> list_for_each_entry(chan, &device->channels, device_node) {
>>> if (chan->local == NULL)
>>> continue;
>>> @@ -1317,8 +1385,9 @@ void dma_async_device_unregister(struct dma_device *device)
>>>
>>> dmaengine_debug_unregister(device);
>>>
>>> - list_for_each_entry_safe(chan, n, &device->channels, device_node)
>>> - __dma_async_device_channel_unregister(device, chan);
>>> + if (!list_empty(&device->channels))
>>> + list_for_each_entry_safe(chan, n, &device->channels, device_node)
>>> + __dma_async_device_channel_unregister(device, chan);
>>>
>>> mutex_lock(&dma_list_mutex);
>>> /*
>>
>>
>>
>
> - Péter
>
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
Powered by blists - more mailing lists