Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted

[Oliver Hartkopp]

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@xxxxxxxxxxxx/

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@xxxxxxxxxxxx>
> > 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) {