Svace has identified unchecked return value of mt7615_init_tx_queue function in 5.10 branch, even though it makes sense to track it instead. This issue is fixed in upstream version by Lorenzo's patch.
The same patch can be cleanly applied to the 5.10 branch.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
From: Lorenzo Bianconi lorenzo@kernel.org
commit b671da33d1c5973f90f098ff66a91953691df582 upstream.
Move mt76_init_tx_queue in mac80211.c since it is shared by all drivers.
Signed-off-by: Lorenzo Bianconi lorenzo@kernel.org Signed-off-by: Felix Fietkau nbd@nbd.name Signed-off-by: Nikita Zhandarovich n.zhandarovich@fintech.ru --- drivers/net/wireless/mediatek/mt76/mac80211.c | 21 ++++++++ drivers/net/wireless/mediatek/mt76/mt76.h | 4 ++ .../net/wireless/mediatek/mt76/mt7603/dma.c | 10 +--- .../net/wireless/mediatek/mt76/mt7615/dma.c | 50 +++++++------------ .../net/wireless/mediatek/mt76/mt76x02_mmio.c | 10 +--- .../net/wireless/mediatek/mt76/mt7915/dma.c | 48 +++++------------- 6 files changed, 58 insertions(+), 85 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c index 2bc1ef1cbfea..d48f09a3c539 100644 --- a/drivers/net/wireless/mediatek/mt76/mac80211.c +++ b/drivers/net/wireless/mediatek/mt76/mac80211.c @@ -1213,3 +1213,24 @@ int mt76_get_antenna(struct ieee80211_hw *hw, u32 *tx_ant, u32 *rx_ant) return 0; } EXPORT_SYMBOL_GPL(mt76_get_antenna); + +int mt76_init_tx_queue(struct mt76_dev *dev, int qid, int idx, + int n_desc, int ring_base) +{ + struct mt76_queue *hwq; + int err; + + hwq = devm_kzalloc(dev->dev, sizeof(*hwq), GFP_KERNEL); + if (!hwq) + return -ENOMEM; + + err = dev->queue_ops->alloc(dev, hwq, idx, n_desc, 0, ring_base); + if (err < 0) + return err; + + hwq->qid = qid; + dev->q_tx[qid] = hwq; + + return 0; +} +EXPORT_SYMBOL_GPL(mt76_init_tx_queue); diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h index 34b6d32ea1ec..63549a7806cb 100644 --- a/drivers/net/wireless/mediatek/mt76/mt76.h +++ b/drivers/net/wireless/mediatek/mt76/mt76.h @@ -134,6 +134,7 @@ struct mt76_queue {
u8 buf_offset; u8 hw_idx; + u8 qid;
dma_addr_t desc_dma; struct sk_buff *rx_head; @@ -778,6 +779,9 @@ void mt76_seq_puts_array(struct seq_file *file, const char *str, int mt76_eeprom_init(struct mt76_dev *dev, int len); void mt76_eeprom_override(struct mt76_dev *dev);
+int mt76_init_tx_queue(struct mt76_dev *dev, int qid, int idx, + int n_desc, int ring_base); + static inline struct mt76_phy * mt76_dev_phy(struct mt76_dev *dev, bool phy_ext) { diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/dma.c b/drivers/net/wireless/mediatek/mt76/mt7603/dma.c index d60d00f6f6a0..05a5801646d7 100644 --- a/drivers/net/wireless/mediatek/mt76/mt7603/dma.c +++ b/drivers/net/wireless/mediatek/mt76/mt7603/dma.c @@ -7,19 +7,13 @@ static int mt7603_init_tx_queue(struct mt7603_dev *dev, int qid, int idx, int n_desc) { - struct mt76_queue *hwq; int err;
- hwq = devm_kzalloc(dev->mt76.dev, sizeof(*hwq), GFP_KERNEL); - if (!hwq) - return -ENOMEM; - - err = mt76_queue_alloc(dev, hwq, idx, n_desc, 0, MT_TX_RING_BASE); + err = mt76_init_tx_queue(&dev->mt76, qid, idx, n_desc, + MT_TX_RING_BASE); if (err < 0) return err;
- dev->mt76.q_tx[qid] = hwq; - mt7603_irq_enable(dev, MT_INT_TX_DONE(idx));
return 0; diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/dma.c b/drivers/net/wireless/mediatek/mt76/mt7615/dma.c index bf8ae14121db..333254734ac5 100644 --- a/drivers/net/wireless/mediatek/mt76/mt7615/dma.c +++ b/drivers/net/wireless/mediatek/mt76/mt7615/dma.c @@ -11,25 +11,6 @@ #include "../dma.h" #include "mac.h"
-static int -mt7615_init_tx_queue(struct mt7615_dev *dev, int qid, int idx, int n_desc) -{ - struct mt76_queue *hwq; - int err; - - hwq = devm_kzalloc(dev->mt76.dev, sizeof(*hwq), GFP_KERNEL); - if (!hwq) - return -ENOMEM; - - err = mt76_queue_alloc(dev, hwq, idx, n_desc, 0, MT_TX_RING_BASE); - if (err < 0) - return err; - - dev->mt76.q_tx[qid] = hwq; - - return 0; -} - static int mt7622_init_tx_queues_multi(struct mt7615_dev *dev) { @@ -43,20 +24,22 @@ mt7622_init_tx_queues_multi(struct mt7615_dev *dev) int i;
for (i = 0; i < ARRAY_SIZE(wmm_queue_map); i++) { - ret = mt7615_init_tx_queue(dev, i, wmm_queue_map[i], - MT7615_TX_RING_SIZE / 2); + ret = mt76_init_tx_queue(&dev->mt76, i, wmm_queue_map[i], + MT7615_TX_RING_SIZE / 2, + MT_TX_RING_BASE); if (ret) return ret; }
- ret = mt7615_init_tx_queue(dev, MT_TXQ_PSD, - MT7622_TXQ_MGMT, MT7615_TX_MGMT_RING_SIZE); + ret = mt76_init_tx_queue(&dev->mt76, MT_TXQ_PSD, MT7622_TXQ_MGMT, + MT7615_TX_MGMT_RING_SIZE, + MT_TX_RING_BASE); if (ret) return ret;
- ret = mt7615_init_tx_queue(dev, MT_TXQ_MCU, - MT7622_TXQ_MCU, MT7615_TX_MCU_RING_SIZE); - return ret; + return mt76_init_tx_queue(&dev->mt76, MT_TXQ_MCU, MT7622_TXQ_MCU, + MT7615_TX_MCU_RING_SIZE, + MT_TX_RING_BASE); }
static int @@ -64,25 +47,26 @@ mt7615_init_tx_queues(struct mt7615_dev *dev) { int ret, i;
- ret = mt7615_init_tx_queue(dev, MT_TXQ_FWDL, - MT7615_TXQ_FWDL, - MT7615_TX_FWDL_RING_SIZE); + ret = mt76_init_tx_queue(&dev->mt76, MT_TXQ_FWDL, MT7615_TXQ_FWDL, + MT7615_TX_FWDL_RING_SIZE, + MT_TX_RING_BASE); if (ret) return ret;
if (!is_mt7615(&dev->mt76)) return mt7622_init_tx_queues_multi(dev);
- ret = mt7615_init_tx_queue(dev, 0, 0, MT7615_TX_RING_SIZE); + ret = mt76_init_tx_queue(&dev->mt76, 0, 0, MT7615_TX_RING_SIZE, + MT_TX_RING_BASE); if (ret) return ret;
for (i = 1; i < MT_TXQ_MCU; i++) dev->mt76.q_tx[i] = dev->mt76.q_tx[0];
- ret = mt7615_init_tx_queue(dev, MT_TXQ_MCU, MT7615_TXQ_MCU, - MT7615_TX_MCU_RING_SIZE); - return 0; + return mt76_init_tx_queue(&dev->mt76, MT_TXQ_MCU, MT7615_TXQ_MCU, + MT7615_TX_MCU_RING_SIZE, + MT_TX_RING_BASE); }
static int mt7615_poll_tx(struct napi_struct *napi, int budget) diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c index 67911c021633..82f65fa1a39d 100644 --- a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c @@ -106,19 +106,13 @@ EXPORT_SYMBOL_GPL(mt76x02e_init_beacon_config); static int mt76x02_init_tx_queue(struct mt76x02_dev *dev, int qid, int idx, int n_desc) { - struct mt76_queue *hwq; int err;
- hwq = devm_kzalloc(dev->mt76.dev, sizeof(*hwq), GFP_KERNEL); - if (!hwq) - return -ENOMEM; - - err = mt76_queue_alloc(dev, hwq, idx, n_desc, 0, MT_TX_RING_BASE); + err = mt76_init_tx_queue(&dev->mt76, qid, idx, n_desc, + MT_TX_RING_BASE); if (err < 0) return err;
- dev->mt76.q_tx[qid] = hwq; - mt76x02_irq_enable(dev, MT_INT_TX_DONE(idx));
return 0; diff --git a/drivers/net/wireless/mediatek/mt76/mt7915/dma.c b/drivers/net/wireless/mediatek/mt76/mt7915/dma.c index 33c42ecef2a4..7c9fe142ed41 100644 --- a/drivers/net/wireless/mediatek/mt76/mt7915/dma.c +++ b/drivers/net/wireless/mediatek/mt76/mt7915/dma.c @@ -6,41 +6,16 @@ #include "mac.h"
static int -mt7915_init_tx_queues(struct mt7915_dev *dev, int n_desc) +mt7915_init_tx_queues(struct mt7915_dev *dev, int idx, int n_desc) { - struct mt76_queue *hwq; - int err, i; + int i, err;
- hwq = devm_kzalloc(dev->mt76.dev, sizeof(*hwq), GFP_KERNEL); - if (!hwq) - return -ENOMEM; - - err = mt76_queue_alloc(dev, hwq, MT7915_TXQ_BAND0, n_desc, 0, - MT_TX_RING_BASE); + err = mt76_init_tx_queue(&dev->mt76, 0, idx, n_desc, MT_TX_RING_BASE); if (err < 0) return err;
for (i = 0; i < MT_TXQ_MCU; i++) - dev->mt76.q_tx[i] = hwq; - - return 0; -} - -static int -mt7915_init_mcu_queue(struct mt7915_dev *dev, int qid, int idx, int n_desc) -{ - struct mt76_queue *hwq; - int err; - - hwq = devm_kzalloc(dev->mt76.dev, sizeof(*hwq), GFP_KERNEL); - if (!hwq) - return -ENOMEM; - - err = mt76_queue_alloc(dev, hwq, idx, n_desc, 0, MT_TX_RING_BASE); - if (err < 0) - return err; - - dev->mt76.q_tx[qid] = hwq; + dev->mt76.q_tx[i] = dev->mt76.q_tx[0];
return 0; } @@ -262,25 +237,26 @@ int mt7915_dma_init(struct mt7915_dev *dev) mt76_wr(dev, MT_WFDMA1_PRI_DLY_INT_CFG0, 0);
/* init tx queue */ - ret = mt7915_init_tx_queues(dev, MT7915_TX_RING_SIZE); + ret = mt7915_init_tx_queues(dev, MT7915_TXQ_BAND0, + MT7915_TX_RING_SIZE); if (ret) return ret;
/* command to WM */ - ret = mt7915_init_mcu_queue(dev, MT_TXQ_MCU, MT7915_TXQ_MCU_WM, - MT7915_TX_MCU_RING_SIZE); + ret = mt76_init_tx_queue(&dev->mt76, MT_TXQ_MCU, MT7915_TXQ_MCU_WM, + MT7915_TX_MCU_RING_SIZE, MT_TX_RING_BASE); if (ret) return ret;
/* command to WA */ - ret = mt7915_init_mcu_queue(dev, MT_TXQ_MCU_WA, MT7915_TXQ_MCU_WA, - MT7915_TX_MCU_RING_SIZE); + ret = mt76_init_tx_queue(&dev->mt76, MT_TXQ_MCU_WA, MT7915_TXQ_MCU_WA, + MT7915_TX_MCU_RING_SIZE, MT_TX_RING_BASE); if (ret) return ret;
/* firmware download */ - ret = mt7915_init_mcu_queue(dev, MT_TXQ_FWDL, MT7915_TXQ_FWDL, - MT7915_TX_FWDL_RING_SIZE); + ret = mt76_init_tx_queue(&dev->mt76, MT_TXQ_FWDL, MT7915_TXQ_FWDL, + MT7915_TX_FWDL_RING_SIZE, MT_TX_RING_BASE); if (ret) return ret;
My apologies, I should've have explained my reasoning for the patch better (and sooner).
1. My issue with 5.10 version of mt7615_init_tx_queues() in drivers/net/wireless/mediatek/mt76/mt7615/dma.c is that return value of final call to mt7615_init_tx_queue() is not taken into account when returning result of mt7615_init_tx_queues(). So, if last mt7615_init_tx_queue() fails (due to memory issues, for instance), parent function will still erroneously return 0.
2. To correct the issue, I turned to Lorenzo's patch in b671da33d1c5973f90f098ff66a91953691df582 which solves my petit problem as well as rewrites a single mt76_init_tx_queue() function to be used across all mt76 drivers.
3. I was torn between writing my own little patch to fix a single mistake or use an existing one that increases code readability and uniformity. I settled on latter.
4. As for this patch exclusivity to 5.10.y branch, I have an incentive to prioritize 5.10 of all others. Wasn't sure I should be the one to suggest the patch for other branches but it does make sense.
Keep having issues with quoting emails properly, hope this one worked fine.
Thanks,
Nikita
On Sun, Jan 22, 2023 at 07:59:10AM -0800, Nikita Zhandarovich wrote:
My apologies, I should've have explained my reasoning for the patch better (and sooner).
I'm sorry, but I have no context here at all. Always properly quote the email you are referring to. Remember, some of us get 1000+ emails a day and can't remember anything we wrote in response an hour later, let alone days or weeks.
confused,
greg k-h
On Thu, Jan 12, 2023 at 03:58:49AM -0800, Nikita Zhandarovich wrote:
Svace has identified unchecked return value of mt7615_init_tx_queue function in 5.10 branch, even though it makes sense to track it instead. This issue is fixed in upstream version by Lorenzo's patch.
The same patch can be cleanly applied to the 5.10 branch.
I do not understand, what issue/bug does this fix? And how can you trigger it? And why only worry about the 5.10.y kernel branch?
thanks,
greg k-h
My apologies, I should've have explained my reasoning better.
1. My issue with 5.10 version of mt7615_init_tx_queues() in drivers/net/wireless/mediatek/mt76/mt7615/dma.c is that return value of final call to mt7615_init_tx_queue() is not taken into account when returning result of mt7615_init_tx_queues(). So, if last mt7615_init_tx_queue() fails (due to memory issues, for instance), parent function will still erroneously return 0.
2. To correct the issue, I turned to Lorenzo's patch in b671da33d1c5973f90f098ff66a91953691df582 which solves my petit problem as well as rewrites a single mt76_init_tx_queue() function to be used across all mt76 drivers.
3. I was torn between writing my own little patch to fix a single mistake or use an existing one that increases code readability and uniformity. I settled on latter.
4. As for this patch exclusivity to 5.10.y branch, I have an incentive to prioritise prioritize 5.10. Wasn't sure I should be the one to suggest the patch for other branches.
Thanks,
Nikita
On Fri, Jan 13, 2023 at 07:04:45AM -0800, Nikita Zhandarovich wrote:
My apologies, I should've have explained my reasoning better.
Reasoning for what?
Sorry, I have no context here, please properly quote emails so that we have a hint as to what is going on. Remember, some of us get 1000+ a day that we need to process somehow...
thanks,
greg k-h
linux-stable-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Nikita Zhandarovich