lkml.org 
[lkml]   [2021]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH net v2 2/2] can: isotp: fix tx buffer concurrent access in isotp_sendmsg()
From
Date


On 09.10.21 09:40, Ziyang Xuan wrote:
> 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")
> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com>

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Many thanks!
Oliver

> ---
> net/can/isotp.c | 46 +++++++++++++++++++++++++++++++---------------
> 1 file changed, 31 insertions(+), 15 deletions(-)
>
> 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,
>

\
 
 \ /
  Last update: 2021-10-11 21:15    [W:0.068 / U:0.244 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site