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: <2bdbcb46-fbaa-a2e6-81dd-9e36aad7ab9a@infradead.org>
Date:   Sat, 25 Feb 2023 12:46:34 -0800
From:   Geoff Levand <geoff@...radead.org>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net v5 1/2] net/ps3_gelic_net: Fix RX sk_buff length

Hi,

On 2/14/23 09:34, Alexander Lobakin 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)
>> Signed-off-by: Geoff Levand <geoff@...radead.org>
>> ---
>>  drivers/net/ethernet/toshiba/ps3_gelic_net.c | 55 ++++++++++++--------
>>  1 file changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> index cf8de8a7a8a1..2bb68e60d0d5 100644
>> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
>> @@ -365,51 +365,62 @@ static int gelic_card_init_chain(struct gelic_card *card,
>>   *
>>   * allocates a new rx skb, iommu-maps it and attaches it to the descriptor.
>>   * Activate the descriptor state-wise
>> + *
>> + * Gelic RX sk_buffs must be aligned to GELIC_NET_RXBUF_ALIGN and the length
>> + * must be a multiple of GELIC_NET_RXBUF_ALIGN.
>>   */
>>  static int gelic_descr_prepare_rx(struct gelic_card *card,
>>  				  struct gelic_descr *descr)
>>  {
>> -	int offset;
>> -	unsigned int bufsize;
>> +	struct device *dev = ctodev(card);
>> +	struct {
>> +		const unsigned int buffer_bytes;
>> +		const unsigned int total_bytes;
>> +		unsigned int offset;
>> +	} aligned_buf = {
>> +		.buffer_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN),
>> +		.total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) +
>> +			ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN),
>> +	};
>>  
>>  	if (gelic_descr_get_status(descr) !=  GELIC_DESCR_DMA_NOT_IN_USE)
>>  		dev_info(ctodev(card), "%s: ERROR status\n", __func__);
>> -	/* we need to round up the buffer size to a multiple of 128 */
>> -	bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
>>  
>> -	/* and we need to have it 128 byte aligned, therefore we allocate a
>> -	 * bit more */
>> -	descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
>> +	descr->skb = dev_alloc_skb(aligned_buf.total_bytes);
> 
> I highly recommend using {napi,netdev}_alloc_frag_align() +
> {napi_,}build_skb() to not waste memory. It will align everything for
> ye, so you won't need all this.

I converted this over to use napi_alloc_frag_align and napi_build_skb, and
then cleanup with skb_free_frag.  I couldn't find any good documentation for
those napi routines though.

For napi_alloc_frag_align, it seems the align parameter should be the
alignment required by the gelic hardware device, so GELIC_NET_RXBUF_ALIGN.
But for the fragsz parameter I couldn't quite figure out from the existing
users of it what exactly it should be.  

I assumed it needed to be the maximum number of bytes the hardware device can
fill, so I set it to be ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN), but
with that I got skb_over_panic errors (skb->tail > skb->end).  I added some
debugging code and found that when it hit the panic skb->end was always 384
bytes short.  I changed GELIC_NET_MAX_MTU to be 1920 which is
ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN) + 384, and it seems to be
working OK.  Does this seem OK?  Maybe the current GELIC_NET_MAX_MTU value is
incorrect.  I did not write that driver, and I don't have any good
documentation for the gelic device.

Thanks for any help you can give.

-Geoff