[PATCH RESEND] wifi: iwlwifi: mvm: fix an error code in iwl_mvm_alloc_sta_after_restart()

[PATCH RESEND] wifi: iwlwifi: mvm: fix an error code in iwl_mvm_alloc_sta_after_restart()

[Ma Ke]

This error path should return -EINVAL instead of success.

Fixes: 57974a55d995 ("wifi: iwlwifi: mvm: refactor iwl_mvm_mac_sta_state_common()")
Signed-off-by: Ma Ke <make24@iscas.ac.cn>
---
 drivers/net/wireless/intel/iwlwifi/mvm/mld-sta.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mld-sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/mld-sta.c
index d5a204e52076..477239a1ed83 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mld-sta.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mld-sta.c
@@ -644,12 +644,12 @@ static int iwl_mvm_alloc_sta_after_restart(struct iwl_mvm *mvm,
 						  lockdep_is_held(&mvm->mutex));
 
 		if (!link_conf)
-			continue;
+			return -EINVAL;
 
 		mvm_link = mvmvif->link[link_conf->link_id];
 
 		if (!mvm_link || !mvm_link_sta)
-			continue;
+			return -EINVAL;
 
 		sta_id = mvm_link_sta->sta_id;
 		ret = iwl_mvm_mld_cfg_sta(mvm, sta, vif, link_sta,
-- 
2.25.1

Re: [PATCH RESEND] wifi: iwlwifi: mvm: fix an error code in iwl_mvm_alloc_sta_after_restart()

[Dan Carpenter]

The Subject says RESEND but doesn't explain why you are resending.
You probably meant v2, but again it needs an explanation.

On Fri, Aug 02, 2024 at 12:27:40PM +0800, Ma Ke wrote:
> This error path should return -EINVAL instead of success.

Why do you feel that way?  Have you tested it?  What is the user visible
effect of this bug?

I slightly feel hypocritical because I have send lots of commit messages
with exactly this commit message.  The difference is that I only send
really easy patches where it's obvious what the intent was.  A normal
kernel developer wouldn't need to leave their email client or view any
outside information to see that my patch is correct.  If a patch is not
dead easy, I normally just report it.  (Sometimes I report dead easy
bugs as well because I am lazy and maybe it's the end of my work day
or whatever).

This patch on the other hand is more subtle and it's not clear why the
continue statements changed into returns.

regards,
dan carpenter

Re: [PATCH RESEND] wifi: iwlwifi: mvm: fix an error code in iwl_mvm_alloc_sta_after_restart()

[Ma Ke]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1764 bytes --]

Dan Carpenter<dan.carpenter@linaro.org> wrote:
> The Subject says RESEND but doesn't explain why you are resending.
> You probably meant v2, but again it needs an explanation.
> 
> On Fri, Aug 02, 2024 at 12:27:40PM +0800, Ma Ke wrote:
> > This error path should return -EINVAL instead of success.
> 
> Why do you feel that way?  Have you tested it?  What is the user visible
> effect of this bug?
> 
> I slightly feel hypocritical because I have send lots of commit messages
> with exactly this commit message.  The difference is that I only send
> really easy patches where it's obvious what the intent was.  A normal
> kernel developer wouldn't need to leave their email client or view any
> outside information to see that my patch is correct.  If a patch is not
> dead easy, I normally just report it.  (Sometimes I report dead easy
> bugs as well because I am lazy and maybe it's the end of my work day
> or whatever).
> 
> This patch on the other hand is more subtle and it's not clear why the
> continue statements changed into returns.
> 
> regards,
> dan carpenter
Thank you for your response to the vulnerability I submitted. Yes, we 
believe there is a similar issue. As described in [1], it gets pointers 
which are handled under the protection mechanism. If the path is error, it 
should return -EINVAL directly instead of success. The discovery of this 
problem was confirmed through manual review of the code and compilation 
testing. And by the way, I resent the patch because I hadn’t received a 
reply for a long time, so I resent it.

[1] https://lore.kernel.org/all/MW5PR11MB58102E1897D7437CD8E1DF27A3DDA@MW5PR11MB5810.namprd11.prod.outlook.com/

--
Regards,

Ma Ke

Re: [PATCH RESEND] wifi: iwlwifi: mvm: fix an error code in iwl_mvm_alloc_sta_after_restart()

[Kalle Valo]

Ma Ke <make24@iscas.ac.cn> writes:

> Dan Carpenter<dan.carpenter@linaro.org> wrote:.
>> The Subject says RESEND but doesn't explain why you are resending..
>> You probably meant v2, but again it needs an explanation..
>> .
>> On Fri, Aug 02, 2024 at 12:27:40PM +0800, Ma Ke wrote:.
>> > This error path should return -EINVAL instead of success..
>> .
>> Why do you feel that way?  Have you tested it?  What is the user visible.
>> effect of this bug?.
>> .
>> I slightly feel hypocritical because I have send lots of commit messages.
>> with exactly this commit message.  The difference is that I only send.
>> really easy patches where it's obvious what the intent was.  A normal.
>> kernel developer wouldn't need to leave their email client or view any.
>> outside information to see that my patch is correct.  If a patch is not.
>> dead easy, I normally just report it.  (Sometimes I report dead easy.
>> bugs as well because I am lazy and maybe it's the end of my work day.
>> or whatever)..
>> .
>> This patch on the other hand is more subtle and it's not clear why the.
>> continue statements changed into returns..
>> .
>> regards,.
>> dan carpenter.
> Thank you for your response to the vulnerability I submitted. Yes, we .
> believe there is a similar issue. As described in [1], it gets pointers .
> which are handled under the protection mechanism. If the path is error, it .
> should return -EINVAL directly instead of success.

The commit message should explain _why_ it should return an error.
Currently there's no explanation neither in the commit message or in
your email.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
https://docs.kernel.org/process/submitting-patches.html

Re: [PATCH RESEND] wifi: iwlwifi: mvm: fix an error code in iwl_mvm_alloc_sta_after_restart()

[Dan Carpenter]

On Fri, Aug 23, 2024 at 11:04:23AM +0800, Ma Ke wrote:
> Dan Carpenter<dan.carpenter@linaro.org> wrote:
> > The Subject says RESEND but doesn't explain why you are resending.
> > You probably meant v2, but again it needs an explanation.
> > 
> > On Fri, Aug 02, 2024 at 12:27:40PM +0800, Ma Ke wrote:
> > > This error path should return -EINVAL instead of success.
> > 
> > Why do you feel that way?  Have you tested it?  What is the user visible
> > effect of this bug?
> > 
> > I slightly feel hypocritical because I have send lots of commit messages
> > with exactly this commit message.  The difference is that I only send
> > really easy patches where it's obvious what the intent was.  A normal
> > kernel developer wouldn't need to leave their email client or view any
> > outside information to see that my patch is correct.  If a patch is not
> > dead easy, I normally just report it.  (Sometimes I report dead easy
> > bugs as well because I am lazy and maybe it's the end of my work day
> > or whatever).
> > 
> > This patch on the other hand is more subtle and it's not clear why the
> > continue statements changed into returns.
> > 
> > regards,
> > dan carpenter
> Thank you for your response to the vulnerability I submitted. Yes, we 
> believe there is a similar issue. As described in [1], it gets pointers 
> which are handled under the protection mechanism. If the path is error, it 
> should return -EINVAL directly instead of success. The discovery of this 
> problem was confirmed through manual review of the code and compilation 
> testing. And by the way, I resent the patch because I hadn’t received a 
> reply for a long time, so I resent it.
> 
> [1] https://lore.kernel.org/all/MW5PR11MB58102E1897D7437CD8E1DF27A3DDA@MW5PR11MB5810.namprd11.prod.outlook.com/
> 

Oh, huh.  If I understand it correctly, you're copying the logic from my patch.

71b5e40651d8 ("wifi: iwlwifi: mvm: fix an error code in iwl_mvm_mld_add_sta()")

That was a different situation:
1) The code was already doing a return instead of a continue, it's just that
   the error code wasn't set.
2) I mentioned in my email that I wasn't sure of the logic, but just copy and
   pasting from similar code.
3) Plus my code is on a WARN_ON() path so it's almost certainly dead code.  That
   means my patch was very safe.

Meanwhile Gregory's code looks deliberate.  If it's causing an issue at runtime
definitely we need to fix that.  Or if we can find a bug in it then sure.  But
don't assume my code is better than his, because it's likely not the case.

regards,
dan carpenter

Re: [PATCH RESEND] wifi: iwlwifi: mvm: fix an error code in iwl_mvm_alloc_sta_after_restart()

[Dan Carpenter]

It would have been nice to know what Gregory's thoughts were about this code but
he left Intel in Jan.

regards,
dan carpenter

Re: [PATCH RESEND] wifi: iwlwifi: mvm: fix an error code in iwl_mvm_alloc_sta_after_restart()

[Berg, Benjamin]

On Fri, 2024-08-23 at 12:03 +0300, Dan Carpenter wrote:
> On Fri, Aug 23, 2024 at 11:04:23AM +0800, Ma Ke wrote:
> > Dan Carpenter<dan.carpenter@linaro.org> wrote:
> > > The Subject says RESEND but doesn't explain why you are resending.
> > > You probably meant v2, but again it needs an explanation.
> > > 
> > > On Fri, Aug 02, 2024 at 12:27:40PM +0800, Ma Ke wrote:
> > > > This error path should return -EINVAL instead of success.
> > > 
> > > Why do you feel that way?  Have you tested it?  What is the user visible
> > > effect of this bug?
> > > 
> > > I slightly feel hypocritical because I have send lots of commit messages
> > > with exactly this commit message.  The difference is that I only send
> > > really easy patches where it's obvious what the intent was.  A normal
> > > kernel developer wouldn't need to leave their email client or view any
> > > outside information to see that my patch is correct.  If a patch is not
> > > dead easy, I normally just report it.  (Sometimes I report dead easy
> > > bugs as well because I am lazy and maybe it's the end of my work day
> > > or whatever).
> > > 
> > > This patch on the other hand is more subtle and it's not clear why the
> > > continue statements changed into returns.
> > > 
> > > regards,
> > > dan carpenter
> > Thank you for your response to the vulnerability I submitted. Yes, we 
> > believe there is a similar issue. As described in [1], it gets pointers 
> > which are handled under the protection mechanism. If the path is error, it 
> > should return -EINVAL directly instead of success. The discovery of this 
> > problem was confirmed through manual review of the code and compilation 
> > testing. And by the way, I resent the patch because I hadn’t received a 
> > reply for a long time, so I resent it.
> > 
> > [1] https://lore.kernel.org/all/MW5PR11MB58102E1897D7437CD8E1DF27A3DDA@MW5PR11MB5810.namprd11.prod.outlook.com/
> > 
> 
> Oh, huh.  If I understand it correctly, you're copying the logic from my patch.
> 
> 71b5e40651d8 ("wifi: iwlwifi: mvm: fix an error code in iwl_mvm_mld_add_sta()")
> 
> That was a different situation:
> 1) The code was already doing a return instead of a continue, it's just that
>    the error code wasn't set.
> 2) I mentioned in my email that I wasn't sure of the logic, but just copy and
>    pasting from similar code.
> 3) Plus my code is on a WARN_ON() path so it's almost certainly dead code.  That
>    means my patch was very safe.
> 
> Meanwhile Gregory's code looks deliberate.  If it's causing an issue at runtime
> definitely we need to fix that.  Or if we can find a bug in it then sure.  But
> don't assume my code is better than his, because it's likely not the case.

I think the code is correct as-is. Especially as this function is in
the recovery flow after an error, and, if I remember correctly, the
driver state may be somewhat inconsistent with mac80211 here.

An argument about the (in)correctness of this code should consider both
how mac80211 handles restarts and what state the driver might be in
after a firmware assert at an inopportune moment.

Benjamin
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928