linux-kernel - Re: [PATCH v15 2/4] dmaengine: tegra: Add tegra gpcdma driver

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: <6241cfa3-dd7e-012c-3687-daad0aa4631d@gmail.com>
Date:   Mon, 20 Dec 2021 19:30:37 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Akhil R <akhilrajeev@...dia.com>
Cc:     Pavan Kunapuli <pkunapuli@...dia.com>,
        "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "dmaengine@...r.kernel.org" <dmaengine@...r.kernel.org>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Krishna Yarlagadda <kyarlagadda@...dia.com>,
        Laxman Dewangan <ldewangan@...dia.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        Rajesh Gumasta <rgumasta@...dia.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "thierry.reding@...il.com" <thierry.reding@...il.com>,
        "vkoul@...nel.org" <vkoul@...nel.org>
Subject: Re: [PATCH v15 2/4] dmaengine: tegra: Add tegra gpcdma driver

20.12.2021 18:23, Akhil R пишет:
>> 16.12.2021 20:11, Akhil R пишет:
>>> +static int tegra_dma_terminate_all(struct dma_chan *dc) {
>>> +     struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>> +     unsigned long wcount = 0;
>>> +     unsigned long status;
>>> +     unsigned long flags;
>>> +     int err;
>>> +
>>> +     raw_spin_lock_irqsave(&tdc->lock, flags);
>>> +
>>> +     if (!tdc->dma_desc) {
>>> +             raw_spin_unlock_irqrestore(&tdc->lock, flags);
>>> +             return 0;
>>> +     }
>>> +
>>> +     if (!tdc->busy)
>>> +             goto skip_dma_stop;
>>> +
>>> +     if (tdc->tdma->chip_data->hw_support_pause)
>>> +             err = tegra_dma_pause(tdc);
>>> +     else
>>> +             err = tegra_dma_stop_client(tdc);
>>> +
>>> +     if (err) {
>>> +             raw_spin_unlock_irqrestore(&tdc->lock, flags);
>>> +             return err;
>>> +     }
>>> +
>>> +     status = tdc_read(tdc, TEGRA_GPCDMA_CHAN_STATUS);
>>> +     if (status & TEGRA_GPCDMA_STATUS_ISE_EOC) {
>>> +             dev_dbg(tdc2dev(tdc), "%s():handling isr\n", __func__);
>>> +             tegra_dma_xfer_complete(tdc);
>>> +             status = tdc_read(tdc, TEGRA_GPCDMA_CHAN_STATUS);
>>> +     }
>>> +
>>> +     wcount = tdc_read(tdc, TEGRA_GPCDMA_CHAN_XFER_COUNT);
>>> +     tegra_dma_stop(tdc);
>>> +
>>> +     tdc->dma_desc->bytes_transferred +=
>>> +                     tdc->dma_desc->bytes_requested - (wcount * 4);
>>> +
>>> +skip_dma_stop:
>>> +     tegra_dma_sid_free(tdc);
>>> +     vchan_free_chan_resources(&tdc->vc);
>>> +     kfree(&tdc->vc);
>>
>> You really going to kfree the head of tegra_dma_channel here? Once again, this
>> code was 100% untested :/
> I did validate this using DMATEST which did not show any error.
> https://www.kernel.org/doc/html/latest/driver-api/dmaengine/dmatest.html
> Do you suggest something better?
> 
>> You're not allowed to free channel from the dma_terminate_all() callback. This
>> callback terminates submitted descs, that's it.
>>
> Sorry, I am relatively new to DMA framework (probably you get it from the patch 
> version no. :)). I read your previous comment as to use tdc->vc instead of dma_desc.
> I would learn a bit more and update with a change. Thanks for the inputs.

Looks like DMATEST doesn't try to terminate in a middle of transfer and
then check that further transfers work. You may try to extend DMATEST or
simulate I2C error to test it, you should also test it with enabled KASAN.