Re: [PATCH] wifi: iwlwifi: correctly lookup DMA address in SG table

[Ben Greear <greearb@candelatech.com>]

On 8/8/24 10:29, Benjamin Berg wrote:
> From: Benjamin Berg <benjamin.berg@intel.com>

Hello,

We'll try testing this on our systems...

Thanks,
Ben

> 
> The code to lookup the scatter gather table entry assumed that it was
> possible to use sg_virt() in order to lookup the DMA address in a mapped
> scatter gather table. However, this assumption is incorrect as the DMA
> mapping code may merge multiple entries into one. In that case, the DMA
> address space may have e.g. two consecutive pages which is correctly
> represented by the scatter gather list entry, however the virtual
> addresses for these two pages may differ and the relationship cannot be
> resolved anymore.
> 
> Avoid this problem entirely by working with the offset into the mapped
> area instead of using virtual addresses. With that we only use the DMA
> length and DMA address from the scatter gather list entries. The
> underlying DMA/IOMMU code is therefore free to merge two entries into
> one even if the virtual addresses space for the area is not continuous.
> 
> Fixes: 90db50755228 ("wifi: iwlwifi: use already mapped data when TXing an AMSDU")
> Reported-by: Chris Bainbridge <chris.bainbridge@gmail.com>
> Closes: https://lore.kernel.org/r/ZrNRoEbdkxkKFMBi@debian.local
> Signed-off-by: Benjamin Berg <benjamin.berg@intel.com>
> 
> ---
> 
> Note that I was not yet able to fully verify this patch, but will
> probably get results tomorrow. Unfortunately, much of the testing we do
> internally happens on machines that do not reproduce the problem.
> 
> Also, I think Ben already reported the same issue much earlier. At the
> time, I was not yet aware of the internal reproductions and did not take
> the report seriously.
> ---
>   .../wireless/intel/iwlwifi/pcie/internal.h    |  2 +-
>   .../net/wireless/intel/iwlwifi/pcie/tx-gen2.c |  4 ++-
>   drivers/net/wireless/intel/iwlwifi/pcie/tx.c  | 25 +++++++++++--------
>   3 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
> index b59de4f80b4b..3af9c2b40ef1 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/internal.h
> @@ -639,7 +639,7 @@ void iwl_trans_pcie_tx_reset(struct iwl_trans *trans);
>   int iwl_pcie_txq_alloc(struct iwl_trans *trans, struct iwl_txq *txq,
>   		       int slots_num, bool cmd_queue);
>   
> -dma_addr_t iwl_pcie_get_sgt_tb_phys(struct sg_table *sgt, void *addr);
> +dma_addr_t iwl_pcie_get_sgt_tb_phys(struct sg_table *sgt, unsigned int offset);
>   struct sg_table *iwl_pcie_prep_tso(struct iwl_trans *trans, struct sk_buff *skb,
>   				   struct iwl_cmd_meta *cmd_meta,
>   				   u8 **hdr, unsigned int hdr_room);
> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c b/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
> index 2e780fb2da42..54d26523f692 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/tx-gen2.c
> @@ -168,6 +168,7 @@ static int iwl_txq_gen2_build_amsdu(struct iwl_trans *trans,
>   	struct ieee80211_hdr *hdr = (void *)skb->data;
>   	unsigned int snap_ip_tcp_hdrlen, ip_hdrlen, total_len, hdr_room;
>   	unsigned int mss = skb_shinfo(skb)->gso_size;
> +	unsigned int data_offset = 0;
>   	dma_addr_t start_hdr_phys;
>   	u16 length, amsdu_pad;
>   	u8 *start_hdr;
> @@ -260,7 +261,7 @@ static int iwl_txq_gen2_build_amsdu(struct iwl_trans *trans,
>   			int ret;
>   
>   			tb_len = min_t(unsigned int, tso.size, data_left);
> -			tb_phys = iwl_pcie_get_sgt_tb_phys(sgt, tso.data);
> +			tb_phys = iwl_pcie_get_sgt_tb_phys(sgt, data_offset);
>   			/* Not a real mapping error, use direct comparison */
>   			if (unlikely(tb_phys == DMA_MAPPING_ERROR))
>   				goto out_err;
> @@ -272,6 +273,7 @@ static int iwl_txq_gen2_build_amsdu(struct iwl_trans *trans,
>   				goto out_err;
>   
>   			data_left -= tb_len;
> +			data_offset += tb_len;
>   			tso_build_data(skb, &tso, tb_len);
>   		}
>   	}
> diff --git a/drivers/net/wireless/intel/iwlwifi/pcie/tx.c b/drivers/net/wireless/intel/iwlwifi/pcie/tx.c
> index 22d482ae53d9..78f417cdb9ac 100644
> --- a/drivers/net/wireless/intel/iwlwifi/pcie/tx.c
> +++ b/drivers/net/wireless/intel/iwlwifi/pcie/tx.c
> @@ -1814,23 +1814,24 @@ static void *iwl_pcie_get_page_hdr(struct iwl_trans *trans,
>   /**
>    * iwl_pcie_get_sgt_tb_phys - Find TB address in mapped SG list
>    * @sgt: scatter gather table
> - * @addr: Virtual address
> + * @offset: Offset into the mapped memory (i.e. SKB payload data)
>    *
> - * Find the entry that includes the address for the given address and return
> - * correct physical address for the TB entry.
> + * Find the DMA address that corresponds to the SKB payload data at the
> + * position given by @offset.
>    *
>    * Returns: Address for TB entry
>    */
> -dma_addr_t iwl_pcie_get_sgt_tb_phys(struct sg_table *sgt, void *addr)
> +dma_addr_t iwl_pcie_get_sgt_tb_phys(struct sg_table *sgt, unsigned int offset)
>   {
>   	struct scatterlist *sg;
> +	unsigned int sg_offset = 0;
>   	int i;
>   
>   	for_each_sgtable_dma_sg(sgt, sg, i) {
> -		if (addr >= sg_virt(sg) &&
> -		    (u8 *)addr < (u8 *)sg_virt(sg) + sg_dma_len(sg))
> -			return sg_dma_address(sg) +
> -			       ((unsigned long)addr - (unsigned long)sg_virt(sg));
> +		if (offset >= sg_offset && offset < sg_offset + sg_dma_len(sg))
> +			return sg_dma_address(sg) + offset - sg_offset;
> +
> +		sg_offset += sg_dma_len(sg);
>   	}
>   
>   	WARN_ON_ONCE(1);
> @@ -1875,7 +1876,9 @@ struct sg_table *iwl_pcie_prep_tso(struct iwl_trans *trans, struct sk_buff *skb,
>   
>   	sg_init_table(sgt->sgl, skb_shinfo(skb)->nr_frags + 1);
>   
> -	sgt->orig_nents = skb_to_sgvec(skb, sgt->sgl, 0, skb->len);
> +	/* Only map the data, not the header (it is copied to the TSO page) */
> +	sgt->orig_nents = skb_to_sgvec(skb, sgt->sgl, skb_headlen(skb),
> +				       skb->data_len);
>   	if (WARN_ON_ONCE(sgt->orig_nents <= 0))
>   		return NULL;
>   
> @@ -1900,6 +1903,7 @@ static int iwl_fill_data_tbs_amsdu(struct iwl_trans *trans, struct sk_buff *skb,
>   	struct ieee80211_hdr *hdr = (void *)skb->data;
>   	unsigned int snap_ip_tcp_hdrlen, ip_hdrlen, total_len, hdr_room;
>   	unsigned int mss = skb_shinfo(skb)->gso_size;
> +	unsigned int data_offset = 0;
>   	u16 length, iv_len, amsdu_pad;
>   	dma_addr_t start_hdr_phys;
>   	u8 *start_hdr, *pos_hdr;
> @@ -2000,7 +2004,7 @@ static int iwl_fill_data_tbs_amsdu(struct iwl_trans *trans, struct sk_buff *skb,
>   						  data_left);
>   			dma_addr_t tb_phys;
>   
> -			tb_phys = iwl_pcie_get_sgt_tb_phys(sgt, tso.data);
> +			tb_phys = iwl_pcie_get_sgt_tb_phys(sgt, data_offset);
>   			/* Not a real mapping error, use direct comparison */
>   			if (unlikely(tb_phys == DMA_MAPPING_ERROR))
>   				return -EINVAL;
> @@ -2011,6 +2015,7 @@ static int iwl_fill_data_tbs_amsdu(struct iwl_trans *trans, struct sk_buff *skb,
>   						tb_phys, size);
>   
>   			data_left -= size;
> +			data_offset += size;
>   			tso_build_data(skb, &tso, size);
>   		}
>   	}

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com