* Re: ixgbe: Set IXGBE_RSC_CB(skb)->DMA field to zero after unmapping the address [not found] <201003221759.o2MHxEaj015546@hera.kernel.org> @ 2010-03-23 3:57 ` Benjamin Herrenschmidt 2010-03-23 17:40 ` Malli 0 siblings, 1 reply; 4+ messages in thread From: Benjamin Herrenschmidt @ 2010-03-23 3:57 UTC (permalink / raw) To: Mallikarjuna R Chilakala; +Cc: Linux Kernel Mailing List, David Miller, netdev On Mon, 2010-03-22 at 17:59 +0000, Linux Kernel Mailing List wrote: > Gitweb: http://git.kernel.org/linus/fd3686a842717b890fbe3024b83a616c54d5dba0 > Commit: fd3686a842717b890fbe3024b83a616c54d5dba0 > Parent: 936332b8e00103fc20eb7e915c9a3bcb2835a11a > Author: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com> > AuthorDate: Fri Mar 19 04:41:33 2010 +0000 > Committer: David S. Miller <davem@davemloft.net> > CommitDate: Fri Mar 19 21:00:44 2010 -0700 > > ixgbe: Set IXGBE_RSC_CB(skb)->DMA field to zero after unmapping the address > > As per Simon Horman's feedback set IXGBE_RSC_CB(skb)->dma to zero > after unmapping HWRSC DMA address to avoid double freeing. > Note that this whole code is bogus :-) You cannot just assume that 0 is a invalid DMA address. It is not. In fact, while you can check if a dma_addr_t is invalid using dma_mapping_error(), the generic APIs don't provide you with a magic "bad" value you can use for what you are trying to do. Granted, I think we should make our iommu code reserve the first page for the sake of everybody's sanity and to avoid such pitfalls, but this code is wrong with today iommu implementations. Cheers, Ben. > Signed-off-by: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com> > Acked-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > --- > drivers/net/ixgbe/ixgbe_main.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ixgbe/ixgbe_main.c b/drivers/net/ixgbe/ixgbe_main.c > index 18b5b21..d75c46f 100644 > --- a/drivers/net/ixgbe/ixgbe_main.c > +++ b/drivers/net/ixgbe/ixgbe_main.c > @@ -935,10 +935,12 @@ static bool ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, > if (skb->prev) > skb = ixgbe_transform_rsc_queue(skb, &(rx_ring->rsc_count)); > if (adapter->flags2 & IXGBE_FLAG2_RSC_ENABLED) { > - if (IXGBE_RSC_CB(skb)->dma) > + if (IXGBE_RSC_CB(skb)->dma) { > pci_unmap_single(pdev, IXGBE_RSC_CB(skb)->dma, > rx_ring->rx_buf_len, > PCI_DMA_FROMDEVICE); > + IXGBE_RSC_CB(skb)->dma = 0; > + } > if (rx_ring->flags & IXGBE_RING_RX_PS_ENABLED) > rx_ring->rsc_count += skb_shinfo(skb)->nr_frags; > else > @@ -3126,10 +3128,12 @@ static void ixgbe_clean_rx_ring(struct ixgbe_adapter *adapter, > rx_buffer_info->skb = NULL; > do { > struct sk_buff *this = skb; > - if (IXGBE_RSC_CB(this)->dma) > + if (IXGBE_RSC_CB(this)->dma) { > pci_unmap_single(pdev, IXGBE_RSC_CB(this)->dma, > rx_ring->rx_buf_len, > PCI_DMA_FROMDEVICE); > + IXGBE_RSC_CB(this)->dma = 0; > + } > skb = skb->prev; > dev_kfree_skb(this); > } while (skb); > -- > To unsubscribe from this list: send the line "unsubscribe git-commits-head" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ixgbe: Set IXGBE_RSC_CB(skb)->DMA field to zero after unmapping the address 2010-03-23 3:57 ` ixgbe: Set IXGBE_RSC_CB(skb)->DMA field to zero after unmapping the address Benjamin Herrenschmidt @ 2010-03-23 17:40 ` Malli 0 siblings, 0 replies; 4+ messages in thread From: Malli @ 2010-03-23 17:40 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Mallikarjuna R Chilakala, Linux Kernel Mailing List, David Miller, netdev On Mon, Mar 22, 2010 at 8:57 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Mon, 2010-03-22 at 17:59 +0000, Linux Kernel Mailing List wrote: >> Gitweb: http://git.kernel.org/linus/fd3686a842717b890fbe3024b83a616c54d5dba0 >> Commit: fd3686a842717b890fbe3024b83a616c54d5dba0 >> Parent: 936332b8e00103fc20eb7e915c9a3bcb2835a11a >> Author: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com> >> AuthorDate: Fri Mar 19 04:41:33 2010 +0000 >> Committer: David S. Miller <davem@davemloft.net> >> CommitDate: Fri Mar 19 21:00:44 2010 -0700 >> >> ixgbe: Set IXGBE_RSC_CB(skb)->DMA field to zero after unmapping the address >> >> As per Simon Horman's feedback set IXGBE_RSC_CB(skb)->dma to zero >> after unmapping HWRSC DMA address to avoid double freeing. >> > > Note that this whole code is bogus :-) You cannot just assume that 0 is > a invalid DMA address. It is not. In fact, while you can check if a > dma_addr_t is invalid using dma_mapping_error(), the generic APIs > don't provide you with a magic "bad" value you can use for what you are > trying to do. > > Granted, I think we should make our iommu code reserve the first page > for the sake of everybody's sanity and to avoid such pitfalls, but > this code is wrong with today iommu implementations. > > Cheers, > Ben. Yes. I just realized that i can't assign a zero magic "bad" value. It is only valid in x86/arm/m68k/alpha architecures and not in spark & PowerPC arch, (it should be ~0). In some other architecutres it throws a BUG() on with dma_mapping_error() checks. So the patch is not a total bogus in some architectures :(. May be it is best to create an internal FLAG in IXGBE_RSC_CB(skb) which can be used to avoid this double freeing. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ixgbe: Set IXGBE_RSC_CB(skb)->DMA field to zero after unmapping the address @ 2010-03-23 17:40 ` Malli 0 siblings, 0 replies; 4+ messages in thread From: Malli @ 2010-03-23 17:40 UTC (permalink / raw) To: Benjamin Herrenschmidt Cc: Mallikarjuna R Chilakala, Linux Kernel Mailing List, David Miller, netdev On Mon, Mar 22, 2010 at 8:57 PM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Mon, 2010-03-22 at 17:59 +0000, Linux Kernel Mailing List wrote: >> Gitweb: http://git.kernel.org/linus/fd3686a842717b890fbe3024b83a616c54d5dba0 >> Commit: fd3686a842717b890fbe3024b83a616c54d5dba0 >> Parent: 936332b8e00103fc20eb7e915c9a3bcb2835a11a >> Author: Mallikarjuna R Chilakala <mallikarjuna.chilakala@intel.com> >> AuthorDate: Fri Mar 19 04:41:33 2010 +0000 >> Committer: David S. Miller <davem@davemloft.net> >> CommitDate: Fri Mar 19 21:00:44 2010 -0700 >> >> ixgbe: Set IXGBE_RSC_CB(skb)->DMA field to zero after unmapping the address >> >> As per Simon Horman's feedback set IXGBE_RSC_CB(skb)->dma to zero >> after unmapping HWRSC DMA address to avoid double freeing. >> > > Note that this whole code is bogus :-) You cannot just assume that 0 is > a invalid DMA address. It is not. In fact, while you can check if a > dma_addr_t is invalid using dma_mapping_error(), the generic APIs > don't provide you with a magic "bad" value you can use for what you are > trying to do. > > Granted, I think we should make our iommu code reserve the first page > for the sake of everybody's sanity and to avoid such pitfalls, but > this code is wrong with today iommu implementations. > > Cheers, > Ben. Yes. I just realized that i can't assign a zero magic "bad" value. It is only valid in x86/arm/m68k/alpha architecures and not in spark & PowerPC arch, (it should be ~0). In some other architecutres it throws a BUG() on with dma_mapping_error() checks. So the patch is not a total bogus in some architectures :(. May be it is best to create an internal FLAG in IXGBE_RSC_CB(skb) which can be used to avoid this double freeing. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ixgbe: Set IXGBE_RSC_CB(skb)->DMA field to zero after unmapping the address 2010-03-23 17:40 ` Malli (?) @ 2010-03-23 23:40 ` Benjamin Herrenschmidt -1 siblings, 0 replies; 4+ messages in thread From: Benjamin Herrenschmidt @ 2010-03-23 23:40 UTC (permalink / raw) To: Malli Cc: Mallikarjuna R Chilakala, Linux Kernel Mailing List, David Miller, netdev On Tue, 2010-03-23 at 10:40 -0700, Malli wrote: > > Yes. I just realized that i can't assign a zero magic "bad" value. It > is only > valid in x86/arm/m68k/alpha architecures and not in spark & PowerPC > arch, > (it should be ~0). In some other architecutres it throws a BUG() on > with > dma_mapping_error() checks. So the patch is not a total bogus in some > architectures :(. > > May be it is best to create an internal FLAG in IXGBE_RSC_CB(skb) > which can be used to avoid this double freeing. I suppose if Linus reads that he'll just come back and say "you idiots, just make 0 invalid everywhere" like he did for interrupts tho :-) It wouldn't be too hard for us to make the IOMMU allocator always skip "0". But for now, I'd suggest you add a flag. Cheers, Ben. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-03-23 23:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <201003221759.o2MHxEaj015546@hera.kernel.org>
2010-03-23 3:57 ` ixgbe: Set IXGBE_RSC_CB(skb)->DMA field to zero after unmapping the address Benjamin Herrenschmidt
2010-03-23 17:40 ` Malli
2010-03-23 17:40 ` Malli
2010-03-23 23:40 ` Benjamin Herrenschmidt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.