Re: [PATCH 6.1 043/131] can: isotp: isotp_sendmsg(): fix TX state detection and wait behavior

Oliver Hartkopp
2023-10-24T18:37:47Z

Hello Lukas, hello Greg,

this patch fixed the issue introduced with

79e19fa79cb5 ("can: isotp: isotp_ops: fix poll() to not report false
EPOLLOUT events")

for Linux 6.1 and Linux 6.5 which is fine.

Unfortunately the problematic patch has also been applied to 5.15 and
5.10 (referencing another upstream commit as it needed a backport).

@Lukas: The 5.x code is much more similar to the latest code, so would
it probably fix the issue to remove the "wq_has_sleeper(&so->wait)"
condition?

@Greg: I double checked the changes and fixes from the latest 6.6 kernel
compared to the 5.10 when isotp.c was introduced in the mainline kernel.
Would it be ok, to "backport" the latest 6.6 code to the 5.x LTS trees?
It really is the same isotp code but only some kernel API functions and
names have been changed.

Best regards,
Oliver

On 16.10.23 10:40, Greg Kroah-Hartman wrote:
> 6.1-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> From: Lukas Magel <lukas.magel@posteo.net>
>
> [ Upstream commit d9c2ba65e651467de739324d978b04ed8729f483 ]
>
> With patch [1], isotp_poll was updated to also queue the poller in the
> so->wait queue, which is used for send state changes. Since the queue
> now also contains polling tasks that are not interested in sending, the
> queue fill state can no longer be used as an indication of send
> readiness. As a consequence, nonblocking writes can lead to a race and
> lock-up of the socket if there is a second task polling the socket in
> parallel.
>
> With this patch, isotp_sendmsg does not consult wq_has_sleepers but
> instead tries to atomically set so->tx.state and waits on so->wait if it
> is unable to do so. This behavior is in alignment with isotp_poll, which
> also checks so->tx.state to determine send readiness.
>
> V2:
> - Revert direct exit to goto err_event_drop
>
> [1] https://lore.kernel.org/all/20230331125511.372783-1-michal.sojka@cvut.cz
>
> Reported-by: Maxime Jayat <maxime.jayat@mobile-devices.fr>
> Closes: https://lore.kernel.org/linux-can/11328958-453f-447f-9af8-3b5824dfb041@munic.io/
> Signed-off-by: Lukas Magel <lukas.magel@posteo.net>
> Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
> Fixes: 79e19fa79cb5 ("can: isotp: isotp_ops: fix poll() to not report false EPOLLOUT events")
> Link: https://github.com/pylessard/python-udsoncan/issues/178#issuecomment-1743786590
> Link: https://lore.kernel.org/all/20230827092205.7908-1-lukas.magel@posteo.net
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> net/can/isotp.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 8c97f4061ffd7..545889935d39c 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -925,21 +925,18 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> if (!so->bound || so->tx.state == ISOTP_SHUTDOWN)
> return -EADDRNOTAVAIL;
>
> -wait_free_buffer:
> - /* we do not support multiple buffers - for now */
> - if (wq_has_sleeper(&so->wait) && (msg->msg_flags & MSG_DONTWAIT))
> - return -EAGAIN;
> + while (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) {
> + /* we do not support multiple buffers - for now */
> + if (msg->msg_flags & MSG_DONTWAIT)
> + return -EAGAIN;
>
> - /* wait for complete transmission of current pdu */
> - err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
> - if (err)
> - goto err_event_drop;
> -
> - if (cmpxchg(&so->tx.state, ISOTP_IDLE, ISOTP_SENDING) != ISOTP_IDLE) {
> if (so->tx.state == ISOTP_SHUTDOWN)
> return -EADDRNOTAVAIL;
>
> - goto wait_free_buffer;
> + /* wait for complete transmission of current pdu */
> + err = wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
> + if (err)
> + goto err_event_drop;
> }
>
> if (!size || size > MAX_MSG_LENGTH) {