lkml.org 
[lkml]   [2023]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: WARNING in isotp_tx_timer_handler and WARNING in print_tainted
From
Hi,

On 26.03.23 10:10, Dae R. Jeong wrote:
> Hi,
>
> I am curious about the error handling logic in isotp_sendmsg() which
> looks a bit unclear to me.
>
> I was looking the `WARNING in isotp_tx_timer_handler` warning [1],
> which was firstly addressed by a commit [2] but reoccured even after
> the commit.
> [1]: https://syzkaller.appspot.com/bug?id=4f492d593461a5e44d76dd9322e179d13191a8ef
> [2]: c6adf659a8ba can: isotp: check CAN address family in isotp_bind()
>
> I thought that the warning is caused by the concurrent execution of
> two isotp_sendmsg() as described below (I'm not 100% sure though).
>
> CPU1 CPU2
> isotp_sendmsg() isotp_sendmsg()
> ----- -----
> old_state = so->tx.state; // ISOTP_IDLE
>
> cmpxchg(&so->tx.state, ISTOP_IDLE, ISOTP_SENDING) // success
> ...
> so->tx.state = ISTOP_WAIT_FIRST_FC;
> hrtimer_start(&so->txtimer);
>
> cmpxchg(&so->tx.state, ISTOP_IDLE, ISOTP_SENDING) // failed
> // if MSG_DONTWAIT is set in msg->msg_flags or
> // a signal is delivered during wait_event_interruptible()
> goto err_out;
> err_out:
> so->tx.state = old_state; // ISTOP_IDLE
>
> isotp_tx_timer_handler()
> -----
> switch (so->tx.state) {
> default:
> WARN_ONCE();
> }
>
> Then, a commit [3] changed the logic of tx timer, and removed the
> WARN_ONCE() statement. So I thought that the issue is completely
> handled.
> [3]: 4f027cba8216 can: isotp: split tx timer into transmission and timeout
>
> But even after [3] is applied, I found a warning that seems related
> occurred [4] (in the kernel commit: 478a351ce0d6).
> [4]: https://syzkaller.appspot.com/bug?id=11d0e5f6fef53a0ea486bbd07ddd3cba66132150
>
> So I wonder whether the `err_out` logic in isotp_sendmsg() is safe.
> For me, it looks like isotp_sendmsg() can change so->tx.state to
> ISTOP_IDLE at any time. It may not be a problem if all other locations
> are aware of this. Is this an intended behavior?
>
> Thank you in advance.

Thank you for picking this up!

In fact I was not aware of the possibility of a concurrent execution of
isotp_sendmsg() and thought cmpxchg() would just make it ...

But looking at other *_sendmsg() implementations a lock_sock() seems to
be a common pattern to handle concurrent syscalls, see:

git grep -p lock_sock net | grep sendmsg

What do you think about adopting this to isotp_sendmsg()? See patch below.

Best regards,
Oliver

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;


\
 
 \ /
  Last update: 2023-03-27 01:17    [W:0.117 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site