| Message ID | 20210205163434.14D94C433ED@smtp.codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | pull-request: wireless-drivers-2021-02-05 | expand |
On Fri, 5 Feb 2021 16:34:34 +0000 (UTC) Kalle Valo wrote: > Hi, > > here's a pull request to net tree, more info below. Please let me know if there > are any problems. Pulled, thanks! One thing to confirm tho.. > ath9k > > * fix build regression related to LEDS_CLASS > > mt76 > > * fix a memory leak Lorenzo, I'm just guessing what this code does, but you're dropping a frag without invalidating the rest of the SKB, which I presume is now truncated? Shouldn't the skb be dropped?
Hello: This pull request was applied to netdev/net.git (refs/heads/master): On Fri, 5 Feb 2021 16:34:34 +0000 (UTC) you wrote: > Hi, > > here's a pull request to net tree, more info below. Please let me know if there > are any problems. > > Kalle > > [...] Here is the summary with links: - pull-request: wireless-drivers-2021-02-05 https://git.kernel.org/netdev/net/c/2da4b24b1dfb You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
> On Fri, 5 Feb 2021 16:34:34 +0000 (UTC) Kalle Valo wrote: > > Hi, > > > > here's a pull request to net tree, more info below. Please let me know if there > > are any problems. > > Pulled, thanks! One thing to confirm tho.. > > > ath9k > > > > * fix build regression related to LEDS_CLASS > > > > mt76 > > > > * fix a memory leak > > Lorenzo, I'm just guessing what this code does, but you're dropping a > frag without invalidating the rest of the SKB, which I presume is now > truncated? Shouldn't the skb be dropped? > Hi Jakub, I agree. We can do something like: diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c index e81dfaf99bcb..6d84533d1df2 100644 --- a/drivers/net/wireless/mediatek/mt76/dma.c +++ b/drivers/net/wireless/mediatek/mt76/dma.c @@ -511,8 +511,9 @@ mt76_add_fragment(struct mt76_dev *dev, struct mt76_queue *q, void *data, { struct sk_buff *skb = q->rx_head; struct skb_shared_info *shinfo = skb_shinfo(skb); + int nr_frags = shinfo->nr_frags; - if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) { + if (nr_frags < ARRAY_SIZE(shinfo->frags)) { struct page *page = virt_to_head_page(data); int offset = data - page_address(page) + q->buf_offset; @@ -526,7 +527,10 @@ mt76_add_fragment(struct mt76_dev *dev, struct mt76_queue *q, void *data, return; q->rx_head = NULL; - dev->drv->rx_skb(dev, q - dev->q_rx, skb); + if (nr_frags < ARRAY_SIZE(shinfo->frags)) + dev->drv->rx_skb(dev, q - dev->q_rx, skb); + else + dev_kfree_skb(skb); } I do not know if it can occur, but I guess we should even check q->rx_head pointer before overwriting it because if the hw does not report more set to false for last fragment we will get a memory leak as well. Something like: @@ -578,6 +582,8 @@ mt76_dma_rx_process(struct mt76_dev *dev, struct mt76_queue *q, int budget) done++; if (more) { + if (q->rx_head) + dev_kfree_skb(q->rx_head); q->rx_head = skb; continue; } Regards, Lorenzo
On Sat, 6 Feb 2021 20:43:25 +0100 Lorenzo Bianconi wrote: > > Lorenzo, I'm just guessing what this code does, but you're dropping a > > frag without invalidating the rest of the SKB, which I presume is now > > truncated? Shouldn't the skb be dropped? > > > > Hi Jakub, > > I agree. We can do something like: > > diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c > index e81dfaf99bcb..6d84533d1df2 100644 > --- a/drivers/net/wireless/mediatek/mt76/dma.c > +++ b/drivers/net/wireless/mediatek/mt76/dma.c > @@ -511,8 +511,9 @@ mt76_add_fragment(struct mt76_dev *dev, struct mt76_queue *q, void *data, > { > struct sk_buff *skb = q->rx_head; > struct skb_shared_info *shinfo = skb_shinfo(skb); > + int nr_frags = shinfo->nr_frags; > > - if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) { > + if (nr_frags < ARRAY_SIZE(shinfo->frags)) { > struct page *page = virt_to_head_page(data); > int offset = data - page_address(page) + q->buf_offset; > > @@ -526,7 +527,10 @@ mt76_add_fragment(struct mt76_dev *dev, struct mt76_queue *q, void *data, > return; > > q->rx_head = NULL; > - dev->drv->rx_skb(dev, q - dev->q_rx, skb); > + if (nr_frags < ARRAY_SIZE(shinfo->frags)) > + dev->drv->rx_skb(dev, q - dev->q_rx, skb); > + else > + dev_kfree_skb(skb); > } > > > I do not know if it can occur, but I guess we should even check q->rx_head > pointer before overwriting it because if the hw does not report more set to > false for last fragment we will get a memory leak as well. Something like: > > @@ -578,6 +582,8 @@ mt76_dma_rx_process(struct mt76_dev *dev, struct mt76_queue *q, int budget) > done++; > > if (more) { > + if (q->rx_head) > + dev_kfree_skb(q->rx_head); > q->rx_head = skb; > continue; > }
Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes: >> On Fri, 5 Feb 2021 16:34:34 +0000 (UTC) Kalle Valo wrote: >> > Hi, >> > >> > here's a pull request to net tree, more info below. Please let me know if there >> > are any problems. >> >> Pulled, thanks! One thing to confirm tho.. >> >> > ath9k >> > >> > * fix build regression related to LEDS_CLASS >> > >> > mt76 >> > >> > * fix a memory leak >> >> Lorenzo, I'm just guessing what this code does, but you're dropping a >> frag without invalidating the rest of the SKB, which I presume is now >> truncated? Shouldn't the skb be dropped? >> > > Hi Jakub, > > I agree. We can do something like: > > diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c > index e81dfaf99bcb..6d84533d1df2 100644 > --- a/drivers/net/wireless/mediatek/mt76/dma.c > +++ b/drivers/net/wireless/mediatek/mt76/dma.c > @@ -511,8 +511,9 @@ mt76_add_fragment(struct mt76_dev *dev, struct mt76_queue *q, void *data, > { > struct sk_buff *skb = q->rx_head; > struct skb_shared_info *shinfo = skb_shinfo(skb); > + int nr_frags = shinfo->nr_frags; > > - if (shinfo->nr_frags < ARRAY_SIZE(shinfo->frags)) { > + if (nr_frags < ARRAY_SIZE(shinfo->frags)) { > struct page *page = virt_to_head_page(data); > int offset = data - page_address(page) + q->buf_offset; > > @@ -526,7 +527,10 @@ mt76_add_fragment(struct mt76_dev *dev, struct mt76_queue *q, void *data, > return; > > q->rx_head = NULL; > - dev->drv->rx_skb(dev, q - dev->q_rx, skb); > + if (nr_frags < ARRAY_SIZE(shinfo->frags)) > + dev->drv->rx_skb(dev, q - dev->q_rx, skb); > + else > + dev_kfree_skb(skb); > } > > > I do not know if it can occur, but I guess we should even check q->rx_head > pointer before overwriting it because if the hw does not report more set to > false for last fragment we will get a memory leak as well. Something like: > > @@ -578,6 +582,8 @@ mt76_dma_rx_process(struct mt76_dev *dev, struct mt76_queue *q, int budget) > done++; > > if (more) { > + if (q->rx_head) > + dev_kfree_skb(q->rx_head); > q->rx_head = skb; > continue; > } So what's the plan? Is there going to be a followup patch? And should that also go to v5.11 or can it wait v5.12?
> [...] > > So what's the plan? Is there going to be a followup patch? And should > that also go to v5.11 or can it wait v5.12? > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches > Hi Kalle, I will post two followup patches later today. I think the issues are not harmful but it will be easier to post them to wireless-drivers tree, agree? Regards, Lorenzo
> > > > > [...] > > > > > So what's the plan? Is there going to be a followup patch? And should > > that also go to v5.11 or can it wait v5.12? > > > > -- > > https://patchwork.kernel.org/project/linux-wireless/list/ > > > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches > > > > Hi Kalle, > Hi Kalle, > I will post two followup patches later today. I think the issues are > not harmful but it will be easier to post them to wireless-drivers > tree, agree? > carefully reviewing the code, I do not think the second one is a real issue, so I have only posted a fix to address Jakub's comment. Regards, Lorenzo > Regards, > Lorenzo
Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes: >> So what's the plan? Is there going to be a followup patch? And should >> that also go to v5.11 or can it wait v5.12? >> >> -- >> https://patchwork.kernel.org/project/linux-wireless/list/ >> >> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches >> > > Hi Kalle, > > I will post two followup patches later today. I think the issues are > not harmful but it will be easier to post them to wireless-drivers > tree, agree? Most likely Linus releases the final v5.11 next Sunday, so we are very close to release. If this is not urgent I would rather wait for the merge window to open (on Sunday) and apply the patch for v5.12 to avoid a last minute rush. Would that work?
On Feb 08, Kalle Valo wrote: > Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes: > > >> So what's the plan? Is there going to be a followup patch? And should > >> that also go to v5.11 or can it wait v5.12? > >> > >> -- > >> https://patchwork.kernel.org/project/linux-wireless/list/ > >> > >> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches > >> > > > > Hi Kalle, > > > > I will post two followup patches later today. I think the issues are > > not harmful but it will be easier to post them to wireless-drivers > > tree, agree? > > Most likely Linus releases the final v5.11 next Sunday, so we are very > close to release. If this is not urgent I would rather wait for the > merge window to open (on Sunday) and apply the patch for v5.12 to avoid > a last minute rush. Would that work? Sure, I guess it is not urgent. Regards, Lorenzo > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Mon, 8 Feb 2021 09:22:53 +0100 Lorenzo Bianconi wrote: > > > I will post two followup patches later today. I think the issues are > > > not harmful but it will be easier to post them to wireless-drivers > > > tree, agree? > > > > Most likely Linus releases the final v5.11 next Sunday, so we are very > > close to release. If this is not urgent I would rather wait for the > > merge window to open (on Sunday) and apply the patch for v5.12 to avoid > > a last minute rush. Would that work? > > Sure, I guess it is not urgent. Agreed, thanks!
Hi, here's a pull request to net tree, more info below. Please let me know if there are any problems. Kalle The following changes since commit 0acb20a5438c36e0cf2b8bf255f314b59fcca6ef: mt7601u: fix kernel crash unplugging the device (2021-01-25 16:02:52 +0200) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git tags/wireless-drivers-2021-02-05 for you to fetch changes up to 93a1d4791c10d443bc67044def7efee2991d48b7: mt76: dma: fix a possible memory leak in mt76_add_fragment() (2021-01-28 09:30:37 +0200) ---------------------------------------------------------------- wireless-drivers fixes for v5.11 Third, and most likely the last, set of fixes for v5.11. Two very small fixes. ath9k * fix build regression related to LEDS_CLASS mt76 * fix a memory leak ---------------------------------------------------------------- Arnd Bergmann (1): ath9k: fix build error with LEDS_CLASS=m Lorenzo Bianconi (1): mt76: dma: fix a possible memory leak in mt76_add_fragment() drivers/net/wireless/ath/ath9k/Kconfig | 8 ++------ drivers/net/wireless/mediatek/mt76/dma.c | 8 +++++--- net/mac80211/Kconfig | 2 +- 3 files changed, 8 insertions(+), 10 deletions(-)