netdev - Re: [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling

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]

Date:	Tue, 14 Apr 2015 07:55:18 +0200
From:	Rafał Miłecki <zajec5@...il.com>
To:	Felix Fietkau <nbd@...nwrt.org>
Cc:	Network Development <netdev@...r.kernel.org>,
	Hauke Mehrtens <hauke@...ke-m.de>
Subject: Re: [PATCH v6 4/9] bgmac: simplify/optimize rx DMA error handling

On 14 April 2015 at 01:42, Felix Fietkau <nbd@...nwrt.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;
>         }
>  }

-- 
Rafał
--
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