netdev - Re: [PATCH net v8 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: <64932ccc97c3bd2ab7fac6216e465550107b7fa4.camel@gmail.com>
Date:   Sat, 11 Mar 2023 14:37:00 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Geoff Levand <geoff@...radead.org>, netdev@...r.kernel.org,
        Jakub Kicinski <kuba@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net v8 1/2] net/ps3_gelic_net: Fix RX sk_buff length

On Sat, 2023-03-11 at 21:46 +0000, Geoff Levand wrote:
> The Gelic Ethernet device needs to have the RX sk_buffs aligned to
> GELIC_NET_RXBUF_ALIGN, and also 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.
> 
> Also, correct the maximum and minimum MTU sizes, and add a new
> preprocessor macro for the maximum frame size, GELIC_NET_MAX_FRAME.
> 
> 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 | 21 +++++++++++---------
>  drivers/net/ethernet/toshiba/ps3_gelic_net.h |  5 +++--
>  2 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index cf8de8a7a8a1..56557fc8d18a 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -44,6 +44,14 @@ MODULE_AUTHOR("SCE Inc.");
>  MODULE_DESCRIPTION("Gelic Network driver");
>  MODULE_LICENSE("GPL");
>  
> +/**
> + * 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 const unsigned int gelic_rx_skb_size =
> +	ALIGN(GELIC_NET_MAX_FRAME, GELIC_NET_RXBUF_ALIGN) +
> +	GELIC_NET_RXBUF_ALIGN - 1;
> 

After a bit more digging I now understand the need for the
"GELIC_NET_RXBUF_ALIGN - 1". It shouldn't be added here. The device
will not be able to DMA into it. It is being used to align the buffer
itself to 128B. I am assuming it must be 128B aligned in BOTH size and
offset.

>  /* set irq_mask */
>  int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
> @@ -370,21 +378,16 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>  				  struct gelic_descr *descr)
>  {
>  	int offset;
> -	unsigned int bufsize;
>  
>  	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 = netdev_alloc_skb(*card->netdev, gelic_rx_skb_size);

I would leave the "+  GELIC_NET_RXBUF_ALIGN - 1" here. so it should be
	descr->skb = netdev_alloc_skb(*card->netdev, 		
				      gelic_rx_skb_size +
				      GELIC_NET_RXBUF_ALIGN - 1);

Also I would leav the comment as it makes it a bit clearer what is
going on here.

>  	if (!descr->skb) {
>  		descr->buf_addr = 0; /* tell DMAC don't touch memory */
>  		return -ENOMEM;
>  	}
> -	descr->buf_size = cpu_to_be32(bufsize);
> +	descr->buf_size = cpu_to_be32(gelic_rx_skb_size);
>  	descr->dmac_cmd_status = 0;
>  	descr->result_size = 0;
>  	descr->valid_size = 0;

The part I missed was the lines of code that didn't make it into the
patch that like between the two code blocks here above and below. They
are doing an AND with your align mask and then adding the difference to
the skb reserve to pad it to be 128B aligned.

> @@ -397,7 +400,7 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
>  	/* io-mmu-map the skb */
>  	descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
>  						     descr->skb->data,
> -						     GELIC_NET_MAX_MTU,
> +						     gelic_rx_skb_size,
>  						     DMA_FROM_DEVICE));
>  	if (!descr->buf_addr) {
>  		dev_kfree_skb_any(descr->skb);
> @@ -915,7 +918,7 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
>  	data_error = be32_to_cpu(descr->data_error);
>  	/* unmap skb buffer */
>  	dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr),
> -			 GELIC_NET_MAX_MTU,
> +			 gelic_rx_skb_size,
>  			 DMA_FROM_DEVICE);
>  
>  	skb_put(skb, be32_to_cpu(descr->valid_size)?
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> index 68f324ed4eaf..0d98defb011e 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> @@ -19,8 +19,9 @@
>  #define GELIC_NET_RX_DESCRIPTORS        128 /* num of descriptors */
>  #define GELIC_NET_TX_DESCRIPTORS        128 /* num of descriptors */
>  
> -#define GELIC_NET_MAX_MTU               VLAN_ETH_FRAME_LEN
> -#define GELIC_NET_MIN_MTU               VLAN_ETH_ZLEN
> +#define GELIC_NET_MAX_FRAME             2312
> +#define GELIC_NET_MAX_MTU               2294
> +#define GELIC_NET_MIN_MTU               64
>  #define GELIC_NET_RXBUF_ALIGN           128
>  #define GELIC_CARD_RX_CSUM_DEFAULT      1 /* hw chksum */
>  #define GELIC_NET_WATCHDOG_TIMEOUT      5*HZ