Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Sottas Guillaume (LMB)]

Hello,

First of all thank you for your fast reply.

From my point of view, I think it would make sense to return an error to the user when CAN_ISOTP_WAIT_TX_DONE option is set. Otherwise, I don't understand the use case of this option?
If the CAN_ISOTP_WAIT_TX_DONE option is not set, then not receiving an error on timeout could be the expected behavior, as the user does not explicitly ask for completion.

Concerning the way of reporting the error, setting the errno would be a nice solution, as it would not require that much modifications. However, as Mark mentioned, the errno is not set.
I'm not an expert in how this sk_error_report works internally, but I will try to get more infos about it.

In both cases, the documentation in the isotp.c (https://elixir.bootlin.com/linux/latest/source/net/can/isotp.c#L7) file is not correct, as it is not the actual behavior (most likely the expected).
Additionally, I guess the -ETIMEOUT, -EILSEQ, -EBADMSG,... described in the same documentation are most likely not working that way either, but would be fixed if the errno is reported correctly.

Best Regards,

Guillaume

Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Oliver Hartkopp]

On 28.04.21 13:43, Sottas Guillaume (LMB) wrote:
> Hello,
> 
> First of all thank you for your fast reply.
> 
>  From my point of view, I think it would make sense to return an error to the user when CAN_ISOTP_WAIT_TX_DONE option is set. Otherwise, I don't understand the use case of this option?
> If the CAN_ISOTP_WAIT_TX_DONE option is not set, then not receiving an error on timeout could be the expected behavior, as the user does not explicitly ask for completion.
> 
> Concerning the way of reporting the error, setting the errno would be a nice solution, as it would not require that much modifications. However, as Mark mentioned, the errno is not set.
> I'm not an expert in how this sk_error_report works internally, but I will try to get more infos about it.
> 
> In both cases, the documentation in the isotp.c (https://elixir.bootlin.com/linux/latest/source/net/can/isotp.c#L7) file is not correct, as it is not the actual behavior (most likely the expected).
> Additionally, I guess the -ETIMEOUT, -EILSEQ, -EBADMSG,... described in the same documentation are most likely not working that way either, but would be fixed if the errno is reported correctly.

Yep.

The sk_error reporting is obviously not the way to go.

See:
https://lore.kernel.org/linux-can/ee8058e0-af0a-8759-a62b-b1585c8992b3@hartkopp.net/

Best regards,
Oliver

Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Sottas Guillaume (LMB)]

Hello Marc,

Thank you for your investigations, seems to be the way to go? What is the next step, could I help in any way or are you going to merge your patch directly?
I'm not really sure how we should go further in order to inegrate your fix in the code...

Thank you in advance for your feedback,

Best Regards,

Guillaume

Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On 06.05.2021 12:45:38, Sottas Guillaume (LMB) wrote:
> Thank you for your investigations, seems to be the way to go? What is
> the next step, could I help in any way or are you going to merge your
> patch directly? I'm not really sure how we should go further in order
> to inegrate your fix in the code...

Maybe we can use sk->sk_err directly instead of adding a new variable.
I've send a patch, please test.

> Thank you in advance for your feedback,

Thanks,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Marc Kleine-Budde]

From: "Sottas Guillaume (LMB)" <Guillaume.Sottas@liebherr.com>

Link: https://github.com/hartkopp/can-isotp/pull/43
Cc: Oliver Hartkopp <socketcan@hartkopp.net>
Not-Signed-off-by: Sottas Guillaume (LMB) <Guillaume.Sottas@liebherr.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---

Hello,

for better reviewing I'm posting Sottas's patch here. While porting to
current net/master I've fixed up the indention.

regards,
Marc

 net/can/isotp.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 9f94ad3caee9..d02e8f1f240f 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -110,6 +110,11 @@ MODULE_ALIAS("can-proto-6");
 #define ISOTP_FC_WT 1		/* wait */
 #define ISOTP_FC_OVFLW 2	/* overflow */
 
+enum {
+	ISOTP_ERR_NO_ERROR = 0,
+	ISOTP_ERR_FC_TIMEOUT,
+};
+
 enum {
 	ISOTP_IDLE = 0,
 	ISOTP_WAIT_FIRST_FC,
@@ -122,6 +127,7 @@ struct tpcon {
 	int idx;
 	int len;
 	u8 state;
+	u8 error;
 	u8 bs;
 	u8 sn;
 	u8 ll_dl;
@@ -756,6 +762,10 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
 		if (!sock_flag(sk, SOCK_DEAD))
 			sk->sk_error_report(sk);
 
+		/* set error flag in order to consume it later in the
+		 * isotp_sendmsg function */
+		so->tx.error = ISOTP_ERR_FC_TIMEOUT;
+
 		/* reset tx state */
 		so->tx.state = ISOTP_IDLE;
 		wake_up_interruptible(&so->wait);
@@ -954,6 +964,13 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	if (wait_tx_done) {
 		/* wait for complete transmission of current pdu */
 		wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
+
+		/* check if an error has been raised in the timer
+		 * function handler */
+		if (so->tx.error == ISOTP_ERR_FC_TIMEOUT) {
+			so->tx.error = ISOTP_ERR_NO_ERROR;
+			return -ECOMM;
+		}
 	}
 
 	return size;
@@ -1371,6 +1388,9 @@ static int isotp_init(struct sock *sk)
 	so->tx.state = ISOTP_IDLE;
 
 	hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
+
+	so->tx.error = ISOTP_ERR_NO_ERROR;
+	so->rx.error = ISOTP_ERR_NO_ERROR;
 	so->rxtimer.function = isotp_rx_timer_handler;
 	hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	so->txtimer.function = isotp_tx_timer_handler;
-- 
2.30.2

Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3001 bytes --]

On 28.04.2021 11:09:14, Marc Kleine-Budde wrote:
> From: "Sottas Guillaume (LMB)" <Guillaume.Sottas@liebherr.com>
> 
> Link: https://github.com/hartkopp/can-isotp/pull/43
> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
> Not-Signed-off-by: Sottas Guillaume (LMB) <Guillaume.Sottas@liebherr.com>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
> 
> Hello,
> 
> for better reviewing I'm posting Sottas's patch here. While porting to
> current net/master I've fixed up the indention.
> 
> regards,
> Marc
> 
>  net/can/isotp.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 9f94ad3caee9..d02e8f1f240f 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -110,6 +110,11 @@ MODULE_ALIAS("can-proto-6");
>  #define ISOTP_FC_WT 1		/* wait */
>  #define ISOTP_FC_OVFLW 2	/* overflow */
>  
> +enum {
> +	ISOTP_ERR_NO_ERROR = 0,
> +	ISOTP_ERR_FC_TIMEOUT,
> +};
> +
>  enum {
>  	ISOTP_IDLE = 0,
>  	ISOTP_WAIT_FIRST_FC,
> @@ -122,6 +127,7 @@ struct tpcon {
>  	int idx;
>  	int len;
>  	u8 state;
> +	u8 error;
>  	u8 bs;
>  	u8 sn;
>  	u8 ll_dl;
> @@ -756,6 +762,10 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)

>               sk->sk_err = ECOMM;
>  		if (!sock_flag(sk, SOCK_DEAD))
>  			sk->sk_error_report(sk);

I think the idea is that sk_error_report takes care of propagation of
the error to the user space. I don't know why it's not working as
expected.

>  
> +		/* set error flag in order to consume it later in the
> +		 * isotp_sendmsg function */
> +		so->tx.error = ISOTP_ERR_FC_TIMEOUT;
> +
>  		/* reset tx state */
>  		so->tx.state = ISOTP_IDLE;
>  		wake_up_interruptible(&so->wait);
> @@ -954,6 +964,13 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>  	if (wait_tx_done) {
>  		/* wait for complete transmission of current pdu */
>  		wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
> +
> +		/* check if an error has been raised in the timer
> +		 * function handler */
> +		if (so->tx.error == ISOTP_ERR_FC_TIMEOUT) {
> +			so->tx.error = ISOTP_ERR_NO_ERROR;
> +			return -ECOMM;
> +		}
>  	}
>  
>  	return size;
> @@ -1371,6 +1388,9 @@ static int isotp_init(struct sock *sk)
>  	so->tx.state = ISOTP_IDLE;
>  
>  	hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
> +
> +	so->tx.error = ISOTP_ERR_NO_ERROR;
> +	so->rx.error = ISOTP_ERR_NO_ERROR;
>  	so->rxtimer.function = isotp_rx_timer_handler;
>  	hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
>  	so->txtimer.function = isotp_tx_timer_handler;


Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Oliver Hartkopp]

Hello Marc and Guillaume,

thanks for picking this up here. I was busy the last days.

On 28.04.21 11:12, Marc Kleine-Budde wrote:
> On 28.04.2021 11:09:14, Marc Kleine-Budde wrote:
>> From: "Sottas Guillaume (LMB)" <Guillaume.Sottas@liebherr.com>
>>
>> Link: https://github.com/hartkopp/can-isotp/pull/43
>> Cc: Oliver Hartkopp <socketcan@hartkopp.net>
>> Not-Signed-off-by: Sottas Guillaume (LMB) <Guillaume.Sottas@liebherr.com>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>>
>> Hello,
>>
>> for better reviewing I'm posting Sottas's patch here. While porting to
>> current net/master I've fixed up the indention.
>>
>> regards,
>> Marc
>>
>>   net/can/isotp.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index 9f94ad3caee9..d02e8f1f240f 100644
>> --- a/net/can/isotp.c
>> +++ b/net/can/isotp.c
>> @@ -110,6 +110,11 @@ MODULE_ALIAS("can-proto-6");
>>   #define ISOTP_FC_WT 1		/* wait */
>>   #define ISOTP_FC_OVFLW 2	/* overflow */
>>   
>> +enum {
>> +	ISOTP_ERR_NO_ERROR = 0,
>> +	ISOTP_ERR_FC_TIMEOUT,
>> +};
>> +
>>   enum {
>>   	ISOTP_IDLE = 0,
>>   	ISOTP_WAIT_FIRST_FC,
>> @@ -122,6 +127,7 @@ struct tpcon {
>>   	int idx;
>>   	int len;
>>   	u8 state;
>> +	u8 error;
>>   	u8 bs;
>>   	u8 sn;
>>   	u8 ll_dl;
>> @@ -756,6 +762,10 @@ static enum hrtimer_restart isotp_tx_timer_handler(struct hrtimer *hrtimer)
> 
>>                sk->sk_err = ECOMM;
>>   		if (!sock_flag(sk, SOCK_DEAD))
>>   			sk->sk_error_report(sk);
> 
> I think the idea is that sk_error_report takes care of propagation of
> the error to the user space. I don't know why it's not working as
> expected.

Yes, I fact I was thinking about this question too.

> 
>>   
>> +		/* set error flag in order to consume it later in the
>> +		 * isotp_sendmsg function */
>> +		so->tx.error = ISOTP_ERR_FC_TIMEOUT;
>> +
>>   		/* reset tx state */
>>   		so->tx.state = ISOTP_IDLE;
>>   		wake_up_interruptible(&so->wait);
>> @@ -954,6 +964,13 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>>   	if (wait_tx_done) {
>>   		/* wait for complete transmission of current pdu */
>>   		wait_event_interruptible(so->wait, so->tx.state == ISOTP_IDLE);
>> +
>> +		/* check if an error has been raised in the timer
>> +		 * function handler */
>> +		if (so->tx.error == ISOTP_ERR_FC_TIMEOUT) {
>> +			so->tx.error = ISOTP_ERR_NO_ERROR;
>> +			return -ECOMM;

Maybe the way to trigger the sk_error_report(sk) we might return '-1' 
while the error is then propagated inside errno.

But I haven't checked that myself so far.

The reason why this return value is not that important is that ISO-TP is 
an (unreliable!) UDP like protocol where the application checks for an 
application response timeout.

Having the information of a FF timeout only makes sense when 
CAN_ISOTP_WAIT_TX_DONE is enabled to assign the timeout to a that 
specific PDU.

Best,
Oliver

>> +		}
>>   	}
>>   
>>   	return size;
>> @@ -1371,6 +1388,9 @@ static int isotp_init(struct sock *sk)
>>   	so->tx.state = ISOTP_IDLE;
>>   
>>   	hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
>> +
>> +	so->tx.error = ISOTP_ERR_NO_ERROR;
>> +	so->rx.error = ISOTP_ERR_NO_ERROR;
>>   	so->rxtimer.function = isotp_rx_timer_handler;
>>   	hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
>>   	so->txtimer.function = isotp_tx_timer_handler;
> 
> 
> Marc
> 

Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Oliver Hartkopp]

On 28.04.21 11:26, Oliver Hartkopp wrote:
> On 28.04.21 11:12, Marc Kleine-Budde wrote:
>> On 28.04.2021 11:09:14, Marc Kleine-Budde wrote:

>>> @@ -756,6 +762,10 @@ static enum hrtimer_restart 
>>> isotp_tx_timer_handler(struct hrtimer *hrtimer)
>>
>>>                sk->sk_err = ECOMM;
>>>           if (!sock_flag(sk, SOCK_DEAD))
>>>               sk->sk_error_report(sk);
>>
>> I think the idea is that sk_error_report takes care of propagation of
>> the error to the user space. I don't know why it's not working as
>> expected.
> 
> Yes, I fact I was thinking about this question too.
> 
>>
>>> +        /* set error flag in order to consume it later in the
>>> +         * isotp_sendmsg function */
>>> +        so->tx.error = ISOTP_ERR_FC_TIMEOUT;
>>> +
>>>           /* reset tx state */
>>>           so->tx.state = ISOTP_IDLE;
>>>           wake_up_interruptible(&so->wait);
>>> @@ -954,6 +964,13 @@ static int isotp_sendmsg(struct socket *sock, 
>>> struct msghdr *msg, size_t size)
>>>       if (wait_tx_done) {
>>>           /* wait for complete transmission of current pdu */
>>>           wait_event_interruptible(so->wait, so->tx.state == 
>>> ISOTP_IDLE);
>>> +
>>> +        /* check if an error has been raised in the timer
>>> +         * function handler */
>>> +        if (so->tx.error == ISOTP_ERR_FC_TIMEOUT) {
>>> +            so->tx.error = ISOTP_ERR_NO_ERROR;
>>> +            return -ECOMM;
> 
> Maybe the way to trigger the sk_error_report(sk) we might return '-1' 
> while the error is then propagated inside errno.

I added some debug print in isotpsend:

diff --git a/isotpsend.c b/isotpsend.c
index 3ea574c..c2937fa 100644
--- a/isotpsend.c
+++ b/isotpsend.c
@@ -45,10 +45,11 @@
  #include <libgen.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
  #include <unistd.h>
+#include <errno.h>

  #include <net/if.h>
  #include <sys/ioctl.h>
  #include <sys/socket.h>
  #include <sys/types.h>
@@ -252,10 +253,11 @@ int main(int argc, char **argv)
                     buf[buflen] = ((buflen % 0xFF) + 1) & 0xFF;
      }


      retval = write(s, buf, buflen);
+    printf("retval %d errno %d\n", retval, errno);
      if (retval < 0) {
             perror("write");
             return retval;
      }


$ date +%S.%N && ./isotpsend vcan0 -s 123 -d 321 -D 44 -b && date +%S.%N
43.269173590
retval 44 errno 0
44.271162277

So it waits for the timeout as required by the '-b' option - but the 
errno is not set :-/

> 
> But I haven't checked that myself so far.
> 
> The reason why this return value is not that important is that ISO-TP is 
> an (unreliable!) UDP like protocol where the application checks for an 
> application response timeout.
> 
> Having the information of a FF timeout only makes sense when 
> CAN_ISOTP_WAIT_TX_DONE is enabled to assign the timeout to a that 
> specific PDU.
> 
> Best,
> Oliver
> 
>>> +        }
>>>       }
>>>       return size;
>>> @@ -1371,6 +1388,9 @@ static int isotp_init(struct sock *sk)
>>>       so->tx.state = ISOTP_IDLE;
>>>       hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC, 
>>> HRTIMER_MODE_REL_SOFT);
>>> +
>>> +    so->tx.error = ISOTP_ERR_NO_ERROR;
>>> +    so->rx.error = ISOTP_ERR_NO_ERROR;
>>>       so->rxtimer.function = isotp_rx_timer_handler;
>>>       hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, 
>>> HRTIMER_MODE_REL_SOFT);
>>>       so->txtimer.function = isotp_tx_timer_handler;
>>
>>
>> Marc
>>

Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1592 bytes --]

On 28.04.2021 12:24:32, Oliver Hartkopp wrote:
> > Maybe the way to trigger the sk_error_report(sk) we might return '-1'
> > while the error is then propagated inside errno.
> 
> I added some debug print in isotpsend:
> 
> diff --git a/isotpsend.c b/isotpsend.c
> index 3ea574c..c2937fa 100644
> --- a/isotpsend.c
> +++ b/isotpsend.c
> @@ -45,10 +45,11 @@
>  #include <libgen.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <errno.h>
> 
>  #include <net/if.h>
>  #include <sys/ioctl.h>
>  #include <sys/socket.h>
>  #include <sys/types.h>
> @@ -252,10 +253,11 @@ int main(int argc, char **argv)
>                     buf[buflen] = ((buflen % 0xFF) + 1) & 0xFF;
>      }
> 
> 
>      retval = write(s, buf, buflen);
> +    printf("retval %d errno %d\n", retval, errno);

Note: in user space errno is only valid if retval is "-1"...

>      if (retval < 0) {
>             perror("write");
>             return retval;
>      }
> 
> 
> $ date +%S.%N && ./isotpsend vcan0 -s 123 -d 321 -D 44 -b && date +%S.%N
> 43.269173590
> retval 44 errno 0
> 44.271162277
> 
> So it waits for the timeout as required by the '-b' option - but the errno
> is not set :-/

I'm not sure what we have to return in the kernel...

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Oliver Hartkopp]

On 28.04.21 12:47, Marc Kleine-Budde wrote:
> On 28.04.2021 12:24:32, Oliver Hartkopp wrote:
>>> Maybe the way to trigger the sk_error_report(sk) we might return '-1'
>>> while the error is then propagated inside errno.
>>
>> I added some debug print in isotpsend:
>>
>> diff --git a/isotpsend.c b/isotpsend.c
>> index 3ea574c..c2937fa 100644
>> --- a/isotpsend.c
>> +++ b/isotpsend.c
>> @@ -45,10 +45,11 @@
>>   #include <libgen.h>
>>   #include <stdio.h>
>>   #include <stdlib.h>
>>   #include <string.h>
>>   #include <unistd.h>
>> +#include <errno.h>
>>
>>   #include <net/if.h>
>>   #include <sys/ioctl.h>
>>   #include <sys/socket.h>
>>   #include <sys/types.h>
>> @@ -252,10 +253,11 @@ int main(int argc, char **argv)
>>                      buf[buflen] = ((buflen % 0xFF) + 1) & 0xFF;
>>       }
>>
>>
>>       retval = write(s, buf, buflen);
>> +    printf("retval %d errno %d\n", retval, errno);
> 
> Note: in user space errno is only valid if retval is "-1"...

Ok, just returned '-1' in that case of blocking write (which runs into 
the timeout) ...

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 9f94ad3caee9..d5d541b4fed5 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -952,10 +952,11 @@ static int isotp_sendmsg(struct socket *sock, 
struct msghdr *msg, size_t size)
         }

         if (wait_tx_done) {
                 /* wait for complete transmission of current pdu */
                 wait_event_interruptible(so->wait, so->tx.state == 
ISOTP_IDLE);
+               return -1;
         }

         return size;
  }

And now got this:

$ date +%S.%N && ./isotpsend vcan0 -s 123 -d 321 -D 44 -b && date +%S.%N
22.411570468
retval -1 errno 1
write: Operation not permitted

So still nothing to see from "sk->sk_err = ECOMM;"

But when adding 'return -ECOMM' at the above section it works like this:

$ date +%S.%N && ./isotpsend vcan0 -s 123 -d 321 -D 44 -b && date +%S.%N
58.453452222
retval -1 errno 70
write: Communication error on send

This is obviously the expected behaviour for user space:

- get '-1' in the case of socket related errors
- get the details from errno

Maybe all that sk_err stuff is only relevant for listen/accept 
constructions for connection oriented sockets and can be removed inside 
isotp.c ...

Regards,
Oliver

> 
>>       if (retval < 0) {
>>              perror("write");
>>              return retval;
>>       }
>>
>>
>> $ date +%S.%N && ./isotpsend vcan0 -s 123 -d 321 -D 44 -b && date +%S.%N
>> 43.269173590
>> retval 44 errno 0
>> 44.271162277
>>
>> So it waits for the timeout as required by the '-b' option - but the errno
>> is not set :-/
> 
> I'm not sure what we have to return in the kernel...
> 
> Marc
> 

Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3601 bytes --]

On 28.04.2021 13:45:10, Oliver Hartkopp wrote:
> On 28.04.21 12:47, Marc Kleine-Budde wrote:
> > On 28.04.2021 12:24:32, Oliver Hartkopp wrote:
> > > > Maybe the way to trigger the sk_error_report(sk) we might return '-1'
> > > > while the error is then propagated inside errno.
> > > 
> > > I added some debug print in isotpsend:
> > > 
> > > diff --git a/isotpsend.c b/isotpsend.c
> > > index 3ea574c..c2937fa 100644
> > > --- a/isotpsend.c
> > > +++ b/isotpsend.c
> > > @@ -45,10 +45,11 @@
> > >   #include <libgen.h>
> > >   #include <stdio.h>
> > >   #include <stdlib.h>
> > >   #include <string.h>
> > >   #include <unistd.h>
> > > +#include <errno.h>
> > > 
> > >   #include <net/if.h>
> > >   #include <sys/ioctl.h>
> > >   #include <sys/socket.h>
> > >   #include <sys/types.h>
> > > @@ -252,10 +253,11 @@ int main(int argc, char **argv)
> > >                      buf[buflen] = ((buflen % 0xFF) + 1) & 0xFF;
> > >       }
> > > 
> > > 
> > >       retval = write(s, buf, buflen);
> > > +    printf("retval %d errno %d\n", retval, errno);
> > 
> > Note: in user space errno is only valid if retval is "-1"...
> 
> Ok, just returned '-1' in that case of blocking write (which runs into the
> timeout) ...
> 
> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 9f94ad3caee9..d5d541b4fed5 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -952,10 +952,11 @@ static int isotp_sendmsg(struct socket *sock, struct
> msghdr *msg, size_t size)
>         }
> 
>         if (wait_tx_done) {
>                 /* wait for complete transmission of current pdu */
>                 wait_event_interruptible(so->wait, so->tx.state ==
> ISOTP_IDLE);
> +               return -1;
>         }
> 
>         return size;
>  }
> 
> And now got this:
> 
> $ date +%S.%N && ./isotpsend vcan0 -s 123 -d 321 -D 44 -b && date +%S.%N
> 22.411570468
> retval -1 errno 1
         ^        ^
         |     that is your -1
      it's -1, as your return value was negative
      
> write: Operation not permitted
> 
> So still nothing to see from "sk->sk_err = ECOMM;"
> 
> But when adding 'return -ECOMM' at the above section it works like this:
> 
> $ date +%S.%N && ./isotpsend vcan0 -s 123 -d 321 -D 44 -b && date +%S.%N
> 58.453452222
> retval -1 errno 70
> write: Communication error on send

looks good

> This is obviously the expected behaviour for user space:
> 
> - get '-1' in the case of socket related errors
> - get the details from errno

ACK

> Maybe all that sk_err stuff is only relevant for listen/accept constructions
> for connection oriented sockets and can be removed inside isotp.c ...

My theory is:
- return bytes send from isotp_sendmsg() or -errno as usual
- use sk->sk_error_report(sk) to wake up user space if it is blocked in
  poll() or blocking in sendmsg()/recvmsg()

For example, if you have a normal read() blocking on a CAN_RAW socket on
an interface, you get a ENETDOWN when you ifdown the interface:

https://elixir.bootlin.com/linux/latest/source/net/can/raw.c#L302

e.g.:

| ➜ (pts/54) frogger@hardanger:/tmp candump can0 &
| [1] 260474
| ➜ (pts/54) frogger@hardanger:/tmp (1) sudo ip link set can0 down
| read: Network is down
| [1]  + 260474 exit 1     candump can0

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]