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

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