wifi: mt76: mt7996: prevent uninit return in mt7996_mac_sta_add_links

PATCH Superseded Qasim Ijaz 6 participants 11 messages Mar 20, 2025 – Mar 22, 2025

Subsystems (2)

Arm MediaTek MediaTek MT76 wireless LAN driver

lore

Share & tools

lore

Qasim Ijaz

author

2025-03-20 20:19 UTC #0 lore raw

PATCH 1/1 wifi: mt76: mt7996: prevent uninit return in mt7996_mac_sta_add_links

If link_conf_dereference_protected() or mt7996_vif_link() or link_sta_dereference_protected() fail the code jumps to the error_unlink label and returns ret which is uninitialised.

Fix this by setting err before jumping to error_unlink.

Fixes c7e4fc362443 ("wifi: mt76: mt7996: Update mt7996_mcu_add_sta to MLO support")

Fixes dd82a9e02c05 ("wifi: mt76: mt7996: Rely on mt7996_sta_link in sta_add/sta_remove callbacks")

Signed-off-by Qasim Ijaz <[email protected]>

---
 drivers/net/wireless/mediatek/mt76/mt7996/main.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

drivers/net/wireless/mediatek/mt76/mt7996/main.c +9 -3

index 91c64e3a0860..78f7f1fc867e 100644
@@ -998,16 +998,22 @@ mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif,
 			continue;
 
 		link_conf = link_conf_dereference_protected(vif, link_id);
-		if (!link_conf)
+		if (!link_conf) {
+			err = -EINVAL;
 			goto error_unlink;
+		}
 
 		link = mt7996_vif_link(dev, vif, link_id);
-		if (!link)
+		if (!link) {
+			err = -EINVAL;
 			goto error_unlink;
+		}
 
 		link_sta = link_sta_dereference_protected(sta, link_id);
-		if (!link_sta)
+		if (!link_sta) {
+			err = -EINVAL
 			goto error_unlink;
+		}
 
 		err = mt7996_mac_sta_init_link(dev, link_conf, link_sta, link,
 					       link_id);

Signature · 1 line

2.39.5

Dan Carpenter

2025-03-21 16:36 UTC #1 lore raw

Quoted 18 lines · Qasim Ijaz

If link_conf_dereference_protected() or mt7996_vif_link() or link_sta_dereference_protected() fail the code jumps to the error_unlink label and returns ret which is uninitialised.
Fix this by setting err before jumping to error_unlink.
Fixes c7e4fc362443 ("wifi: mt76: mt7996: Update mt7996_mcu_add_sta to MLO support")
Fixes dd82a9e02c05 ("wifi: mt76: mt7996: Rely on mt7996_sta_link in sta_add/sta_remove callbacks")
Signed-off-by Qasim Ijaz <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/mt7996/main.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/net/wireless/mediatek/mt76/mt7996/main.c +0 -0
index 91c64e3a0860..78f7f1fc867e 100644

drivers/net/wireless/mediatek/mt76/mt7996/main.c mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif, · line 998

Quoted 1 line

drivers/net/wireless/mediatek/mt76/mt7996/main.c +0 -0

@@ -998,16 +998,22 @@ mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif,
 			continue;

What about if the list is empty or we hit this continue on every link?

regards,
dan carpenter

Quoted 27 lines

drivers/net/wireless/mediatek/mt76/mt7996/main.c +9 -3

 		link_conf = link_conf_dereference_protected(vif, link_id);
-		if (!link_conf)
+		if (!link_conf) {
+			err = -EINVAL;
 			goto error_unlink;
+		}
 
 		link = mt7996_vif_link(dev, vif, link_id);
-		if (!link)
+		if (!link) {
+			err = -EINVAL;
 			goto error_unlink;
+		}
 
 		link_sta = link_sta_dereference_protected(sta, link_id);
-		if (!link_sta)
+		if (!link_sta) {
+			err = -EINVAL
 			goto error_unlink;
+		}
 
 		err = mt7996_mac_sta_init_link(dev, link_conf, link_sta, link,
 					       link_id);

Signature · 1 line

2.39.5

Lorenzo Bianconi

maintainer

2025-03-21 17:14 UTC #2 lore raw

Quoted 14 lines · Qasim Ijaz

Quoted 13 lines
If link_conf_dereference_protected() or mt7996_vif_link() or link_sta_dereference_protected() fail the code jumps to the error_unlink label and returns ret which is uninitialised.
Fix this by setting err before jumping to error_unlink.
Fixes c7e4fc362443 ("wifi: mt76: mt7996: Update mt7996_mcu_add_sta to MLO support")
Fixes dd82a9e02c05 ("wifi: mt76: mt7996: Rely on mt7996_sta_link in sta_add/sta_remove callbacks")
Signed-off-by Qasim Ijaz <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/mt7996/main.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Quoted 8 lines

drivers/net/wireless/mediatek/mt76/mt7996/main.c mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif, · line 998
Quoted 6 lines
drivers/net/wireless/mediatek/mt76/mt7996/main.c +0 -0
index 91c64e3a0860..78f7f1fc867e 100644
@@ -998,16 +998,22 @@ mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif,
 			continue;
What about if the list is empty or we hit this continue on every link?

we will return 0 and I guess that's fine, agree?

Regards,
Lorenzo

Quoted 31 lines

regards,
dan carpenter

Quoted 27 lines

drivers/net/wireless/mediatek/mt76/mt7996/main.c +9 -3

 		link_conf = link_conf_dereference_protected(vif, link_id);
-		if (!link_conf)
+		if (!link_conf) {
+			err = -EINVAL;
 			goto error_unlink;
+		}
 
 		link = mt7996_vif_link(dev, vif, link_id);
-		if (!link)
+		if (!link) {
+			err = -EINVAL;
 			goto error_unlink;
+		}
 
 		link_sta = link_sta_dereference_protected(sta, link_id);
-		if (!link_sta)
+		if (!link_sta) {
+			err = -EINVAL
 			goto error_unlink;
+		}
 
 		err = mt7996_mac_sta_init_link(dev, link_conf, link_sta, link,
 					       link_id);

Signature · 1 line

2.39.5

Dan Carpenter

2025-03-21 17:35 UTC #3 lore raw

Quoted 23 lines · Lorenzo Bianconi

Quoted 14 lines · Qasim Ijaz
Quoted 13 lines
If link_conf_dereference_protected() or mt7996_vif_link() or link_sta_dereference_protected() fail the code jumps to the error_unlink label and returns ret which is uninitialised.
Fix this by setting err before jumping to error_unlink.
Fixes c7e4fc362443 ("wifi: mt76: mt7996: Update mt7996_mcu_add_sta to MLO support")
Fixes dd82a9e02c05 ("wifi: mt76: mt7996: Rely on mt7996_sta_link in sta_add/sta_remove callbacks")
Signed-off-by Qasim Ijaz <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/mt7996/main.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
Quoted 8 lines
drivers/net/wireless/mediatek/mt76/mt7996/main.c mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif, · line 998
Quoted 6 lines
drivers/net/wireless/mediatek/mt76/mt7996/main.c +0 -0
index 91c64e3a0860..78f7f1fc867e 100644
@@ -998,16 +998,22 @@ mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif,
 			continue;
What about if the list is empty or we hit this continue on every link?

Quoted 2 lines

we will return 0 and I guess that's fine, agree?

Fine by me?

regards,
dan carpenter

Markus Elfring

2025-03-22 10:01 UTC #4 lore raw

In reply to Qasim Ijaz

…

Quoted 7 lines

+++ b/drivers/net/wireless/mediatek/mt76/mt7996/main.c

drivers/net/wireless/mediatek/mt76/mt7996/main.c +1 -1

@@ -998,16 +998,22 @@ mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif,
 			continue;
 
 		link_conf = link_conf_dereference_protected(vif, link_id);
-		if (!link_conf)
+		if (!link_conf) {

drivers/net/wireless/mediatek/mt76/mt7996/main.c mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif, · line 1010

Quoted 10 lines

drivers/net/wireless/mediatek/mt76/mt7996/main.c +5 -1

+			err = -EINVAL;
 			goto error_unlink;
+		}
 
 		link = mt7996_vif_link(dev, vif, link_id);
-		if (!link)
+		if (!link) {
+			err = -EINVAL;
 			goto error_unlink;
+		}

…

I suggest to avoid such repeated error code assignments. Can an additional label be applied instead for this purpose?

Regards,
Markus

Dan Carpenter

2025-03-22 10:19 UTC #5 lore raw

Quoted 20 lines · Markus Elfring

…

Quoted 17 lines

+++ b/drivers/net/wireless/mediatek/mt76/mt7996/main.c

drivers/net/wireless/mediatek/mt76/mt7996/main.c +6 -2

@@ -998,16 +998,22 @@ mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif,
 			continue;
 
 		link_conf = link_conf_dereference_protected(vif, link_id);
-		if (!link_conf)
+		if (!link_conf) {
+			err = -EINVAL;
 			goto error_unlink;
+		}
 
 		link = mt7996_vif_link(dev, vif, link_id);
-		if (!link)
+		if (!link) {
+			err = -EINVAL;
 			goto error_unlink;
+		}

…

Quoted 2 lines

I suggest to avoid such repeated error code assignments. Can an additional label be applied instead for this purpose?

Maintainers get the final vote but generally when someone is doing the work they get to decide.

I would say that generally the way that Qasim did it is normally more readable and more future proof.

regards
dan carpenter

Johannes Berg

2025-03-22 10:20 UTC #6 lore raw

In reply to Markus Elfring

Quoted 4 lines · Markus Elfring

I suggest to avoid such repeated error code assignments. Can an additional label be applied instead for this purpose?

Please stop your "suggestions" on this list. None have really been helpful.

johannes

Markus Elfring

2025-03-22 10:43 UTC #7 lore raw

Quoted 5 lines

Quoted 2 lines
I suggest to avoid such repeated error code assignments. Can an additional label be applied instead for this purpose?
Please stop your "suggestions" on this list. None have really been helpful.

It seems that you care less then for the avoidance of duplicate source code also for affected error/exception handling.

Regards,
Markus

Markus Elfring

2025-03-22 11:45 UTC #8 lore raw

In reply to Qasim Ijaz

Quoted 2 lines

If link_conf_dereference_protected() or mt7996_vif_link() or link_sta_dereference_protected() fail the code jumps to

call failed?

Quoted 1 line

the error_unlink label and returns ret which is uninitialised.

a value from a local variable which was not initialised properly?

Can the summary phrase be improved also another bit?

Regards,
Markus

Jonas Gorski

2025-03-22 11:52 UTC #9 lore raw

In reply to Qasim Ijaz

Hi,

Quoted 29 lines · Qasim Ijaz

If link_conf_dereference_protected() or mt7996_vif_link() or link_sta_dereference_protected() fail the code jumps to the error_unlink label and returns ret which is uninitialised.
Fix this by setting err before jumping to error_unlink.
Fixes c7e4fc362443 ("wifi: mt76: mt7996: Update mt7996_mcu_add_sta to MLO support")
Fixes dd82a9e02c05 ("wifi: mt76: mt7996: Rely on mt7996_sta_link in sta_add/sta_remove callbacks")
Signed-off-by Qasim Ijaz <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/mt7996/main.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/net/wireless/mediatek/mt76/mt7996/main.c +3 -1
index 91c64e3a0860..78f7f1fc867e 100644
@@ -998,16 +998,22 @@ mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif,
                        continue;
 
                link_conf = link_conf_dereference_protected(vif, link_id);
-               if (!link_conf)
+               if (!link_conf) {
+                       err = -EINVAL;
                        goto error_unlink;
+               }
 
                link = mt7996_vif_link(dev, vif, link_id);

drivers/net/wireless/mediatek/mt76/mt7996/main.c mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif, · line 1014

Quoted 10 lines

drivers/net/wireless/mediatek/mt76/mt7996/main.c +5 -2

-               if (!link)
+               if (!link) {
+                       err = -EINVAL;
                        goto error_unlink;
+               }
 
                link_sta = link_sta_dereference_protected(sta, link_id);
-               if (!link_sta)
+               if (!link_sta) {
+                       err = -EINVAL

You are missing a semicolon at the end of the line.

To also do some bike shedding, you could initialize err (with 0 or -EINVAL), and then change the err return to return err ? : -EINVAL;. But your way has an explicit error code assigned to each failure, even if it is always the same. So I won't claim my suggestion is better.

Best regards,
Jonas

Qasim Ijaz

author

2025-03-22 22:51 UTC #10 lore raw

Quoted 38 lines · Jonas Gorski

Hi,
Quoted 35 lines · Qasim Ijaz
If link_conf_dereference_protected() or mt7996_vif_link() or link_sta_dereference_protected() fail the code jumps to the error_unlink label and returns ret which is uninitialised.
Fix this by setting err before jumping to error_unlink.
Fixes c7e4fc362443 ("wifi: mt76: mt7996: Update mt7996_mcu_add_sta to MLO support")
Fixes dd82a9e02c05 ("wifi: mt76: mt7996: Rely on mt7996_sta_link in sta_add/sta_remove callbacks")
Signed-off-by Qasim Ijaz <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/mt7996/main.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/net/wireless/mediatek/mt76/mt7996/main.c +6 -2
index 91c64e3a0860..78f7f1fc867e 100644
@@ -998,16 +998,22 @@ mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif,
                        continue;
 
                link_conf = link_conf_dereference_protected(vif, link_id);
-               if (!link_conf)
+               if (!link_conf) {
+                       err = -EINVAL;
                        goto error_unlink;
+               }
 
                link = mt7996_vif_link(dev, vif, link_id);
-               if (!link)
+               if (!link) {
+                       err = -EINVAL;
                        goto error_unlink;
+               }
 

Quoted 7 lines

drivers/net/wireless/mediatek/mt76/mt7996/main.c mt7996_mac_sta_add_links(struct mt7996_dev *dev, struct ieee80211_vif *vif, · line 1014
Quoted 4 lines
drivers/net/wireless/mediatek/mt76/mt7996/main.c +2 -1
                link_sta = link_sta_dereference_protected(sta, link_id);
-               if (!link_sta)
+               if (!link_sta) {
+                       err = -EINVAL
You are missing a semicolon at the end of the line.

Good spot! not sure how that happened but I will resend a v2 patch.

Quoted 5 lines

To also do some bike shedding, you could initialize err (with 0 or -EINVAL), and then change the err return to return err ? : -EINVAL;. But your way has an explicit error code assigned to each failure, even if it is always the same. So I won't claim my suggestion is better.

Thanks for the suggestion, I think this could work however I think my current approach is more a bit more readable, so I will leave it as is.

Thanks,
Qasim

Quoted 2 lines

Best regards,
Jonas