Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted

[Oliver Hartkopp]

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);
>