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]

Date:   Tue, 14 Feb 2023 11:46:26 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Geoff Levand <geoff@...radead.org>, 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

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

> 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);
> +
>  	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);

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.

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

When reposting, please be sure to CC people that gave feedback on
previous iterations.

Cheers,

Paolo