linux-kernel - RE: [PATCH v16 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM5PR12MB1850D67F9B5640943F1AEB2EC05B9@DM5PR12MB1850.namprd12.prod.outlook.com>
Date:   Fri, 21 Jan 2022 16:24:37 +0000
From:   Akhil R <akhilrajeev@...dia.com>
To:     Dmitry Osipenko <digetx@...il.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>
CC:     Pavan Kunapuli <pkunapuli@...dia.com>
Subject: RE: [PATCH v16 2/4] dmaengine: tegra: Add tegra gpcdma driver

> >>>>> +static int tegra_dma_terminate_all(struct dma_chan *dc)
> >>>>> +{
> >>>>> +     struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> >>>>> +     unsigned long flags;
> >>>>> +     LIST_HEAD(head);
> >>>>> +     int err;
> >>>>> +
> >>>>> +     if (tdc->dma_desc) {
> >>>>
> >>>> Needs locking protection against racing with the interrupt handler.
> >>> tegra_dma_stop_client() waits for the in-flight transfer
> >>> to complete and prevents any additional transfer to start.
> >>> Wouldn't it manage the race? Do you see any potential issue there?
> >>
> >> You should consider interrupt handler like a process running in a
> >> parallel thread. The interrupt handler sets tdc->dma_desc to NULL, hence
> >> you'll get NULL dereference in tegra_dma_stop_client().
> >
> > Is it better if I remove the below part from tegra_dma_stop_client() so
> > that dma_desc is not accessed at all?
> >
> > +     wcount = tdc_read(tdc, TEGRA_GPCDMA_CHAN_XFER_COUNT);
> > +     tdc->dma_desc->bytes_transferred +=
> > +                     tdc->dma_desc->bytes_requested - (wcount * 4);
> >
> > Because I don't see a point in updating the value there. dma_desc is set
> > to NULL in the next step in terminate_all() anyway.
> 
> That isn't going help you much because you also can't release DMA
> descriptor while interrupt handler still may be running and using that
> descriptor.

Does the below functions look good to resolve the issue, provided
tegra_dma_stop_client() doesn't access dma_desc?

+static int tegra_dma_terminate_all(struct dma_chan *dc)
+{
+       struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
+       unsigned long flags;
+       LIST_HEAD(head);
+       int err;
+
+       err = tegra_dma_stop_client(tdc);
+       if (err)
+               return err;
+
+       tegra_dma_stop(tdc);
+
+       spin_lock_irqsave(&tdc->vc.lock, flags);
+       tegra_dma_sid_free(tdc);
+       tdc->dma_desc = NULL;
+
+       vchan_get_all_descriptors(&tdc->vc, &head);
+       spin_unlock_irqrestore(&tdc->vc.lock, flags);
+
+       vchan_dma_desc_free_list(&tdc->vc, &head);
+
+       return 0;
+}

+static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
+{
+       struct tegra_dma_channel *tdc = dev_id;
+       struct tegra_dma_desc *dma_desc = tdc->dma_desc;
+       struct tegra_dma_sg_req *sg_req;
+       u32 status;
+
+       /* Check channel error status register */
+       status = tdc_read(tdc, TEGRA_GPCDMA_CHAN_ERR_STATUS);
+       if (status) {
+               tegra_dma_chan_decode_error(tdc, status);
+               tegra_dma_dump_chan_regs(tdc);
+               tdc_write(tdc, TEGRA_GPCDMA_CHAN_ERR_STATUS, 0xFFFFFFFF);
+       }
+
+       status = tdc_read(tdc, TEGRA_GPCDMA_CHAN_STATUS);
+       if (!(status & TEGRA_GPCDMA_STATUS_ISE_EOC))
+               return IRQ_HANDLED;
+
+       tdc_write(tdc, TEGRA_GPCDMA_CHAN_STATUS,
+                 TEGRA_GPCDMA_STATUS_ISE_EOC);
+
+       spin_lock(&tdc->vc.lock);
+       if (!dma_desc)
+               goto irq_done;
+
+       sg_req = dma_desc->sg_req;
+       dma_desc->bytes_transferred += sg_req[dma_desc->sg_idx - 1].len;
+
+       if (dma_desc->sg_idx == dma_desc->sg_count) {
+               tegra_dma_xfer_complete(tdc);
+       } else if (dma_desc->cyclic) {
+               vchan_cyclic_callback(&dma_desc->vd);
+               tegra_dma_configure_next_sg(tdc);
+       } else {
+               tegra_dma_start(tdc);
+       }
+
+irq_done:
+       spin_unlock(&tdc->vc.lock);
+       return IRQ_HANDLED;
+}


Thanks,
Akhil