| Message ID | 20250320201914.48159-1-qasdev00@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: mt76: mt7996: prevent uninit return in mt7996_mac_sta_add_links | expand |
… > +++ b/drivers/net/wireless/mediatek/mt76/mt7996/main.c > @@ -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; > + } … I suggest to avoid such repeated error code assignments. Can an additional label be applied instead for this purpose? Regards, Markus
On Sat, 2025-03-22 at 11:01 +0100, Markus Elfring wrote: > > 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
> If link_conf_dereference_protected() or mt7996_vif_link() > or link_sta_dereference_protected() fail the code jumps to call failed? > 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
On Sat, Mar 22, 2025 at 12:51:57PM +0100, Jonas Gorski wrote: > Hi, > > On Thu, Mar 20, 2025 at 9:19 PM Qasim Ijaz <qasdev00@gmail.com> wrote: > > > > 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 <qasdev00@gmail.com> > > --- > > drivers/net/wireless/mediatek/mt76/mt7996/main.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/main.c b/drivers/net/wireless/mediatek/mt76/mt7996/main.c > > index 91c64e3a0860..78f7f1fc867e 100644 > > --- a/drivers/net/wireless/mediatek/mt76/mt7996/main.c > > +++ b/drivers/net/wireless/mediatek/mt76/mt7996/main.c > > @@ -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 > > 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. > 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 > Best regards, > Jonas
diff --git a/drivers/net/wireless/mediatek/mt76/mt7996/main.c b/drivers/net/wireless/mediatek/mt76/mt7996/main.c index 91c64e3a0860..78f7f1fc867e 100644 --- a/drivers/net/wireless/mediatek/mt76/mt7996/main.c +++ b/drivers/net/wireless/mediatek/mt76/mt7996/main.c @@ -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);
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 <qasdev00@gmail.com> --- drivers/net/wireless/mediatek/mt76/mt7996/main.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)