| Message ID | 20250320201914.48159-1-qasdev00@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: mt76: mt7996: prevent uninit return in mt7996_mac_sta_add_links | expand |
On Thu, Mar 20, 2025 at 08:19:14PM +0000, Qasim Ijaz 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; What about if the list is empty or we hit this continue on every link? regards, dan carpenter > > 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); > -- > 2.39.5 >
> On Thu, Mar 20, 2025 at 08:19:14PM +0000, Qasim Ijaz 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; > > 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 > > regards, > dan carpenter > > > > > 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); > > -- > > 2.39.5 > >
On Fri, Mar 21, 2025 at 06:14:49PM +0100, Lorenzo Bianconi wrote: > > On Thu, Mar 20, 2025 at 08:19:14PM +0000, Qasim Ijaz 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; > > > > 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? > Fine by me? regards, dan carpenter
… > +++ 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, Mar 22, 2025 at 11:01:18AM +0100, Markus Elfring wrote: > … > > +++ 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? 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
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
>> 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
> 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
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. 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
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(-)