netdev - Re: [PATCH net v4 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: <29b83fc2-af28-e19d-b837-80778e429417@infradead.org>
Date:   Sun, 12 Feb 2023 09:06:22 -0800
From:   Geoff Levand <geoff@...radead.org>
To:     Alexander H Duyck <alexander.duyck@...il.com>,
        netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net v4 1/2] net/ps3_gelic_net: Fix RX sk_buff length

Hi Alexander,

Thanks for the review.

On 2/6/23 08:25, Alexander H Duyck wrote:
> On Sun, 2023-02-05 at 22:10 +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.
>>

>>  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 {
>> +		unsigned int total_bytes;
>> +		unsigned int offset;
>> +	} aligned_buf;
>> +	dma_addr_t cpu_addr;
>>  
>>  	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);
>> +	aligned_buf.total_bytes = (GELIC_NET_RXBUF_ALIGN - 1) +
>> +		GELIC_NET_MAX_MTU + (GELIC_NET_RXBUF_ALIGN - 1);
>> +
> 
> This value isn't aligned to anything as there have been no steps taken
> to align it. In fact it is guaranteed to be off by 2. Did you maybe
> mean to use an "&" somewhere?

total_bytes here means the total number of bytes to allocate that will
allow for the desired alignment.  This value a bit too much though since
we really just need it to end on a GELIC_NET_RXBUF_ALIGN boundary, so 
adding ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN) should be enough.
I'll fix that in the next patch version.

>> +	descr->skb = dev_alloc_skb(aligned_buf.total_bytes);
>> +
>>  	if (!descr->skb) {
>> -		descr->buf_addr = 0; /* tell DMAC don't touch memory */
>> +		descr->buf_addr = 0;
>>  		return -ENOMEM;
> 
> Why remove this comment?

If we return -ENOMEM this descriptor shouldn't be used.

>>  	}
>> -	descr->buf_size = cpu_to_be32(bufsize);
>> +
>> +	aligned_buf.offset =
>> +		PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN) -
>> +			descr->skb->data;
>> +
>> +	descr->buf_size = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
> 
> Originally this was being written using cpu_to_be32. WIth this you are
> writing it raw w/ the cpu endianness. Is there a byte ordering issue
> here?

No. The PS3 has a big endian CPU, so we really don't need any
of the endian conversions.

> 
>>  	descr->dmac_cmd_status = 0;
>>  	descr->result_size = 0;
>>  	descr->valid_size = 0;
>>  	descr->data_error = 0;
>>  
>> -	offset = ((unsigned long)descr->skb->data) &
>> -		(GELIC_NET_RXBUF_ALIGN - 1);
>> -	if (offset)
>> -		skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
> 
> Rather than messing with all this it might be easier to just drop
> offset in favor of NET_SKB_PAD since that should be offset in all cases
> where dev_alloc_skb is being used. With that the reserve could just be
> a constant.

GELIC_NET_RXBUF_ALIGN is a property of the gelic hardware device.  I
would think if NET_SKB_PAD would work it would just be by coincidence.

>> -	/* 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);
>> +
>> +	cpu_addr = dma_map_single(dev, descr->skb->data, descr->buf_size,
>> +		DMA_FROM_DEVICE);
>> +
>> +	descr->buf_addr = cpu_to_be32(cpu_addr);
>> +
>>  	if (!descr->buf_addr) {
> 
> This check should be for dma_mapping_error based on "cpu_addr". There
> are some configs that don't return NULL to indicate a mapping error.

As was requested, I have put those corrections into the second patch
of this series.

-Geoff