Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi Dae,

On 26.03.23 13:55, Dae R. Jeong wrote:
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index 9bc344851704..0b95c0df7a63 100644
>> --- a/net/can/isotp.c
>> +++ b/net/can/isotp.c
>> @@ -912,13 +912,12 @@ static enum hrtimer_restart
>> isotp_txfr_timer_handler(struct hrtimer *hrtimer)
>>   		isotp_send_cframe(so);
>>
>>   	return HRTIMER_NORESTART;
>>   }
>>
>> -static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t
>> size)
>> +static int isotp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t
>> size)
>>   {
>> -	struct sock *sk = sock->sk;
>>   	struct isotp_sock *so = isotp_sk(sk);
>>   	u32 old_state = so->tx.state;
>>   	struct sk_buff *skb;
>>   	struct net_device *dev;
>>   	struct canfd_frame *cf;
>> @@ -1091,10 +1090,22 @@ static int isotp_sendmsg(struct socket *sock, struct
>> msghdr *msg, size_t size)
>>   		wake_up_interruptible(&so->wait);
>>
>>   	return err;
>>   }
>>
>> +static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t
>> size)
>> +{
>> +	struct sock *sk = sock->sk;
>> +	int ret;
>> +
>> +	lock_sock(sk);
>> +	ret = isotp_sendmsg_locked(sk, msg, size);
>> +	release_sock(sk);
>> +
>> +	return ret;
>> +}
>> +
>>   static int isotp_recvmsg(struct socket *sock, struct msghdr *msg, size_t
>> size,
>>   			 int flags)
>>   {
>>   	struct sock *sk = sock->sk;
>>   	struct sk_buff *skb;
> 
> Hi, Oliver.
> 
> It seems that the patch should address the scenario I was thinking
> of. But using a lock is always scary for a newbie like me because of
> the possibility of causing other problems, e.g., deadlock. If it does
> not cause other problems, it looks good for me.

Yes, I feel you!

We use lock_sock() also in the notifier which is called when someone 
removes the CAN interface.

But the other cases for e.g. set_sockopt() and for sendmsg() seem to be 
a common pattern to lock concurrent user space calls.

> Or although I'm not sure about this, what about getting rid of
> reverting so->tx.state to old_state?
> 
> I think the concurrent execution of isotp_sendmsg() would be
> problematic when reverting so->tx.state to old_state after goto'ing
> err_out.
Your described case in the original post indeed shows that this might 
lead to a problem.

> There are two locations of "goto err_out", and
> iostp_sendmsg() does nothing to the socket before both of "goto
> err_out". So after goto'ing err_out, it seems fine for me even if we
> do not revert so->tx.state to old_state.
> 
> 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

(..)

> 
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 9bc344851704..4630fad13803 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -918,7 +918,6 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>   {
>   	struct sock *sk = sock->sk;
>   	struct isotp_sock *so = isotp_sk(sk);
> -	u32 old_state = so->tx.state;
>   	struct sk_buff *skb;
>   	struct net_device *dev;
>   	struct canfd_frame *cf;
> @@ -1084,9 +1083,8 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>   
>   err_out_drop:
>   	/* drop this PDU and unlock a potential wait queue */
> -	old_state = ISOTP_IDLE;
> +	so->tx.state = ISOTP_IDLE;
>   err_out:
> -	so->tx.state = old_state;
>   	if (so->tx.state == ISOTP_IDLE)
>   		wake_up_interruptible(&so->wait);
>