Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 27 Mar 2023 10:58:27 +0900
From:   "Dae R. Jeong" <threeearcat@gmail.com>
To:     Oliver Hartkopp <socketcan@hartkopp.net>
Cc:     mkl@pengutronix.de, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, linux-can@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: WARNING in isotp_tx_timer_handler and WARNING in print_tainted
Message-ID: <ZCD4Q2rHnQokUxbe@dragonet>
References: <ZB/93xJxq/BUqAgG@dragonet>
 <31c4a218-ee1b-4b64-59b6-ba5ef6ecce3c@hartkopp.net>
 <ZCAytf0CpfAhjUSe@dragonet>
 <81ebf23b-f539-5782-2abd-8db8a232bb72@hartkopp.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <81ebf23b-f539-5782-2abd-8db8a232bb72@hartkopp.net>
Precedence: bulk

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.