[PATCH v1 0/2] net: rfkill: Fix 2 bugs within rfkill_set_hw_state_reason()

[PATCH v1 0/2] net: rfkill: Fix 2 bugs within rfkill_set_hw_state_reason()

[Zijun Hu]

This patch series are to fix 2 bugs for kernel API within kernel API
rfkill_set_hw_state_reason().

Zijun Hu (2):
  net: rfkill: Fix a wrongly handling error case
  net: rfkill: Fix a logic error within rfkill_set_hw_state_reason()

 net/rfkill/core.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] net: rfkill: Fix a wrongly handling error case

[Zijun Hu]

Kernel API rfkill_set_hw_state_reason() does not return current combined
block state when its parameter @reason is invalid, that is wrong according
to its comments.

Fixed by returning API required value, also use pr_err() instead of WARN()
for this error case handling.

Fixes: 14486c82612a ("rfkill: add a reason to the HW rfkill state")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 net/rfkill/core.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index c3feb4f49d09..0dc982b4fce6 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -543,13 +543,15 @@ bool rfkill_set_hw_state_reason(struct rfkill *rfkill,
 {
 	unsigned long flags;
 	bool ret, prev;
+	const unsigned long reason_mask = RFKILL_HARD_BLOCK_SIGNAL |
+		RFKILL_HARD_BLOCK_NOT_OWNER;
 
 	BUG_ON(!rfkill);
 
-	if (WARN(reason &
-	    ~(RFKILL_HARD_BLOCK_SIGNAL | RFKILL_HARD_BLOCK_NOT_OWNER),
-	    "hw_state reason not supported: 0x%lx", reason))
-		return blocked;
+	if (reason & ~reason_mask) {
+		pr_err("hw_state reason not supported: 0x%lx\n", reason);
+		return rfkill_blocked(rfkill);
+	}
 
 	spin_lock_irqsave(&rfkill->lock, flags);
 	prev = !!(rfkill->hard_block_reasons & reason);
-- 
2.7.4

Re: [PATCH v1 1/2] net: rfkill: Fix a wrongly handling error case

[Johannes Berg]

> 
> use pr_err() instead of WARN()
> for this error case handling.

I don't see anything wrong with the WARN here, it's the user/driver
calling it completely incorrectly.

I also don't really think this is a *fix*, if you used the API
incorrectly you can't necessarily expect a correct return value, I
guess, but anyway it shouldn't happen in the first place.

I'm happy to take the return value change (only) as a cleanup, if you
wish to resend that.

> Fixed by

Please also read
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

johannes

Re: [PATCH v1 1/2] net: rfkill: Fix a wrongly handling error case

[quic_zijuhu]

On 6/12/2024 4:15 PM, Johannes Berg wrote:
>>
>> use pr_err() instead of WARN()
>> for this error case handling.
> 
> I don't see anything wrong with the WARN here, it's the user/driver
> calling it completely incorrectly.
> 
the function is a kernel API and it is handing invalid user input.
below comments for WARN() seems say that pr_err() is better than WARN()
for this case.

include/asm-generic/bug.h:
/*
 * WARN(), WARN_ON(), WARN_ON_ONCE(), and so on can be used to report
 * significant kernel issues that need prompt attention if they should ever
 * appear at runtime.
 *
 * Do not use these macros when checking for invalid external inputs
 * (e.g. invalid system call arguments, or invalid data coming from
 * network/devices), and on transient conditions like ENOMEM or EAGAIN.
 * These macros should be used for recoverable kernel issues only.
 * For invalid external inputs, transient conditions, etc use
 * pr_err[_once/_ratelimited]() followed by dump_stack(), if necessary.
 * Do not include "BUG"/"WARNING" in format strings manually to make these
 * conditions distinguishable from kernel issues.
 *
 * Use the versions with printk format strings to provide better
diagnostics.
 */

> I also don't really think this is a *fix*, if you used the API
> incorrectly you can't necessarily expect a correct return value, I
> guess, but anyway it shouldn't happen in the first place.
> 
okay, will remove term fix and fix tag. the API returns type bool for
block state, the type bool can't cover case for invalid user input.

> I'm happy to take the return value change (only) as a cleanup, if you
> wish to resend that.
> 
i am pleasure to resend it after code review done.
>> Fixed by
> 
> Please also read
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> 
okay, thank you
> johannes

Re: [PATCH v1 1/2] net: rfkill: Fix a wrongly handling error case

[Johannes Berg]

On Wed, 2024-06-12 at 18:12 +0800, quic_zijuhu wrote:
> On 6/12/2024 4:15 PM, Johannes Berg wrote:
> > > 
> > > use pr_err() instead of WARN()
> > > for this error case handling.
> > 
> > I don't see anything wrong with the WARN here, it's the user/driver
> > calling it completely incorrectly.
> > 
> the function is a kernel API

Sure.

> and it is handing invalid user input.

No.

johannes

[PATCH v1 2/2] net: rfkill: Fix a logic error within rfkill_set_hw_state_reason()

[Zijun Hu]

Kernel API rfkill_set_hw_state_reason() wrongly gets previous block state
by using its parameter @reason as reason mask.

Fixed by using @reason_mask as reason mask.

Fixes: 14486c82612a ("rfkill: add a reason to the HW rfkill state")
Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com>
---
 net/rfkill/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 0dc982b4fce6..ee7a751b6c5a 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -554,7 +554,7 @@ bool rfkill_set_hw_state_reason(struct rfkill *rfkill,
 	}
 
 	spin_lock_irqsave(&rfkill->lock, flags);
-	prev = !!(rfkill->hard_block_reasons & reason);
+	prev = !!(rfkill->hard_block_reasons & reason_mask);
 	if (blocked) {
 		rfkill->state |= RFKILL_BLOCK_HW;
 		rfkill->hard_block_reasons |= reason;
-- 
2.7.4

Re: [PATCH v1 2/2] net: rfkill: Fix a logic error within rfkill_set_hw_state_reason()

[Johannes Berg]

On Fri, 2024-06-07 at 22:40 +0800, Zijun Hu wrote:
> Kernel API rfkill_set_hw_state_reason() wrongly gets previous block state
> by using its parameter @reason as reason mask.

Using reason as a mask is perfectly valid.

And checking that the bit changed also seems valid.

We might want to not schedule the worker if it's not needed, but that's
a different issue, I don't see a real bug here?

johannes

Re: [PATCH v1 2/2] net: rfkill: Fix a logic error within rfkill_set_hw_state_reason()

[quic_zijuhu]

On 6/12/2024 4:18 PM, Johannes Berg wrote:
> On Fri, 2024-06-07 at 22:40 +0800, Zijun Hu wrote:
>> Kernel API rfkill_set_hw_state_reason() wrongly gets previous block state
>> by using its parameter @reason as reason mask.
> 
> Using reason as a mask is perfectly valid.
> 
> And checking that the bit changed also seems valid.
> 
i don't think so as explained below.
let us assume @rfkill->hard_block_reasons has value
RFKILL_HARD_BLOCK_SIGNAL which means block state before
__rfkill_set_sw_state(..., true, RFKILL_HARD_BLOCK_NOT_OWNER) is invoked.

@prev should mean previous block state, @prev will have false based on
current logic, it is wrong since rfkill have block state before the call.

> We might want to not schedule the worker if it's not needed, but that's
> a different issue, I don't see a real bug here?
> 
the worker will be unneccessarily scheduled for above example based on
current logic even if the rfkill always stay in block state.
> johannes
> 

Re: [PATCH v1 2/2] net: rfkill: Fix a logic error within rfkill_set_hw_state_reason()

[Johannes Berg]

On Wed, 2024-06-12 at 17:43 +0800, quic_zijuhu wrote:
> On 6/12/2024 4:18 PM, Johannes Berg wrote:
> > On Fri, 2024-06-07 at 22:40 +0800, Zijun Hu wrote:
> > > Kernel API rfkill_set_hw_state_reason() wrongly gets previous block state
> > > by using its parameter @reason as reason mask.
> > 
> > Using reason as a mask is perfectly valid.
> > 
> > And checking that the bit changed also seems valid.
> > 
> i don't think so as explained below.
> let us assume @rfkill->hard_block_reasons has value
> RFKILL_HARD_BLOCK_SIGNAL which means block state before
> __rfkill_set_sw_state(..., true, RFKILL_HARD_BLOCK_NOT_OWNER) is invoked.
> 
> @prev should mean previous block state,
> 

no.

johannes

Re: [PATCH v1 2/2] net: rfkill: Fix a logic error within rfkill_set_hw_state_reason()

[Johannes Berg]

On Wed, 2024-06-12 at 17:43 +0800, quic_zijuhu wrote:
> On 6/12/2024 4:18 PM, Johannes Berg wrote:
> > On Fri, 2024-06-07 at 22:40 +0800, Zijun Hu wrote:
> > > Kernel API rfkill_set_hw_state_reason() wrongly gets previous block state
> > > by using its parameter @reason as reason mask.
> > 
> > Using reason as a mask is perfectly valid.
> > 
> > And checking that the bit changed also seems valid.
> > 
> i don't think so as explained below.
> let us assume @rfkill->hard_block_reasons has value
> RFKILL_HARD_BLOCK_SIGNAL which means block state before
> __rfkill_set_sw_state(..., true, RFKILL_HARD_BLOCK_NOT_OWNER) is invoked.
> 
> @prev should mean previous block state, @prev will have false based on
> current logic, it is wrong since rfkill have block state before the call.
> 
> > We might want to not schedule the worker if it's not needed, but that's
> > a different issue, I don't see a real bug here?
> > 
> the worker will be unneccessarily scheduled for above example based on
> current logic even if the rfkill always stay in block state.
> > 

But yes, this is right. It's just not a bug.

johannes

Re: [PATCH v1 2/2] net: rfkill: Fix a logic error within rfkill_set_hw_state_reason()

[Johannes Berg]

On Fri, 2024-06-07 at 22:40 +0800, Zijun Hu wrote:
> Kernel API rfkill_set_hw_state_reason() wrongly gets previous block state
> by using its parameter @reason as reason mask.
> 
> Fixed by using @reason_mask as reason mask.
> 

Actually, this *introduces* a bug. I'll leave it to you to figure out
what that is, I'm not convinced that you're actually doing *anything*
useful here.

johannes

Re: [PATCH v1 2/2] net: rfkill: Fix a logic error within rfkill_set_hw_state_reason()

[quic_zijuhu]

On 6/12/2024 6:18 PM, Johannes Berg wrote:
> On Fri, 2024-06-07 at 22:40 +0800, Zijun Hu wrote:
>> Kernel API rfkill_set_hw_state_reason() wrongly gets previous block state
>> by using its parameter @reason as reason mask.
>>
>> Fixed by using @reason_mask as reason mask.
>>
> 
> Actually, this *introduces* a bug. I'll leave it to you to figure out
> what that is, I'm not convinced that you're actually doing *anything*
> useful here.
> 
i feels that current logic is weird and it is very difficult to
understand when i read rfkill code.

i think it deserves a comments for current logic if it is right.

current logic was introduced by below code applet of the commit
Commit: 14486c82612a ("rfkill: add a reason to the HW rfkill state")
-       prev = !!(rfkill->state & RFKILL_BLOCK_HW);
-       if (blocked)
+       prev = !!(rfkill->hard_block_reasons & reason);
+       if (blocked) {
                rfkill->state |= RFKILL_BLOCK_HW;

i maybe need to find history to try to understand current logic if it is
right.
> johannes