| Message ID | 1717771212-30723-3-git-send-email-quic_zijuhu@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | net: rfkill: Fix 2 bugs within rfkill_set_hw_state_reason() | expand |
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 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 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
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;
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(-)