netdev - Re: [PATCH] can: isotp: fix tx state handling for echo tx processing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f8a22e38-a7e9-0643-d6a6-6c5901dee7b4@hartkopp.net>
Date:   Fri, 4 Nov 2022 15:28:11 +0100
From:   Oliver Hartkopp <socketcan@...tkopp.net>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     netdev@...r.kernel.org, linux-can@...r.kernel.org,
        Wei Chen <harperchen1110@...il.com>, stable@...r.kernel.org
Subject: Re: [PATCH] can: isotp: fix tx state handling for echo tx processing

Hi Marc,

On 04.11.22 13:10, Marc Kleine-Budde wrote:
> On 01.11.2022 22:29:02, Oliver Hartkopp wrote:
>> In commit 4b7fe92c0690 ("can: isotp: add local echo tx processing for
>> consecutive frames") the data flow for consecutive frames (CF) has been
>> reworked to improve the reliability of long data transfers.
>>
>> This rework did not touch the transmission and the tx state changes of
>> single frame (SF) transfers which likely led to the WARN in the
>> isotp_tx_timer_handler() catching a wrong tx state. This patch makes use
>> of the improved frame processing for SF frames and sets the ISOTP_SENDING
>> state in isotp_sendmsg() within the cmpxchg() condition handling.
>>
>> A review of the state machine and the timer handling additionally revealed
>> a missing echo timeout handling in the case of the burst mode in
>> isotp_rcv_echo() and removes a potential timer configuration uncertainty
>> in isotp_rcv_fc() when the receiver requests consecutive frames.
>>
>> Fixes: 4b7fe92c0690 ("can: isotp: add local echo tx processing for consecutive frames")
>> Link: https://lore.kernel.org/linux-can/CAO4mrfe3dG7cMP1V5FLUkw7s+50c9vichigUMQwsxX4M=45QEw@mail.gmail.com/T/#u
>> Reported-by: Wei Chen <harperchen1110@...il.com>
>> Cc: stable@...r.kernel.org # v6.0
>> Signed-off-by: Oliver Hartkopp <socketcan@...tkopp.net>
> 
> [...]
> 
>> @@ -905,10 +915,11 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
>>   		so->tx.state = ISOTP_IDLE;
>>   		wake_up_interruptible(&so->wait);
>>   		break;
>>   
>>   	default:
>> +		pr_notice_once("can-isotp: tx timer state %X\n", so->tx.state);
>>   		WARN_ON_ONCE(1);
> 
> Can you use WARN_ONCE() instead of pr_notice_once() + WARN_ON_ONCE() here?
> 

Yes. That was a good idea! V2 is sent.

It also allowed me to print another relevant variable.

Thanks,
Oliver