Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <675932fa-f0df-ad53-d517-ee5854c9245e@hartkopp.net>
Date:   Fri, 31 Mar 2023 12:25:42 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted
To:     Hillf Danton <hdanton@sina.com>,
        "Dae R. Jeong" <threeearcat@gmail.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-can <linux-can@vger.kernel.org>
References: <ZB/93xJxq/BUqAgG@dragonet>
 <31c4a218-ee1b-4b64-59b6-ba5ef6ecce3c@hartkopp.net>
 <ZCAytf0CpfAhjUSe@dragonet> <20230327014843.2431-1-hdanton@sina.com>
Content-Language: en-US
From:   Oliver Hartkopp <socketcan@hartkopp.net>
In-Reply-To: <20230327014843.2431-1-hdanton@sina.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@hartkopp.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) {