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);
