netdev - Re: WARNING in isotp_tx_timer_handler and WARNING in print

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>] [day] [month] [year] [list]

Date:   Fri, 31 Mar 2023 12:25:42 +0200
From:   Oliver Hartkopp <socketcan@...tkopp.net>
To:     Hillf Danton <hdanton@...a.com>,
        "Dae R. Jeong" <threeearcat@...il.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-can <linux-can@...r.kernel.org>
Subject: Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted

Hey all,

looking at the code simplification from Hillf I reworked some of the 
cmpxchg() code and removed the old_state mechanism.

I posted a RFC patch here:

https://lore.kernel.org/linux-can/20230331102114.15164-1-socketcan@hartkopp.net/

Please comment on the patch whether you think if this could be an 
improvement.

Many thanks,
Oliver

On 27.03.23 03:48, Hillf Danton wrote:
> On Sun, 26 Mar 2023 18:17:17 +0200 Oliver Hartkopp <socketcan@...tkopp.net>
>> On 26.03.23 13:55, Dae R. Jeong wrote:
>>>
>>> If I think correctly, this will make cmpxchg() work, and prevent the
>>> problematic concurrent execution. Could you please check the patch
>>> below?
>>
>> Hm, interesting idea.
>>
>> But in which state will so->tx.state be here:
>>
>> /* wait for complete transmission of current pdu */
>> err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
>> if (err)
>> 	goto err_out;
>>
>>
>> Should we better set the tx.state in the error case?
>>
>> if (err) {
>> 	so->tx.state = ISOTP_IDLE;
>> 	goto err_out;
>> }
>>
>> Best regards,
>> Oliver
> 
> Another 2c only if cmpxchg is preferred.
> 
> +++ b/net/can/isotp.c
> @@ -932,19 +932,24 @@ static int isotp_sendmsg(struct socket *
>   		return -EADDRNOTAVAIL;
>   
>   	/* we do not support multiple buffers - for now */
> -	if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE ||
> -	    wq_has_sleeper(&so->wait)) {
> -		if (msg->msg_flags & MSG_DONTWAIT) {
> -			err = -EAGAIN;
> -			goto err_out;
> -		}
> -
> +	if (wq_has_sleeper(&so->wait)) {
> +		if (msg->msg_flags & MSG_DONTWAIT)
> +			return -EAGAIN;
>   		/* wait for complete transmission of current pdu */
>   		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
>   		if (err)
> -			goto err_out;
> +			return err;
> +	}
>   
> -		so->tx.state = ISOTP_SENDING;
> +again:
> +	old_state = cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING);
> +	if (old_state != ISOTP_IDLE) {
> +		if (msg->msg_flags & MSG_DONTWAIT)
> +			return -EAGAIN;
> +		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
> +		if (err)
> +			return err;
> +		goto again;
>   	}
>   
>   	if (!size || size > MAX_MSG_LENGTH) {