Messages in this thread Patch in this message | | | Date | Sun, 26 Mar 2023 18:17:17 +0200 | Subject | Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted | From | Oliver Hartkopp <> |
| |
Hi Dae,
On 26.03.23 13:55, Dae R. Jeong wrote: >> 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.
Yes, I feel you!
We use lock_sock() also in the notifier which is called when someone removes the CAN interface.
But the other cases for e.g. set_sockopt() and for sendmsg() seem to be a common pattern to lock concurrent user space calls.
> 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. Your described case in the original post indeed shows that this might lead to a problem.
> 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?
Hm, interesting idea.
But in which state will so->tx.state be here:
/* wait for complete transmission of current pdu */ err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE); if (err) goto err_out;
Should we better set the tx.state in the error case?
if (err) { so->tx.state = ISOTP_IDLE; goto err_out; }
Best regards, Oliver
(..)
> > 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); >
| |