| 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
| ||
|
Message-ID: <BLUPR03MB3738381E274C507DA43E778F56A0@BLUPR03MB373.namprd03.prod.outlook.com>
Date: Thu, 18 Dec 2014 02:29:07 +0000
From: "fugang.duan@...escale.com" <fugang.duan@...escale.com>
To: Russell King - ARM Linux <linux@....linux.org.uk>
CC: David Miller <davem@...emloft.net>,
"Fabio.Estevam@...escale.com" <Fabio.Estevam@...escale.com>,
"ezequiel.garcia@...e-electrons.com"
<ezequiel.garcia@...e-electrons.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: Bug: mv643xxx fails with highmem
From: Russell King - ARM Linux <linux@....linux.org.uk> Sent: Tuesday, December 16, 2014 6:57 PM
> To: Duan Fugang-B38611
> Cc: David Miller; Estevam Fabio-R49496; ezequiel.garcia@...e-
> electrons.com; netdev@...r.kernel.org
> Subject: Re: Bug: mv643xxx fails with highmem
>
> On Tue, Dec 16, 2014 at 02:19:38AM +0000, fugang.duan@...escale.com wrote:
> > From: Russell King - ARM Linux <linux@....linux.org.uk> Sent: Tuesday,
> December 16, 2014 2:05 AM
> > > To: Duan Fugang-B38611
> > > Cc: David Miller; Estevam Fabio-R49496; ezequiel.garcia@...e-
> > > electrons.com; netdev@...r.kernel.org
> > > Subject: Re: Bug: mv643xxx fails with highmem
> > >
> > > On Fri, Dec 12, 2014 at 05:34:01AM +0000, fugang.duan@...escale.com
> wrote:
> > > > I will submit one patch to fix the issue.
> > >
> > > There's more bugs in the FEC driver... here's the relevant bits:
> > >
> > > static void
> > > fec_enet_tx_queue(struct net_device *ndev, u16 queue_id) {
> > > bdp = txq->dirty_tx;
> > >
> > > bdp = fec_enet_get_nextdesc(bdp, fep, queue_id);
> > >
> > > while (((status = bdp->cbd_sc) & BD_ENET_TX_READY) == 0) {
> > > /* current queue is empty */
> > > if (bdp == txq->cur_tx)
> > > break;
> > >
> > > skb = txq->tx_skbuff[index];
> > > txq->tx_skbuff[index] = NULL;
> > > if (!IS_TSO_HEADER(txq, bdp->cbd_bufaddr))
> > > dma_unmap_single(&fep->pdev->dev, bdp-
> > > >cbd_bufaddr,
> > > bdp->cbd_datlen,
> DMA_TO_DEVICE);
> > > bdp->cbd_bufaddr = 0;
> > > if (!skb) {
> > > bdp = fec_enet_get_nextdesc(bdp, fep,
> queue_id);
> > > continue;
> > > }
> > > ...
> > > txq->dirty_tx = bdp;
> > > bdp = fec_enet_get_nextdesc(bdp, fep, queue_id);
> > > }
> > >
> > > Consider the following code path:
> > > - we enter this function
> > > - get the dirty_tx pointer
> > > - move to the next descriptor (which we'll call descriptor A)
> > > - next descriptor indicates that TX_READY = 0
> > > - bdp != txq->cur_tx
> > > - we unmap if needed
> > > - we set bdp->cmdbufaddr = 0
> > > - assume skb is NULL, so we move to the next descriptor (we'll call
> this
> > > B)
> > > - next descriptor _may_ have TX_READY = 1
> > > - we break out of the loop, and return
> > >
> > > Some time later, we re-enter:
> > > - get the dirty_tx pointer
> > > - move to the next descriptor (which is descriptor A above)
> > > - next descriptor indicates that TX_READY = 0
> > > - bdp != txq->cur_tx
> > > - we call dma_unmap_single(..., bdp->cbd_bufaddr, which we previously
> > > zeroed
> > > - the DMA API debugging complains that FEC is unmapping memory
> which it
> > > doesn't own
> > >
> > > Unfortunately, this does appear to happen - from a paste from Jon
> > > Nettleton from iMX6Q:
> > >
> > > 32. [ 45.033001] unmapping this address 0x0 size 66
> 33. [ 45.037470]
> > > ------------[ cut here ]------------ 34. [ 45.042127] WARNING: CPU:
> 0
> > > PID: 102 at lib/dma-debug.c:1080 check_unmap+0x784/0x9f4()
> > > 35. [ 45.050066] fec 2188000.ethernet: DMA-API: device driver tries
> to
> > > free DMA memory it has not a]
> > >
> > > (where the printk at line 32 is something that was added to debug
> this.)
> > >
> > > The sad thing is that the remainder of my FEC patches did go a long
> way
> > > to clean up these kinds of issues in the driver (and there's /many/
> of
> > > them), but unfortunately other conflicting changes got merged before
> I
> > > could finish rebasing them, I decided to move on to other things and
> > > discard the remainder of my patch set. Marek showed some interest in
> > > taking the patch set over, but I've not heard anything more - and I'm
> not
> > > about to resurect my efforts only to get into the same situation
> where
> > > I'm carrying 50 odd patches which I can't merge back into mainline
> > > without spending weeks endlessly rebasing them.
> > >
> > Russell, many thanks for your effort and thanks for your pointing out
> the bug.
> > I will think one method to fix it.
> >
> > And I have one question for highmem dma mapping issue as below:
> > fec_enet_txq_submit_frag_skb(struct fec_enet_priv_tx_q *txq, struct
> sk_buff *skb, struct net_device *ndev)
> > {
> > ...
> > bufaddr = page_address(this_frag->page.p) + this_frag-
> >page_offset;
> >
> > index = fec_enet_get_bd_index(txq->tx_bd_base, bdp,
> fep);
> > if (((unsigned long) bufaddr) & fep->tx_align ||
> > fep->quirks & FEC_QUIRK_SWAP_FRAME) {
> > memcpy(txq->tx_bounce[index], bufaddr,
> frag_len);
> > bufaddr = txq->tx_bounce[index];
> >
> > if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
> > swap_buffer(bufaddr, frag_len);
> > }
> >
> > addr = dma_map_single(&fep->pdev->dev, bufaddr,
> frag_len,
> > DMA_TO_DEVICE);
> > if (dma_mapping_error(&fep->pdev->dev, addr)) {
> > dev_kfree_skb_any(skb);
> > if (net_ratelimit())
> > netdev_err(ndev, "Tx DMA memory map
> failed\n");
> > goto dma_mapping_error;
> > }
> > ...
> > }
> >
> > If the frag page is located at high memory, use dma_map_single() is not
> > right, must use skb_frag_dma_map() or dma_map_page().
>
> Correct.
>
> > But before mapping, if tx has buffer alignment limitation (tx_align is
> > not zero), there need to do memcpy for buffer alignment.
>
> Right, and that can be detected by simply checking this_frag->page_offset
> as we know that a page address is always aligned to a page.
>
> > So, there we need to check whether the page is in highmem, if so, we
> > need to call kmap_atomic() or kmap_high_get() to get cpu address,
> > And then do memcpy or swap buffer operation.
>
> Yes - you'd need to do something like this:
>
> if (this_frag->page_offset & fep->tx_align ||
> fep->quirks & FEC_QUIRK_SWAP_FRAME) {
> bufaddr = kmap_atomic(this_frag->page.p) + this_frag-
> >page_offset;
> memcpy(txq->tx_bounce[index], bufaddr, frag_len);
> kunmap_atomic(bufaddr);
>
> bufaddr = txq->tx_bounce[index];
> if (fep->quirks & FEC_QUIRK_SWAP_FRAME)
> swap_buffer(bufaddr, frag_len);
> addr = dma_map_single(&fep->pdev->dev, bufaddr, frag_len,
> DMA_TO_DEVICE);
> } else {
> addr = skb_frag_dma_map(&fep->pdev->dev, this_frag, 0,
> frag_len, DMA_TO_DEVICE);
> }
>
> if (dma_mapping_error(&fep->pdev->dev, addr)) {
> dev_kfree_skb_any(skb);
> if (net_ratelimit())
> netdev_err(ndev, "Tx DMA memory map failed\n");
> goto dma_mapping_error;
> }
>
> You'll also need to record whether you should use dma_unmap_page() or
> dma_unmap_single().
>
Russell, thanks for your suggestion and comments. I will generate one patch to fix it like your suggestion.
(Sorry for late to reply your mail since I missed to read this mail)
Regards,
Andy
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists