Oliver, what about this patch? Marc On 02.03.2023 10:28:12, Michal Sojka wrote: > When using select/poll/epoll() with a non-blocking ISOTP socket to > wait for when non-blocking write is possible, false EPOLLOUT event is > sometimes returned. This can happen at least after sending a message > which must be split to multiple CAN frames. > > The reason is that isotp_sendmsg() returns -EAGAIN when tx.state is > not equal to ISOTP_IDLE and this behavior is not reflected in > datagram_poll(), which is used in isotp_ops. > > This is fixed by introducing ISOTP-specific poll function, which > suppresses the EPOLLOUT events in that case. > > This can be tested with a program available at > https://github.com/wentasah/isotp-poll-test/blob/fd095b2242c49dc5d3e36faf5ac9f4f47fd002c7/isotp-poll-test.c > which can trigger the problem on a vcan interface. When running the > program as: > > ./isotp-poll-test -s 123 -d 321 -o > > it starts sending ISOTP messages that include increasing ASCII > numbers. poll() is used to wait before the next transmission. > > With current mainline Linux, once the message length is greater than 7 > bytes, write() returns -EAGAIN and the program terminates. This should > not happen, because the previous poll() reported that the write() > would not block. > > After applying this patch, the above command doesn't fail - if one > runs some ISOTP reader such as: > > isotprecv -l -s 321 -d 123 vcan0 > > This test program can also show another problem. When running: > > ./isotp-poll-test -s 321 -d 123 -i -a > > and then in another terminal: > > ./isotp-poll-test -s 123 -d 321 -o > > The first program receives the messages and uses the counter values to > check for lost messages. After a random number of iterations a lost > message is always detected. I believe that ISOTP should be reliable > protocol, at least on vcan, shouldn't it? > > Anyway, this patch doesn't try to address this problem. > > Signed-off-by: Michal Sojka <michal.sojka@xxxxxxx> > Reported-by: Jakub Jira <jirajak2@xxxxxxxxxxx> > > --- > Changelog: > > v2: Added waiting for isotp-specific wait queue: poll_wait(file, &so->wait, wait). > --- > net/can/isotp.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/net/can/isotp.c b/net/can/isotp.c > index 9bc344851704..ec163e36ac53 100644 > --- a/net/can/isotp.c > +++ b/net/can/isotp.c > @@ -1608,6 +1608,21 @@ static int isotp_init(struct sock *sk) > return 0; > } > > +static __poll_t isotp_poll(struct file *file, struct socket *sock, poll_table *wait) > +{ > + struct sock *sk = sock->sk; > + struct isotp_sock *so = isotp_sk(sk); > + > + __poll_t mask = datagram_poll(file, sock, wait); > + poll_wait(file, &so->wait, wait); > + > + /* Check for false positives due to TX state */ > + if ((mask & EPOLLWRNORM) && (so->tx.state != ISOTP_IDLE)) > + mask &= ~(EPOLLOUT | EPOLLWRNORM); > + > + return mask; > +} > + > static int isotp_sock_no_ioctlcmd(struct socket *sock, unsigned int cmd, > unsigned long arg) > { > @@ -1623,7 +1638,7 @@ static const struct proto_ops isotp_ops = { > .socketpair = sock_no_socketpair, > .accept = sock_no_accept, > .getname = isotp_getname, > - .poll = datagram_poll, > + .poll = isotp_poll, > .ioctl = isotp_sock_no_ioctlcmd, > .gettstamp = sock_gettstamp, > .listen = sock_no_listen, > -- > 2.39.2 > > -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
