| Message ID | 20240808172948.303258-1-benjamin@sipsolutions.net (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: iwlwifi: correctly lookup DMA address in SG table | expand |
Benjamin Berg <benjamin@sipsolutions.net> writes: > From: Benjamin Berg <benjamin.berg@intel.com> > > 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> Reminder to myself: if this passes the tests it should go to wireless tree. Assigned the patch to me on patchwork. Miri, if you agree please give an Acked-by.
On Thu, 8 Aug 2024 at 18:32, Benjamin Berg <benjamin@sipsolutions.net> wrote: > [...] Works for me. Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
Hi, We have tested this on our system and verified that we are no longer seeing the issue with this patch applied. - Rory On 8/8/24 10:36, Ben Greear wrote: > 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); >> } >> } >
Benjamin Berg <benjamin@sipsolutions.net> wrote: > From: Benjamin Berg <benjamin.berg@intel.com> > > 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> > Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com> v2: https://patchwork.kernel.org/project/linux-wireless/patch/20240812110640.460514-1-benjamin@sipsolutions.net/ Patch set to Superseded.
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); } }