netdev - Re: [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk

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]

Message-ID: <7d6676f7-c695-a9a7-f64d-798755b2d519@infradead.org>
Date:   Sat, 25 Feb 2023 16:16:38 -0800
From:   Geoff Levand <geoff@...radead.org>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Cc:     Alexander Duyck <alexander.duyck@...il.com>
Subject: Re: [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length

Hi,

On 2/14/23 02:46, Paolo Abeni wrote:
> +Alex
> On Sun, 2023-02-12 at 18:00 +0000, Geoff Levand wrote:
>> The Gelic Ethernet device needs to have the RX sk_buffs aligned to
>> GELIC_NET_RXBUF_ALIGN and the length of the RX sk_buffs must be a
>> multiple of GELIC_NET_RXBUF_ALIGN.
>>
>> The current Gelic Ethernet driver was not allocating sk_buffs large
>> enough to allow for this alignment.
>>
>> Fixes various randomly occurring runtime network errors.
>>
>> Fixes: 02c1889166b4 (ps3: gigabit ethernet driver for PS3, take3)
> 
> Please use the correct format for the Fixes tag: <hash> ("<msg>"). Note
> the missing quotes.

I'll add those.
 
>> -	/* io-mmu-map the skb */
>> -	descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
>> -						     descr->skb->data,
>> -						     GELIC_NET_MAX_MTU,
>> -						     DMA_FROM_DEVICE));
>> +	skb_reserve(descr->skb, aligned_buf.offset);
>> +
>> +	descr->buf_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
>> +		DMA_FROM_DEVICE);
> 
> As already noted by Alex, you should preserve the cpu_to_be32(). If the
> running arch is be32, it has 0 performance and/or code size overhead,
> and it helps readability and maintainability.

I'll add those in for the next patch set.

> Please be sure to check the indentation of new code with checkpatch.

checkpatch reports a CHECK for my indentation. As is discussed in
the kernel's coding style guide, coding style is about
maintainability, and as the maintainer of this driver I think
the indentation I have used is more readable and hence easier to
maintain.

Thanks for the review.

-Geoff