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  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]
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