Re: [PATCH net v2 0/2] ps3_gelic_net: DMA related fixes

Re: [PATCH net v2 0/2] ps3_gelic_net: DMA related fixes

[Jakub Kicinski]

On Mon, 23 Jan 2023 01:24:25 +0000 Geoff Levand wrote:
> The following changes since commit 71ab9c3e2253619136c31c89dbb2c69305cc89b1:
> 
>   net: fix UaF in netns ops registration error path (2023-01-20 18:51:18 -0800)
> 
> are available in the Git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/geoff/ps3-linux.git for-merge-net-v2
> 
> for you to fetch changes up to dca415c1429e4ca57d525b3595513a323cca303e:
> 
>   net/ps3_gelic_net: Use dma_mapping_error (2023-01-22 17:08:58 -0800)

Any particular reason you sent a PR?
We generally apply patches from the list, it's simpler.

Re: [PATCH net v2 1/2] net/ps3_gelic_net: Fix RX skbuff length

[Jakub Kicinski]

The patches didn't make it to patchwork or the lore archive.
Keep an eye for any irregularities when reposting.

On Mon, 23 Jan 2023 01:24:25 +0000 Geoff Levand wrote:
> The Gelic Etherenet device needs to have the RX skbuffs aligned to 128 bytes.
> The current Gelic Etherenet driver was not allocating skbuffs large enough to
> allow for this alignment.
> 
> This change adds a new local structure named aligned_buff to help calculate a
> buffer size large enough to allow for this alignment.
> 
> Fixes various randomly occurring runtime network errors.
> 
> Signed-off-by: Geoff Levand <geoff@infradead.org>

Please add a Fixes tag.

> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index cf8de8a7a8a1..43e438f8f595 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -366,50 +366,67 @@ 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
>   */
> +

Why this new line?

>  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 aligned_buff {
> +		unsigned int total_bytes;
> +		unsigned int offset;
> +	};
> +	struct aligned_buff a_buf;

You can declare this as a anonymous struct:

	struct {
		unsigned int total_bytes;
		unsigned int offset;
	} a_buf;

> +	dma_addr_t cpu_addr;
> +
> +	if (gelic_descr_get_status(descr) !=  GELIC_DESCR_DMA_NOT_IN_USE) {
> +		dev_err(dev, "%s:%d: ERROR status\n", __func__, __LINE__);
> +	}

The fixes should be minimal, please don't change prints or reformat 
the code unless it makes it a lot easier to understand.

> +	a_buf.total_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN)
> +		+ 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);
> +	descr->skb = dev_alloc_skb(a_buf.total_bytes);
>  
> -	/* 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);

So what did you change? This is hard to read.
Allocating ALIGN - 1 more space should be enough to align the pointer.

>  	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);
> +
> +	a_buf.offset = PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN)
> +		- descr->skb->data;
> +
> +	if (a_buf.offset) {
> +		dev_dbg(dev, "%s:%d: offset=%u\n", __func__, __LINE__,
> +			a_buf.offset);
> +		skb_reserve(descr->skb, a_buf.offset);
> +	}
> +
> +	descr->buf_size = a_buf.total_bytes - a_buf.offset;
>  	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));
> -	if (!descr->buf_addr) {
> +	cpu_addr = dma_map_single(dev, descr->skb->data,
> +		descr->buf_size, DMA_FROM_DEVICE);
> +	descr->buf_addr = cpu_to_be32(cpu_addr);
> +
> +	if (unlikely(dma_mapping_error(dev, cpu_addr))) {

adding dma mapping error handling should be a separate fix

> +		dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
> +
> +		descr->buf_addr = 0;
> +		descr->buf_size = 0;
> +
>  		dev_kfree_skb_any(descr->skb);
>  		descr->skb = NULL;
> -		dev_info(ctodev(card),
> -			 "%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;
>  }
>  
>  /**