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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
>
> 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
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
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
>
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
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
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
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
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
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