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;
Date:   Sun, 26 Mar 2023 20:55:33 +0900
From:   "Dae R. Jeong" <threeearcat@gmail.com>
To:     Oliver Hartkopp <socketcan@hartkopp.net>
Cc:     mkl@pengutronix.de, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, linux-can@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted
Message-ID: <ZCAytf0CpfAhjUSe@dragonet>
References: <ZB/93xJxq/BUqAgG@dragonet>
 <31c4a218-ee1b-4b64-59b6-ba5ef6ecce3c@hartkopp.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <31c4a218-ee1b-4b64-59b6-ba5ef6ecce3c@hartkopp.net>
Precedence: bulk

> 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.

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. 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?

Best regards,
Dae R. Jeong.

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