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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6d2a84e9-be27-6707-3d65-b00c8e206d2a@intel.com>
Date:   Tue, 14 Feb 2023 18:34:27 +0100
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Geoff Levand <geoff@...radead.org>
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

From: Geoff Levand <geoff@...radead.org>
Date: Sun, 12 Feb 2023 18:00:58 +0000

> 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.

Also, dunno why create an onstack struct for 3 integers :D

> +
>  	if (!descr->skb) {
> -		descr->buf_addr = 0; /* tell DMAC don't touch memory */
> +		descr->buf_addr = 0;
>  		return -ENOMEM;
>  	}
> -	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 = aligned_buf.buffer_bytes;
>  	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);
> -	/* 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);
> +
>  	if (!descr->buf_addr) {
>  		dev_kfree_skb_any(descr->skb);
> +		descr->buf_addr = 0;
> +		descr->buf_size = 0;
>  		descr->skb = NULL;
> -		dev_info(ctodev(card),
> +		dev_info(dev,
>  			 "%s:Could not iommu-map rx buffer\n", __func__);
>  		gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
>  		return -ENOMEM;
> -	} else {
> -		gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> -		return 0;
>  	}
> +
> +	gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> +	return 0;
>  }
>  
>  /**

Thanks,
Olek