| Message ID | 20200802132949.26788-1-baijiaju@tsinghua.edu.cn (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 478762855b5ae9f68fa6ead1edf7abada70fcd5f |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | p54: avoid accessing the data mapped to streaming DMA | expand |
Jia-Ju Bai <baijiaju@tsinghua.edu.cn> wrote: > In p54p_tx(), skb->data is mapped to streaming DMA on line 337: > mapping = pci_map_single(..., skb->data, ...); > > Then skb->data is accessed on line 349: > desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; > > This access may cause data inconsistency between CPU cache and hardware. > > To fix this problem, ((struct p54_hdr *)skb->data)->req_id is stored in > a local variable before DMA mapping, and then the driver accesses this > local variable instead of skb->data. > > Signed-off-by: Jia-Ju Bai <baijiaju@tsinghua.edu.cn> Can someone review this?
Hello, sorry. Your mail got flagged by google mail as spam. :( On 2020-08-02 15:29, Jia-Ju Bai wrote: > In p54p_tx(), skb->data is mapped to streaming DMA on line 337: > mapping = pci_map_single(..., skb->data, ...); > > Then skb->data is accessed on line 349: > desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; > > This access may cause data inconsistency between CPU cache and hardware. > > To fix this problem, ((struct p54_hdr *)skb->data)->req_id is stored in > a local variable before DMA mapping, and then the driver accesses this > local variable instead of skb->data. Interesting. Please bear with me here. From my understanding, the streaming direction is set to PCI_DMA_TODEVICE. So is it really possible for the hardware to interfere with the data without the IOMMU catching this? (That said, patch looks be fine. I'll need to dust off a old PCI PC to check this with real hardware, if requested.) Cheers, Christian > > Signed-off-by: Jia-Ju Bai <baijiaju@tsinghua.edu.cn> > --- > drivers/net/wireless/intersil/p54/p54pci.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/intersil/p54/p54pci.c b/drivers/net/wireless/intersil/p54/p54pci.c > index 80ad0b7eaef4..f8c6027cab6b 100644 > --- a/drivers/net/wireless/intersil/p54/p54pci.c > +++ b/drivers/net/wireless/intersil/p54/p54pci.c > @@ -329,10 +329,12 @@ static void p54p_tx(struct ieee80211_hw *dev, struct sk_buff *skb) > struct p54p_desc *desc; > dma_addr_t mapping; > u32 idx, i; > + __le32 device_addr; > > spin_lock_irqsave(&priv->lock, flags); > idx = le32_to_cpu(ring_control->host_idx[1]); > i = idx % ARRAY_SIZE(ring_control->tx_data); > + device_addr = ((struct p54_hdr *)skb->data)->req_id; > > mapping = pci_map_single(priv->pdev, skb->data, skb->len, > PCI_DMA_TODEVICE); > @@ -346,7 +348,7 @@ static void p54p_tx(struct ieee80211_hw *dev, struct sk_buff *skb) > > desc = &ring_control->tx_data[i]; > desc->host_addr = cpu_to_le32(mapping); > - desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; > + desc->device_addr = device_addr; > desc->len = cpu_to_le16(skb->len); > desc->flags = 0; > >
Christian Lamparter <chunkeey@gmail.com> writes: > On 2020-08-02 15:29, Jia-Ju Bai wrote: >> In p54p_tx(), skb->data is mapped to streaming DMA on line 337: >> mapping = pci_map_single(..., skb->data, ...); >> >> Then skb->data is accessed on line 349: >> desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; >> >> This access may cause data inconsistency between CPU cache and hardware. >> >> To fix this problem, ((struct p54_hdr *)skb->data)->req_id is stored in >> a local variable before DMA mapping, and then the driver accesses this >> local variable instead of skb->data. > > Interesting. Please bear with me here. From my understanding, the > streaming direction is set to PCI_DMA_TODEVICE. So is it really > possible for the hardware to interfere with the data without the IOMMU > catching this? Also is there any documentation about this scenario? I would like to understand this better. > (That said, patch looks be fine. I'll need to dust off a old PCI PC to > check this with real hardware, if requested.) No need to test with real hardware for my sake, but a careful review is very much appreciated.
On 2020-08-26 18:02, Kalle Valo wrote: > Christian Lamparter <chunkeey@gmail.com> writes: > >> On 2020-08-02 15:29, Jia-Ju Bai wrote: >>> In p54p_tx(), skb->data is mapped to streaming DMA on line 337: >>> mapping = pci_map_single(..., skb->data, ...); >>> >>> Then skb->data is accessed on line 349: >>> desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; >>> >>> This access may cause data inconsistency between CPU cache and hardware. >>> >>> To fix this problem, ((struct p54_hdr *)skb->data)->req_id is stored in >>> a local variable before DMA mapping, and then the driver accesses this >>> local variable instead of skb->data. >> >> Interesting. Please bear with me here. From my understanding, the >> streaming direction is set to PCI_DMA_TODEVICE. So is it really >> possible for the hardware to interfere with the data without the IOMMU >> catching this? > > Also is there any documentation about this scenario? I would like to > understand this better. I usually rely on the information present in Documentation: <https://www.kernel.org/doc/Documentation/DMA-API-HOWTO.txt> The relevant extract for p54's DMA_TO_DEVICE decision likely comes from: "For Networking drivers, it's a rather simple affair. For transmit packets, map/unmap them with the DMA_TO_DEVICE direction specifier. For receive packets, just the opposite, map/unmap them with the DMA_FROM_DEVICE direction specifier." "Only streaming mappings specify a direction, consistent mappings implicitly have a direction attribute setting of DMA_BIDIRECTIONAL." But looking around on the Internet, I came across this in "Chapter 15. Memory Mapping and DMA" of the Linux Device Drivers, 3rd Edition: <https://www.oreilly.com/library/view/linux-device-drivers/0596005903/ch15.html> |Setting up streaming DMA mappings |[...] | |Some important rules apply to streaming DMA mappings: | * [...] | | * Once a buffer has been mapped, it belongs to the device, not the | processor. Until the buffer has been unmapped, the driver should not | touch its contents in any way. Only after dma_unmap_single has been | called is it safe for the driver to access the contents of the | buffer (with one exception that we see shortly). Among other things, | this rule implies that a buffer being written to a device cannot be | mapped until it contains all the data to write." | | [...] (More informative text, but only) From the sentence "Once a buffer has been mapped, it belongs to the device, not the processor". I think that Jia-Ju Bai's patch is doing exactly this "by the book". Therefore, it should be applied and backported: Cc: <stable@vger.kernel.org> Acked-by: Christian Lamparter <chunkeey@gmail.com> Cheers, Christian
Christian Lamparter <chunkeey@gmail.com> writes: > On 2020-08-26 18:02, Kalle Valo wrote: >> Christian Lamparter <chunkeey@gmail.com> writes: >> >>> On 2020-08-02 15:29, Jia-Ju Bai wrote: >>>> In p54p_tx(), skb->data is mapped to streaming DMA on line 337: >>>> mapping = pci_map_single(..., skb->data, ...); >>>> >>>> Then skb->data is accessed on line 349: >>>> desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; >>>> >>>> This access may cause data inconsistency between CPU cache and hardware. >>>> >>>> To fix this problem, ((struct p54_hdr *)skb->data)->req_id is stored in >>>> a local variable before DMA mapping, and then the driver accesses this >>>> local variable instead of skb->data. >>> >>> Interesting. Please bear with me here. From my understanding, the >>> streaming direction is set to PCI_DMA_TODEVICE. So is it really >>> possible for the hardware to interfere with the data without the IOMMU >>> catching this? >> >> Also is there any documentation about this scenario? I would like to >> understand this better. > > I usually rely on the information present in Documentation: > <https://www.kernel.org/doc/Documentation/DMA-API-HOWTO.txt> > > The relevant extract for p54's DMA_TO_DEVICE decision likely comes from: > > "For Networking drivers, it's a rather simple affair. For transmit > packets, map/unmap them with the DMA_TO_DEVICE direction > specifier. For receive packets, just the opposite, map/unmap them > with the DMA_FROM_DEVICE direction specifier." > > "Only streaming mappings specify a direction, consistent mappings > implicitly have a direction attribute setting of DMA_BIDIRECTIONAL." This is not very clearly written, I guess it's assumed everyone know this stuff :) > But looking around on the Internet, I came across this in "Chapter 15. > Memory Mapping and DMA" of the Linux Device Drivers, 3rd Edition: > > <https://www.oreilly.com/library/view/linux-device-drivers/0596005903/ch15.html> > > |Setting up streaming DMA mappings > |[...] > | > |Some important rules apply to streaming DMA mappings: > | * [...] > | > | * Once a buffer has been mapped, it belongs to the device, not the > | processor. Until the buffer has been unmapped, the driver should > not | touch its contents in any way. Only after dma_unmap_single has > been | called is it safe for the driver to access the contents of > the > | buffer (with one exception that we see shortly). Among other things, > | this rule implies that a buffer being written to a device cannot be > | mapped until it contains all the data to write." > | > | [...] (More informative text, but only) > > From the sentence "Once a buffer has been mapped, it belongs to the > device, not the processor". I think that Jia-Ju Bai's patch is doing > exactly this "by the book". Yeah, this is much better and understandable. Thanks for checking. > Therefore, it should be applied and backported: > > Cc: <stable@vger.kernel.org> Ok, I'll add that.
Jia-Ju Bai <baijiaju@tsinghua.edu.cn> wrote: > In p54p_tx(), skb->data is mapped to streaming DMA on line 337: > mapping = pci_map_single(..., skb->data, ...); > > Then skb->data is accessed on line 349: > desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; > > This access may cause data inconsistency between CPU cache and hardware. > > To fix this problem, ((struct p54_hdr *)skb->data)->req_id is stored in > a local variable before DMA mapping, and then the driver accesses this > local variable instead of skb->data. > > Cc: <stable@vger.kernel.org> > Signed-off-by: Jia-Ju Bai <baijiaju@tsinghua.edu.cn> > Acked-by: Christian Lamparter <chunkeey@gmail.com> Patch applied to wireless-drivers-next.git, thanks. 478762855b5a p54: avoid accessing the data mapped to streaming DMA
diff --git a/drivers/net/wireless/intersil/p54/p54pci.c b/drivers/net/wireless/intersil/p54/p54pci.c index 80ad0b7eaef4..f8c6027cab6b 100644 --- a/drivers/net/wireless/intersil/p54/p54pci.c +++ b/drivers/net/wireless/intersil/p54/p54pci.c @@ -329,10 +329,12 @@ static void p54p_tx(struct ieee80211_hw *dev, struct sk_buff *skb) struct p54p_desc *desc; dma_addr_t mapping; u32 idx, i; + __le32 device_addr; spin_lock_irqsave(&priv->lock, flags); idx = le32_to_cpu(ring_control->host_idx[1]); i = idx % ARRAY_SIZE(ring_control->tx_data); + device_addr = ((struct p54_hdr *)skb->data)->req_id; mapping = pci_map_single(priv->pdev, skb->data, skb->len, PCI_DMA_TODEVICE); @@ -346,7 +348,7 @@ static void p54p_tx(struct ieee80211_hw *dev, struct sk_buff *skb) desc = &ring_control->tx_data[i]; desc->host_addr = cpu_to_le32(mapping); - desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; + desc->device_addr = device_addr; desc->len = cpu_to_le16(skb->len); desc->flags = 0;
In p54p_tx(), skb->data is mapped to streaming DMA on line 337: mapping = pci_map_single(..., skb->data, ...); Then skb->data is accessed on line 349: desc->device_addr = ((struct p54_hdr *)skb->data)->req_id; This access may cause data inconsistency between CPU cache and hardware. To fix this problem, ((struct p54_hdr *)skb->data)->req_id is stored in a local variable before DMA mapping, and then the driver accesses this local variable instead of skb->data. Signed-off-by: Jia-Ju Bai <baijiaju@tsinghua.edu.cn> --- drivers/net/wireless/intersil/p54/p54pci.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)