Re: FAILED: patch "[PATCH] can: isotp: isotp_sendmsg(): fix TX buffer concurrent access" failed to apply to 5.10-stable tree

[Oliver Hartkopp <socketcan@xxxxxxxxxxxx>]

Hello Greg,

Linux 5.10 is missing the SF_BROADCAST support which was introduced in 
5.11 - that broke the application of the original patch.

Just sent out a patch for 5.10-stable some minutes ago:

[PATCH 5.10-stable] can: isotp: isotp_sendmsg(): fix TX buffer 
concurrent access in isotp_sendmsg()

Thanks for your work!

Best regards,
Oliver

On 24.10.21 13:27, gregkh@xxxxxxxxxxxxxxxxxxx wrote:
> The patch below does not apply to the 5.10-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@xxxxxxxxxxxxxxx>.
>
> thanks,
>
> greg k-h
>
> ------------------ original commit in Linus's tree ------------------
>
>  From 43a08c3bdac4cb42eff8fe5e2278bffe0c5c3daa Mon Sep 17 00:00:00 2001
> From: Ziyang Xuan <william.xuanziyang@xxxxxxxxxx>
> Date: Sat, 9 Oct 2021 15:40:30 +0800
> Subject: [PATCH] can: isotp: isotp_sendmsg(): fix TX buffer concurrent access
>   in isotp_sendmsg()
>
> When isotp_sendmsg() concurrent, tx.state of all TX processes can be
> ISOTP_IDLE. The conditions so->tx.state != ISOTP_IDLE and
> wq_has_sleeper(&so->wait) can not protect TX buffer from being
> accessed by multiple TX processes.
>
> We can use cmpxchg() to try to modify tx.state to ISOTP_SENDING firstly.
> If the modification of the previous process succeed, the later process
> must wait tx.state to ISOTP_IDLE firstly. Thus, we can ensure TX buffer
> is accessed by only one process at the same time. And we should also
> restore the original tx.state at the subsequent error processes.
>
> Fixes: e057dd3fc20f ("can: add ISO 15765-2:2016 transport protocol")
> Link: https://lore.kernel.org/all/c2517874fbdf4188585cf9ddf67a8fa74d5dbde5.1633764159.git.william.xuanziyang@xxxxxxxxxx
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Ziyang Xuan <william.xuanziyang@xxxxxxxxxx>
> Acked-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
> Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
>
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 2ac29c2b2ca6..d1f54273c0bb 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -121,7 +121,7 @@ enum {
>   struct tpcon {
>   	int idx;
>   	int len;
> -	u8 state;
> +	u32 state;
>   	u8 bs;
>   	u8 sn;
>   	u8 ll_dl;
> @@ -848,6 +848,7 @@ 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;
> @@ -860,47 +861,55 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>   		return -EADDRNOTAVAIL;
>   
>   	/* we do not support multiple buffers - for now */
> -	if (so->tx.state != ISOTP_IDLE || wq_has_sleeper(&so->wait)) {
> -		if (msg->msg_flags & MSG_DONTWAIT)
> -			return -EAGAIN;
> +	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;
> +		}
>   
>   		/* wait for complete transmission of current pdu */
>   		err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
>   		if (err)
> -			return err;
> +			goto err_out;
>   	}
>   
> -	if (!size || size > MAX_MSG_LENGTH)
> -		return -EINVAL;
> +	if (!size || size > MAX_MSG_LENGTH) {
> +		err = -EINVAL;
> +		goto err_out;
> +	}
>   
>   	/* take care of a potential SF_DL ESC offset for TX_DL > 8 */
>   	off = (so->tx.ll_dl > CAN_MAX_DLEN) ? 1 : 0;
>   
>   	/* does the given data fit into a single frame for SF_BROADCAST? */
>   	if ((so->opt.flags & CAN_ISOTP_SF_BROADCAST) &&
> -	    (size > so->tx.ll_dl - SF_PCI_SZ4 - ae - off))
> -		return -EINVAL;
> +	    (size > so->tx.ll_dl - SF_PCI_SZ4 - ae - off)) {
> +		err = -EINVAL;
> +		goto err_out;
> +	}
>   
>   	err = memcpy_from_msg(so->tx.buf, msg, size);
>   	if (err < 0)
> -		return err;
> +		goto err_out;
>   
>   	dev = dev_get_by_index(sock_net(sk), so->ifindex);
> -	if (!dev)
> -		return -ENXIO;
> +	if (!dev) {
> +		err = -ENXIO;
> +		goto err_out;
> +	}
>   
>   	skb = sock_alloc_send_skb(sk, so->ll.mtu + sizeof(struct can_skb_priv),
>   				  msg->msg_flags & MSG_DONTWAIT, &err);
>   	if (!skb) {
>   		dev_put(dev);
> -		return err;
> +		goto err_out;
>   	}
>   
>   	can_skb_reserve(skb);
>   	can_skb_prv(skb)->ifindex = dev->ifindex;
>   	can_skb_prv(skb)->skbcnt = 0;
>   
> -	so->tx.state = ISOTP_SENDING;
>   	so->tx.len = size;
>   	so->tx.idx = 0;
>   
> @@ -956,7 +965,7 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>   	if (err) {
>   		pr_notice_once("can-isotp: %s: can_send_ret %pe\n",
>   			       __func__, ERR_PTR(err));
> -		return err;
> +		goto err_out;
>   	}
>   
>   	if (wait_tx_done) {
> @@ -965,6 +974,13 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>   	}
>   
>   	return size;
> +
> +err_out:
> +	so->tx.state = old_state;
> +	if (so->tx.state == ISOTP_IDLE)
> +		wake_up_interruptible(&so->wait);
> +
> +	return err;
>   }
>   
>   static int isotp_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,