Messages in this thread | | | Subject | Re: [PATCH net v2 2/2] can: isotp: fix tx buffer concurrent access in isotp_sendmsg() | From | Oliver Hartkopp <> | Date | Mon, 11 Oct 2021 21:11:25 +0200 |
| |
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, >
| |