On Sun, Mar 26, 2023 at 06:17:17PM +0200, Oliver Hartkopp wrote:
> 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.
Yes, I see lock_sock() is a common pattern in *_sendmsg(). One thing I
wonder is whether sleeping (i.e., wait_event_*) after lock_sock() is
safe or not, as lock_sock() seems to have mutex_lock() semantics.
Perhaps we may need to unlock - wait_event - lock in istop_sendmsg()?
If so, we also need to consider various possible thread interleaving
cases.
> > 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
>
> (..)
Hmm... my original thought was that 1) isotp_sendmsg() waiting the
event (so->tx.state == ISTOP_IDLE) does not touch anything related to
the socket as well as the sending process yet, so 2) this
isotp_sendmsg() does not need to change the tx.state if it returns an
error due to a signal. I'm not sure that we need to set tx.state in
this case. Do we still need to do it?
Best regards,
Dae R. Jeong.
