| Message ID | 1428968537-6181-4-git-send-email-nbd@openwrt.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote: > @@ -382,6 +382,19 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac, > dma_desc->ctl1 = cpu_to_le32(ctl1); > } > > +static void bgmac_dma_rx_poison_buf(struct device *dma_dev, > + struct bgmac_slot_info *slot) > +{ > + struct bgmac_rx_header *rx = slot->buf + BGMAC_RX_BUF_OFFSET; > + > + dma_sync_single_for_cpu(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE, > + DMA_FROM_DEVICE); > + rx->len = cpu_to_le16(0xdead); > + rx->flags = cpu_to_le16(0xdead); > + dma_sync_single_for_device(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE, > + DMA_FROM_DEVICE); > +} flags should be 0xbeef > @@ -404,51 +417,32 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, > void *buf = slot->buf; > u16 len, flags; > > - /* Unmap buffer to make it accessible to the CPU */ > - dma_sync_single_for_cpu(dma_dev, slot->dma_addr, > - BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE); > + do { > + /* Prepare new skb as replacement */ > + if (bgmac_dma_rx_skb_for_slot(bgmac, slot)) { > + bgmac_dma_rx_poison_buf(dma_dev, slot); > + break; > + } So you just replaced slot->dma_addr with a mapping of new skb data. > > - /* Get info from the header */ > - len = le16_to_cpu(rx->len); > - flags = le16_to_cpu(rx->flags); > + /* Unmap buffer to make it accessible to the CPU */ > + dma_unmap_single(dma_dev, slot->dma_addr, > + BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE); Now you unmap the new sbk data instead of the old one you want to access. That's why the old code included old_dma_addr. > @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac, > > for (i = 0; i < ring->num_slots; i++) { > slot = &ring->slots[i]; > - if (!slot->buf) > + if (!slot->dma_addr) > continue; I think it breaks error path of bgmac_dma_alloc. It may happen that during DMA alloc we alloc skb and then we fail to map it. In such case buf should be freed. Please trace how bgmac_dma_free is used in bgmac_dma_alloc. > > - if (slot->dma_addr) > - dma_unmap_single(dma_dev, slot->dma_addr, > - BGMAC_RX_BUF_SIZE, > - DMA_FROM_DEVICE); > + dma_unmap_single(dma_dev, slot->dma_addr, > + BGMAC_RX_BUF_SIZE, > + DMA_FROM_DEVICE); > put_page(virt_to_head_page(slot->buf)); > + slot->dma_addr = 0; > } > }
On 2015-04-14 07:55, Rafał Miłecki wrote: > On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote: >> @@ -382,6 +382,19 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac, >> dma_desc->ctl1 = cpu_to_le32(ctl1); >> } >> >> +static void bgmac_dma_rx_poison_buf(struct device *dma_dev, >> + struct bgmac_slot_info *slot) >> +{ >> + struct bgmac_rx_header *rx = slot->buf + BGMAC_RX_BUF_OFFSET; >> + >> + dma_sync_single_for_cpu(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE, >> + DMA_FROM_DEVICE); >> + rx->len = cpu_to_le16(0xdead); >> + rx->flags = cpu_to_le16(0xdead); >> + dma_sync_single_for_device(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE, >> + DMA_FROM_DEVICE); >> +} > > flags should be 0xbeef Right. >> @@ -404,51 +417,32 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, >> void *buf = slot->buf; >> u16 len, flags; >> >> - /* Unmap buffer to make it accessible to the CPU */ >> - dma_sync_single_for_cpu(dma_dev, slot->dma_addr, >> - BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE); >> + do { >> + /* Prepare new skb as replacement */ >> + if (bgmac_dma_rx_skb_for_slot(bgmac, slot)) { >> + bgmac_dma_rx_poison_buf(dma_dev, slot); >> + break; >> + } > > So you just replaced slot->dma_addr with a mapping of new skb data. > > >> >> - /* Get info from the header */ >> - len = le16_to_cpu(rx->len); >> - flags = le16_to_cpu(rx->flags); >> + /* Unmap buffer to make it accessible to the CPU */ >> + dma_unmap_single(dma_dev, slot->dma_addr, >> + BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE); > > Now you unmap the new sbk data instead of the old one you want to access. > That's why the old code included old_dma_addr. Stupid mistake, I should be more careful with late night coding ;) >> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac, >> >> for (i = 0; i < ring->num_slots; i++) { >> slot = &ring->slots[i]; >> - if (!slot->buf) >> + if (!slot->dma_addr) >> continue; > > I think it breaks error path of bgmac_dma_alloc. It may happen that > during DMA alloc we alloc skb and then we fail to map it. In such case > buf should be freed. Please trace how bgmac_dma_free is used in > bgmac_dma_alloc. I don't think so: bgmac_dma_rx_skb_for_slot handles both buffer allocation and dma mapping. If dma mapping fails, the buffer is freed before any part of the slot is overwritten. >> - if (slot->dma_addr) >> - dma_unmap_single(dma_dev, slot->dma_addr, >> - BGMAC_RX_BUF_SIZE, >> - DMA_FROM_DEVICE); >> + dma_unmap_single(dma_dev, slot->dma_addr, >> + BGMAC_RX_BUF_SIZE, >> + DMA_FROM_DEVICE); >> put_page(virt_to_head_page(slot->buf)); >> + slot->dma_addr = 0; >> } >> } - Felix -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14 April 2015 at 11:13, Felix Fietkau <nbd@openwrt.org> wrote: > On 2015-04-14 07:55, Rafał Miłecki wrote: >> On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote: >>> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac, >>> >>> for (i = 0; i < ring->num_slots; i++) { >>> slot = &ring->slots[i]; >>> - if (!slot->buf) >>> + if (!slot->dma_addr) >>> continue; >> >> I think it breaks error path of bgmac_dma_alloc. It may happen that >> during DMA alloc we alloc skb and then we fail to map it. In such case >> buf should be freed. Please trace how bgmac_dma_free is used in >> bgmac_dma_alloc. > I don't think so: bgmac_dma_rx_skb_for_slot handles both buffer > allocation and dma mapping. If dma mapping fails, the buffer is freed > before any part of the slot is overwritten. Oops, I think I just spotted a memory leak then. If bgmac_dma_rx_skb_for_slot fails to do DMA mapping it returns an error without freeing a skb.
On 2015-04-14 11:19, Rafał Miłecki wrote: > On 14 April 2015 at 11:13, Felix Fietkau <nbd@openwrt.org> wrote: >> On 2015-04-14 07:55, Rafał Miłecki wrote: >>> On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote: >>>> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac, >>>> >>>> for (i = 0; i < ring->num_slots; i++) { >>>> slot = &ring->slots[i]; >>>> - if (!slot->buf) >>>> + if (!slot->dma_addr) >>>> continue; >>> >>> I think it breaks error path of bgmac_dma_alloc. It may happen that >>> during DMA alloc we alloc skb and then we fail to map it. In such case >>> buf should be freed. Please trace how bgmac_dma_free is used in >>> bgmac_dma_alloc. >> I don't think so: bgmac_dma_rx_skb_for_slot handles both buffer >> allocation and dma mapping. If dma mapping fails, the buffer is freed >> before any part of the slot is overwritten. > > Oops, I think I just spotted a memory leak then. > > If bgmac_dma_rx_skb_for_slot fails to do DMA mapping it returns an > error without freeing a skb. It neither allocates, nor frees an skb - I should probably rename that function in a later patch round. Allocation does: buf = netdev_alloc_frag(BGMAC_RX_ALLOC_SIZE); When handling a DMA mapping error, it does this: put_page(virt_to_head_page(buf)); Where's the memory leak? - Felix -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14 April 2015 at 11:26, Felix Fietkau <nbd@openwrt.org> wrote: > On 2015-04-14 11:19, Rafał Miłecki wrote: >> On 14 April 2015 at 11:13, Felix Fietkau <nbd@openwrt.org> wrote: >>> On 2015-04-14 07:55, Rafał Miłecki wrote: >>>> On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote: >>>>> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac, >>>>> >>>>> for (i = 0; i < ring->num_slots; i++) { >>>>> slot = &ring->slots[i]; >>>>> - if (!slot->buf) >>>>> + if (!slot->dma_addr) >>>>> continue; >>>> >>>> I think it breaks error path of bgmac_dma_alloc. It may happen that >>>> during DMA alloc we alloc skb and then we fail to map it. In such case >>>> buf should be freed. Please trace how bgmac_dma_free is used in >>>> bgmac_dma_alloc. >>> I don't think so: bgmac_dma_rx_skb_for_slot handles both buffer >>> allocation and dma mapping. If dma mapping fails, the buffer is freed >>> before any part of the slot is overwritten. >> >> Oops, I think I just spotted a memory leak then. >> >> If bgmac_dma_rx_skb_for_slot fails to do DMA mapping it returns an >> error without freeing a skb. > It neither allocates, nor frees an skb - I should probably rename that > function in a later patch round. > > Allocation does: > buf = netdev_alloc_frag(BGMAC_RX_ALLOC_SIZE); > > When handling a DMA mapping error, it does this: > put_page(virt_to_head_page(buf)); > > Where's the memory leak? I mean the buf, not skb, sorry: buf = netdev_alloc_frag(BGMAC_RX_ALLOC_SIZE); if (dma_mapping_error(dma_dev, dma_addr)) { LACKING free(buf) HERE return -ENOMEM; }
On 14 April 2015 at 11:26, Felix Fietkau <nbd@openwrt.org> wrote: > On 2015-04-14 11:19, Rafał Miłecki wrote: >> On 14 April 2015 at 11:13, Felix Fietkau <nbd@openwrt.org> wrote: >>> On 2015-04-14 07:55, Rafał Miłecki wrote: >>>> On 14 April 2015 at 01:42, Felix Fietkau <nbd@openwrt.org> wrote: >>>>> @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac, >>>>> >>>>> for (i = 0; i < ring->num_slots; i++) { >>>>> slot = &ring->slots[i]; >>>>> - if (!slot->buf) >>>>> + if (!slot->dma_addr) >>>>> continue; >>>> >>>> I think it breaks error path of bgmac_dma_alloc. It may happen that >>>> during DMA alloc we alloc skb and then we fail to map it. In such case >>>> buf should be freed. Please trace how bgmac_dma_free is used in >>>> bgmac_dma_alloc. >>> I don't think so: bgmac_dma_rx_skb_for_slot handles both buffer >>> allocation and dma mapping. If dma mapping fails, the buffer is freed >>> before any part of the slot is overwritten. >> >> Oops, I think I just spotted a memory leak then. >> >> If bgmac_dma_rx_skb_for_slot fails to do DMA mapping it returns an >> error without freeing a skb. > It neither allocates, nor frees an skb - I should probably rename that > function in a later patch round. > > Allocation does: > buf = netdev_alloc_frag(BGMAC_RX_ALLOC_SIZE); > > When handling a DMA mapping error, it does this: > put_page(virt_to_head_page(buf)); > > Where's the memory leak? Oooh, sorry. I didn't understand put_page. I just expected direct kfree here. It's alright then, sorry.
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c index 9a6742e..f897e62 100644 --- a/drivers/net/ethernet/broadcom/bgmac.c +++ b/drivers/net/ethernet/broadcom/bgmac.c @@ -382,6 +382,19 @@ static void bgmac_dma_rx_setup_desc(struct bgmac *bgmac, dma_desc->ctl1 = cpu_to_le32(ctl1); } +static void bgmac_dma_rx_poison_buf(struct device *dma_dev, + struct bgmac_slot_info *slot) +{ + struct bgmac_rx_header *rx = slot->buf + BGMAC_RX_BUF_OFFSET; + + dma_sync_single_for_cpu(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE, + DMA_FROM_DEVICE); + rx->len = cpu_to_le16(0xdead); + rx->flags = cpu_to_le16(0xdead); + dma_sync_single_for_device(dma_dev, slot->dma_addr, BGMAC_RX_BUF_SIZE, + DMA_FROM_DEVICE); +} + static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, int weight) { @@ -404,51 +417,32 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, void *buf = slot->buf; u16 len, flags; - /* Unmap buffer to make it accessible to the CPU */ - dma_sync_single_for_cpu(dma_dev, slot->dma_addr, - BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE); + do { + /* Prepare new skb as replacement */ + if (bgmac_dma_rx_skb_for_slot(bgmac, slot)) { + bgmac_dma_rx_poison_buf(dma_dev, slot); + break; + } - /* Get info from the header */ - len = le16_to_cpu(rx->len); - flags = le16_to_cpu(rx->flags); + /* Unmap buffer to make it accessible to the CPU */ + dma_unmap_single(dma_dev, slot->dma_addr, + BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE); - do { - dma_addr_t old_dma_addr = slot->dma_addr; - int err; + /* Get info from the header */ + len = le16_to_cpu(rx->len); + flags = le16_to_cpu(rx->flags); /* Check for poison and drop or pass the packet */ if (len == 0xdead && flags == 0xbeef) { bgmac_err(bgmac, "Found poisoned packet at slot %d, DMA issue!\n", ring->start); - dma_sync_single_for_device(dma_dev, - slot->dma_addr, - BGMAC_RX_BUF_SIZE, - DMA_FROM_DEVICE); + put_page(virt_to_head_page(buf)); break; } /* Omit CRC. */ len -= ETH_FCS_LEN; - /* Prepare new skb as replacement */ - err = bgmac_dma_rx_skb_for_slot(bgmac, slot); - if (err) { - /* Poison the old skb */ - rx->len = cpu_to_le16(0xdead); - rx->flags = cpu_to_le16(0xbeef); - - dma_sync_single_for_device(dma_dev, - slot->dma_addr, - BGMAC_RX_BUF_SIZE, - DMA_FROM_DEVICE); - break; - } - bgmac_dma_rx_setup_desc(bgmac, ring, ring->start); - - /* Unmap old skb, we'll pass it to the netfif */ - dma_unmap_single(dma_dev, old_dma_addr, - BGMAC_RX_BUF_SIZE, DMA_FROM_DEVICE); - skb = build_skb(buf, BGMAC_RX_ALLOC_SIZE); skb_put(skb, BGMAC_RX_FRAME_OFFSET + BGMAC_RX_BUF_OFFSET + len); @@ -461,6 +455,8 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring, handled++; } while (0); + bgmac_dma_rx_setup_desc(bgmac, ring, ring->start); + if (++ring->start >= BGMAC_RX_RING_SLOTS) ring->start = 0; @@ -528,14 +524,14 @@ static void bgmac_dma_rx_ring_free(struct bgmac *bgmac, for (i = 0; i < ring->num_slots; i++) { slot = &ring->slots[i]; - if (!slot->buf) + if (!slot->dma_addr) continue; - if (slot->dma_addr) - dma_unmap_single(dma_dev, slot->dma_addr, - BGMAC_RX_BUF_SIZE, - DMA_FROM_DEVICE); + dma_unmap_single(dma_dev, slot->dma_addr, + BGMAC_RX_BUF_SIZE, + DMA_FROM_DEVICE); put_page(virt_to_head_page(slot->buf)); + slot->dma_addr = 0; } }
Allocate a new buffer before processing the completed one. If allocation fails, reuse the old buffer. Signed-off-by: Felix Fietkau <nbd@openwrt.org> --- drivers/net/ethernet/broadcom/bgmac.c | 70 +++++++++++++++++------------------ 1 file changed, 33 insertions(+), 37 deletions(-)