netdev - Re: [OpenWrt-Devel] [RFC TRY#2][PATCH] bgmac: pass received packet to the netif instead of copying it

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] [day] [month] [year] [list]

Message-ID: <CACna6rzUN1fcpdWV=U9WX9_8DNO3g5EJOgkqxyFqcQk6Z5799Q@mail.gmail.com>
Date:	Tue, 29 Oct 2013 07:59:57 +0100
From:	Rafał Miłecki <zajec5@...il.com>
To:	Nathan Hintz <nlhintz@...mail.com>
Cc:	OpenWrt Development List <openwrt-devel@...ts.openwrt.org>,
	Network Development <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [OpenWrt-Devel] [RFC TRY#2][PATCH] bgmac: pass received packet to
 the netif instead of copying it

2013/10/29 Nathan Hintz <nlhintz@...mail.com>:
> On Mon, 28 Oct 2013 18:42:22 +0100
> Rafał Miłecki <zajec5@...il.com> wrote:
>
> Hi:
>
> A few questions/comments inline...
>
> Nathan
>
>> Copying whole packets with skb_copy_from_linear_data_offset is a pretty
>> bad idea. CPU was spending time in __copy_user_common and network
>> performance was lower. With the new solution iperf-measured speed
>> increased from 116Mb/s to 134Mb/s.
>>
>> Another way to improve performance could be switching to build_skb. It
>> is cache-specific, so will require testing of various devices.
>>
>> Signed-off-by: Rafał Miłecki <zajec5@...il.com>
>> ---
>>  drivers/net/ethernet/broadcom/bgmac.c |   71 ++++++++++++++++++++-------------
>>  1 file changed, 44 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>> index 6b7541f..fde9a11 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>> @@ -307,7 +307,6 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>>               struct device *dma_dev = bgmac->core->dma_dev;
>>               struct bgmac_slot_info *slot = &ring->slots[ring->start];
>>               struct sk_buff *skb = slot->skb;
>> -             struct sk_buff *new_skb;
>>               struct bgmac_rx_header *rx;
>>               u16 len, flags;
>>
>> @@ -320,38 +319,56 @@ static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,
>>               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);
>> -             } else {
>> +             do {
>
> "old_skb" duplicates "skb" stored above, can that one be used (renamed) instead of creating a new one here?

I've focused on clean code too much. That won't be needed anyway when
I rebase my patch on top of yours.


>> +                     struct sk_buff *old_skb = slot->skb;
>> +                     dma_addr_t old_dma_addr = slot->dma_addr;
>> +                     int err;
>> +
>> +                     /* 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);
>
> Nothing in the buffer has been changed by the cpu yet, so is this sync necessary?

I've moved your comment a line below, so it comments the code *above*.

I'm using LDD3 to understand DMA and it contains this explanation:

> void dma_sync_single_for_cpu(struct device *dev, dma_handle_t bus_addr,
> size_t size, enum dma_data_direction direction);
>
> This function should be called before the processor accesses a streaming DMA buffer. Once the call has been made, the CPU “owns” the DMA buffer and can work with it as needed. Before the device accesses the buffer, however, ownership should be transferred back to it with:
>
> void dma_sync_single_for_device(struct device *dev, dma_handle_t bus_addr,
> size_t size, enum dma_data_direction direction);

So even if I didn't change anything in the buffer, I believe we still
need to "sync" it back to make it accessible to the hardware.


> I've sent a separate patch against "bgmac_dma_rx_skb_for_slot" to not corrupt the slot at all if an
> error occurs (skb alloc or dma mapping), and free the skb that was allocated if a dma mapping error
> occurs.  Assuming that patch is accepted, then the following two lines would not be needed.
> With "bgmac_dma_rx_skb_for_slot" as it currently exists, this would leak an skb for a dma mapping
> error (this was pre-existing to the changes in this patch).

Thanks, I'll rebase my patch.
--
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