linux-kernel - Re: can: isotp: epoll breaks isotp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <04fd32bc-b1c4-b9c3-3f8b-7987704a1f85@hartkopp.net>
Date:   Tue, 22 Aug 2023 18:37:14 +0200
From:   Oliver Hartkopp <socketcan@...tkopp.net>
To:     Lukas Magel <lukas.magel@...teo.net>,
        Michal Sojka <michal.sojka@...t.cz>,
        Maxime Jayat <maxime.jayat@...ile-devices.fr>,
        Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     linux-can@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        "Dae R. Jeong" <threeearcat@...il.com>,
        Hillf Danton <hdanton@...a.com>
Subject: Re: can: isotp: epoll breaks isotp_sendmsg

Hi Lukas,

On 22.08.23 08:51, Lukas Magel wrote:

>>> @Oliver I adjusted the exit path for the case where the initial wait is
>>> interrupted to return immediately instead of jumping to err_event_drop.
>>> Could you please check if you would agree with this change?
>> The code has really won with your change! Thanks!
>>
>> But as you already assumed I have a problem with the handling of the
>> cleanup when a signal interrupts the wait_event_interruptible() statement.
>>
>> I think it should still be:
>>
>> /* wait for complete transmission of current pdu */
>> err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
>> if (err)
>>           goto err_event_drop;
>>
>> as we need to make sure that the state machine is set to defined values
>> and states for the next isotp_sendmsg() attempt.
>>
>> Best regards,
>> Oliver
> 
> 
> Thank you for the feedback! Can you elaborate why the state needs to be
> reset here? For me, the loop is basically a "let's wait until we win
> arbitration for the tx.state", which means that the task is allowed
> to send. I'm imagining an application that has two threads, both sending
> at the same time (because maybe they don't care about reading). So one
> would always be waiting in the loop until the send operation of the other
> has concluded. My motivation for not going to err_event_drop was that if
> one thread was interrupted in its wait_event_interruptible, why would we
> need to change tx.state that is currently being occupied by the other
> thread? The thread waiting in the loop has not done any state manipulation
> of the socket.

Please don't only look at the isotp_sendmsg() function but the other 
possibilities e.g. from timeouts.

Look for the documentation from the commit 051737439eaee. This patch has 
been added recently as it was needed.

Best regards,
Oliver