reading from socket can

reading from socket can

[Michael Dressel]

Hi,

is it guaranteed that a read on a SOCK_RAW  socket will always
return complete can_frames, if it returns data at all?

The documentation of "read" explicitly states, that it is not an error to
return with any number of bytes less than the "requested" number of
bytes.

I found calls to read in some applications taking it for granted
that read always returns full can_frames. And, having some very
rare issues not understood yet, I wonder if an error in reading
from the socket may cause this issues.

Actually I found a similar bug in a library for ModBus communication,
and there it was possible to verify that the issues came from using read
in a wrong way.

Cheers,
Michael

Re: reading from socket can

[Oliver Hartkopp]

Hi Michael,

On 07/27/2017 03:40 PM, Michael Dressel wrote:

> is it guaranteed that a read on a SOCK_RAW  socket will always
> return complete can_frames, if it returns data at all?

Yes.

I double checked the code in net/can/raw.c which uses 
sock_queue_rcv_skb() in net/core/sock.c which completely fails when it's 
not possible to to place the skb content (the CAN frame) in the socket 
receive buffer.

In this case the dropcount counter is increased which can be monitored 
by recvmsg() (see dropcount example in candump.c)

> The documentation of "read" explicitly states, that it is not an error to
> return with any number of bytes less than the "requested" number of
> bytes.

Yes that's right for read() in general.

At least it makes sense to check whether the return value is 
sizeof(struct can_frame) or sizeof(struct canfd_frame) or if some error 
occured.

> I found calls to read in some applications taking it for granted
> that read always returns full can_frames.

You also might get negative values for error codes, e.g. when the (USB) 
CAN interface disappeared! So assuming to *always* get a CAN frame and 
not checking for errors is bad.

> And, having some very
> rare issues not understood yet, I wonder if an error in reading
> from the socket may cause this issues.

Don't know. Can you post some code?

> Actually I found a similar bug in a library for ModBus communication,
> and there it was possible to verify that the issues came from using read
> in a wrong way.

Dito.

Best,
Oliver

Re: reading from socket can

[Michael Dressel]

Hi Oliver,

thank you very much for the quick reply.


On Thu, 27 Jul 2017, Oliver Hartkopp wrote:

>>  is it guaranteed that a read on a SOCK_RAW  socket will always
>>  return complete can_frames, if it returns data at all?
>
> Yes.
>
> I double checked the code in net/can/raw.c which uses sock_queue_rcv_skb() in 
> net/core/sock.c which completely fails when it's not possible to to place the 
> skb content (the CAN frame) in the socket receive buffer.
>
> In this case the dropcount counter is increased which can be monitored by 
> recvmsg() (see dropcount example in candump.c)

I'm not familiar enough with drivers to really understand that. But is
the read an atomic action? Even if it returns bytes from an completely
filled buffer. Could this read action be interrupted?

Well, anyway I guess I will introduce some messaging to see if I run in
that problem at all.

>>  And, having some very
>>  rare issues not understood yet, I wonder if an error in reading
>>  from the socket may cause this issues.
>
> Don't know. Can you post some code?
>

Here is just some code, that is similar to what happens in the
driver we use:

nread = read(can_fd, &rx[0], sizeof(struct can_frame) * 10);
     if (nread > 0) {
         int i;
         nread = nread / sizeof(struct can_frame);
             for (i = 0; i < nread; i++) {

I was not able to provoke my issues in a controlled test environment.

And in the productive environment it occurs only about every 3 to 5 weeks.

Cheers,
Michael

Re: reading from socket can

[Oliver Hartkopp]

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

Hi Michael,

On 07/28/2017 09:01 AM, Michael Dressel wrote:

> Here is just some code, that is similar to what happens in the
> driver we use:
> 
> nread = read(can_fd, &rx[0], sizeof(struct can_frame) * 10);
>      if (nread > 0) {
>          int i;

If you wanted to check whether only 'correct & complete' struct 
can_frames have been read from the socket at least this should be done:

	if (nread % sizeof(struct can_frame))
		exit(1); // or something like this ;-)

 >          nread = nread / sizeof(struct can_frame);
>              for (i = 0; i < nread; i++) {
> 

I was really unsure if

	nread = read(can_fd, &rx[0], sizeof(struct can_frame) * 10);

makes sense at all - and nread would ever be greater than 1 ?!?

I modified https://github.com/linux-can/can-tests/blob/master/tst-raw.c 
to get a similar setup (diff for tst-raw.c is attached):

while (1) {
    if ((nbytes = read(s, &frame[0], 10*sizeof(struct can_frame))) < 0) {
         perror("read");
         if (!ignore_errors)
              return 1;
    } else if (nbytes % sizeof(struct can_frame)) {
         fprintf(stderr, "read: incomplete CAN frame\n");
         return 1;
    } else {
         for (k=0; k < nbytes / sizeof(struct can_frame); k++) {
              printf("<%d> ", k);

              if (k) exit(1); // see remark below

              if (frame[0].can_id & CAN_EFF_FLAG)
                     printf("%8X  ", frame[0].can_id & CAN_EFF_MASK);

(..)

I tested this patch an NEVER got more than ONE single CAN frame per 
read(). Then I added the brutal

	if (k) exit(1);

check to not miss any attempt of more than one CAN frame - but it did 
not occur ...

So I think your provided code does not have the functionality the 
programmer intended.

If you want to make sure that no frame was dropped in the socket's 
receive queue take a look at this:

http://marc.info/?l=linux-can&m=147929028915128&w=2

Additionally you might increase the RX buffer size of the socket with 
the SO_RCVBUF socket option:

https://github.com/linux-can/can-utils/blob/master/candump.c#L525

There are some examples how to use the Linux specific 'multiple read' 
system calls:

http://rtime.felk.cvut.cz/can/
http://rtime.felk.cvut.cz/can/can-syscalls.pdf

But finally I would just use recvmsg() and check for the dropcount value 
to check whether you have a problem at all.

Best,
Oliver


> I was not able to provoke my issues in a controlled test environment.
> 
> And in the productive environment it occurs only about every 3 to 5 weeks.
> 
> Cheers,
> Michael

[-- Attachment #2: multiread.diff --]
[-- Type: text/x-patch, Size: 2096 bytes --]

diff --git a/tst-raw.c b/tst-raw.c
index 1e0284b..0e5c2a9 100644
--- a/tst-raw.c
+++ b/tst-raw.c
@@ -59,8 +59,8 @@ int main(int argc, char **argv)
 	int s;
 	struct sockaddr_can addr;
 	struct can_filter rfilter[4];
-	struct can_frame frame;
-	int nbytes, i;
+	struct can_frame frame[10];
+	int nbytes, i, k;
 	struct ifreq ifr;
 	char *ifname = "vcan2";
 	int ifindex;
@@ -142,38 +142,44 @@ int main(int argc, char **argv)
 
 	if(send_one_frame) {
 
-		frame.can_id  = 0x123;
-		frame.can_dlc = 2;
-		frame.data[0] = 0x11;
-		frame.data[1] = 0x22;
+		frame[0].can_id  = 0x123;
+		frame[0].can_dlc = 2;
+		frame[0].data[0] = 0x11;
+		frame[0].data[1] = 0x22;
 
-		nbytes = write(s, &frame, sizeof(struct can_frame));
+		nbytes = write(s, &frame[0], sizeof(struct can_frame));
 	}
 
 	while (1) {
 
-		if ((nbytes = read(s, &frame, sizeof(struct can_frame))) < 0) {
+		if ((nbytes = read(s, &frame[0], 10*sizeof(struct can_frame))) < 0) {
 			perror("read");
 			if (!ignore_errors)
 				return 1;
-		} else if (nbytes < sizeof(struct can_frame)) {
+		} else if (nbytes % sizeof(struct can_frame)) {
 			fprintf(stderr, "read: incomplete CAN frame\n");
 			return 1;
 		} else {
-			if (frame.can_id & CAN_EFF_FLAG)
-				printf("%8X  ", frame.can_id & CAN_EFF_MASK);
-			else
-				printf("%3X  ", frame.can_id & CAN_SFF_MASK);
+			for (k=0; k < nbytes / sizeof(struct can_frame); k++) {
+				printf("<%d> ", k);
+
+				if (k) exit(1);
+
+				if (frame[0].can_id & CAN_EFF_FLAG)
+					printf("%8X  ", frame[0].can_id & CAN_EFF_MASK);
+				else
+					printf("%3X  ", frame[0].can_id & CAN_SFF_MASK);
 	    
-			printf("[%d] ", frame.can_dlc);
+				printf("[%d] ", frame[0].can_dlc);
 	    
-			for (i = 0; i < frame.can_dlc; i++) {
-				printf("%02X ", frame.data[i]);
+				for (i = 0; i < frame[0].can_dlc; i++) {
+					printf("%02X ", frame[0].data[i]);
+				}
+				if (frame[0].can_id & CAN_RTR_FLAG)
+					printf("remote request");
+				printf("\n");
+				fflush(stdout);
 			}
-			if (frame.can_id & CAN_RTR_FLAG)
-				printf("remote request");
-			printf("\n");
-			fflush(stdout);
 		}
 	}
 

Re: reading from socket can

[Kurt Van Dijck]

> I was really unsure if
> 
> 	nread = read(can_fd, &rx[0], sizeof(struct can_frame) * 10);
> 
> makes sense at all - and nread would ever be greater than 1 ?!?

recvmmsg() is what you need to read up to 10 CAN frames at once.

Kurt

Re: reading from socket can

[Michael Dressel]

Hi,

thanks again for your reply.

On Fri, 28 Jul 2017, Oliver Hartkopp wrote:

>>  nread = read(can_fd, &rx[0], sizeof(struct can_frame) * 10);
>>       if (nread > 0) {
>>           int i;
>
> If you wanted to check whether only 'correct & complete' struct can_frames 
> have been read from the socket at least this should be done:

That is not code written by us. I don't know why it was done like that.
We are only using that software.

For clarifying if my problem is related to the read at all I did something
similar to your suggestion and added printing a message in case incomplete
can_frames are read.

The difficulty is that my problem takes some weeks to occur and I have
not been able to set up a test case showing the problem.

It could very well be completely unrelated to the read issue.

Thanks for your help. I will post my result if I find one.

Cheers,
Michael

Re: reading from socket can

[Michael Dressel]

Hi,

thanks for the information.

On Fri, 28 Jul 2017, Kurt Van Dijck wrote:

> recvmmsg() is what you need to read up to 10 CAN frames at once.

As stated in my previous message, I'm not sure what the authors aimed
for with that code.

My initial suspicion was that read() could even return with a number
of bytes that is not a multiple of can_frame size.

Cheers,
Michael

Re: reading from socket can

[Michael Dressel]

Hi,

I tried provoking an error by calling read() with less
than sizeof(struct can_frame) bytes intentionally. The
intention was to corrupt the data this way.

This I did after reading the first 5000 frames properly and than every 300
frames.

I expected to get out of synch after corrupting the reading.

When I read only two bytes no interpretation is done because
two bytes are not a multiple of sizeof(struct can_frame).

During the next read I expected the read to put the remaining
bytes (not red by the previous call to read) in the beginning
of the buffer handed into read. Now the data should be corrupted.

But that is not the case.

The next frame read already seams to be correct.

My question here is if already the socket can driver prevents
corrupting data this way?


Cheers,
Michael



On Fri, 28 Jul 2017, Oliver Hartkopp wrote:

> Hi Michael,
>
> On 07/28/2017 09:01 AM, Michael Dressel wrote:
>
>>  Here is just some code, that is similar to what happens in the
>>  driver we use:
>>
>>  nread = read(can_fd, &rx[0], sizeof(struct can_frame) * 10);
>>       if (nread > 0) {
>>           int i;
>
> If you wanted to check whether only 'correct & complete' struct can_frames 
> have been read from the socket at least this should be done:
>
> 	 if (nread % sizeof(struct can_frame))
> 		 exit(1); // or something like this ;-)
>
>>           nread = nread / sizeof(struct can_frame);
>>               for (i = 0; i < nread; i++) {
>> 
>
> I was really unsure if
>
> 	nread = read(can_fd, &rx[0], sizeof(struct can_frame) * 10);
>
> makes sense at all - and nread would ever be greater than 1 ?!?
>
> I modified https://github.com/linux-can/can-tests/blob/master/tst-raw.c to 
> get a similar setup (diff for tst-raw.c is attached):
>
> while (1) {
>    if ((nbytes = read(s, &frame[0], 10*sizeof(struct can_frame))) < 0) {
>         perror("read");
>         if (!ignore_errors)
>              return 1;
>    } else if (nbytes % sizeof(struct can_frame)) {
>         fprintf(stderr, "read: incomplete CAN frame\n");
>         return 1;
>    } else {
>         for (k=0; k < nbytes / sizeof(struct can_frame); k++) {
>              printf("<%d> ", k);
>
>             if (k) exit(1); // see remark below
>
>              if (frame[0].can_id & CAN_EFF_FLAG)
>                     printf("%8X  ", frame[0].can_id & CAN_EFF_MASK);
>
> (..)
>
> I tested this patch an NEVER got more than ONE single CAN frame per read(). 
> Then I added the brutal
>
> 	if (k) exit(1);
>
> check to not miss any attempt of more than one CAN frame - but it did not 
> occur ...
>
> So I think your provided code does not have the functionality the programmer 
> intended.
>
> If you want to make sure that no frame was dropped in the socket's receive 
> queue take a look at this:
>
> http://marc.info/?l=linux-can&m=147929028915128&w=2
>
> Additionally you might increase the RX buffer size of the socket with the 
> SO_RCVBUF socket option:
>
> https://github.com/linux-can/can-utils/blob/master/candump.c#L525
>
> There are some examples how to use the Linux specific 'multiple read' system 
> calls:
>
> http://rtime.felk.cvut.cz/can/
> http://rtime.felk.cvut.cz/can/can-syscalls.pdf
>
> But finally I would just use recvmsg() and check for the dropcount value to 
> check whether you have a problem at all.
>
> Best,
> Oliver
>
>
>>  I was not able to provoke my issues in a controlled test environment.
>>
>>  And in the productive environment it occurs only about every 3 to 5 weeks.
>>
>>  Cheers,
>>  Michael
>

Re: reading from socket can

[Oliver Hartkopp]

Hi Michael,

On 07/31/2017 01:33 PM, Michael Dressel wrote:

> I tried provoking an error by calling read() with less
> than sizeof(struct can_frame) bytes intentionally. The
> intention was to corrupt the data this way.
> 
> This I did after reading the first 5000 frames properly and than every 300
> frames.
> 
> I expected to get out of synch after corrupting the reading.
> 
> When I read only two bytes no interpretation is done because
> two bytes are not a multiple of sizeof(struct can_frame).
> 
> During the next read I expected the read to put the remaining
> bytes (not red by the previous call to read) in the beginning
> of the buffer handed into read. Now the data should be corrupted.
> 
> But that is not the case.
> 
> The next frame read already seams to be correct.
> 
> My question here is if already the socket can driver prevents
> corrupting data this way?

The CAN driver puts struct can(fd)_frames into the socket buffer (skb).

That you don't get 'the rest' of the truncated read() is ensured in the 
function raw_recvmsg() in linux/net/can/raw.c.

When the given buffer in the userspace is smaller than the skb->size (== 
sizeof(struct can_frame)) there's only the MSG_TRUNC flag set.

At the end of raw_recvmsg() the skb is free'd and there's no more action 
on this skb. At the next call of raw_recvmsg() you have new skb and a 
new CAN frame, see:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/net/can/raw.c?h=linux-4.12.y#n796

Best regards,
Oliver

Re: reading from socket can

[Michael Dressel]

Hi Oliver,

On Mon, 31 Jul 2017, Oliver Hartkopp wrote:

> The CAN driver puts struct can(fd)_frames into the socket buffer (skb).
>
> That you don't get 'the rest' of the truncated read() is ensured in the 
> function raw_recvmsg() in linux/net/can/raw.c.
>
> When the given buffer in the userspace is smaller than the skb->size (== 
> sizeof(struct can_frame)) there's only the MSG_TRUNC flag set.
>
> At the end of raw_recvmsg() the skb is free'd and there's no more action on 
> this skb. At the next call of raw_recvmsg() you have new skb and a new CAN 
> frame, see:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/net/can/raw.c?h=linux-4.12.y#n796
>
thanks a lot for the explanations. Actually I have no indication for my
original hypothesis that there is something wrong with the way read() is
used. So I have to wait and see. At least I have provided for some more
information to be printed when things go wrong next time.


Cheers,
Michael

Re: reading from socket can

[Michael Dressel]

Hi Oliver,

now after several weeks we still don't exactly know what causes our
issues. But I think we can exclude it has anything to do with
SocketCAN or the can bus.

Just for information:
We see the problems appear in some older computers but not in computers
that are of the same type but have just slightly different versions of
hardware. There must be some small differences in the
computer hardware that, maybe together with some linux driver, causes
our issues.

Cheers,
Michael


On Tue, 1 Aug 2017, Michael Dressel wrote:

> Hi Oliver,
>
> On Mon, 31 Jul 2017, Oliver Hartkopp wrote:
>
>>  The CAN driver puts struct can(fd)_frames into the socket buffer (skb).
>>
>>  That you don't get 'the rest' of the truncated read() is ensured in the
>>  function raw_recvmsg() in linux/net/can/raw.c.
>>
>>  When the given buffer in the userspace is smaller than the skb->size (==
>>  sizeof(struct can_frame)) there's only the MSG_TRUNC flag set.
>>
>>  At the end of raw_recvmsg() the skb is free'd and there's no more action
>>  on this skb. At the next call of raw_recvmsg() you have new skb and a new
>>  CAN frame, see:
>>
>>  https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/net/can/raw.c?h=linux-4.12.y#n796
>> 
> thanks a lot for the explanations. Actually I have no indication for my
> original hypothesis that there is something wrong with the way read() is
> used. So I have to wait and see. At least I have provided for some more
> information to be printed when things go wrong next time.
>
>
> Cheers,
> Michael
>
>

Re: reading from socket can

[Oliver Hartkopp]

On 09/06/2017 10:32 AM, Michael Dressel wrote:
> now after several weeks we still don't exactly know what causes our
> issues. But I think we can exclude it has anything to do with
> SocketCAN or the can bus.

Puh :-)

> Just for information:
> We see the problems appear in some older computers but not in computers
> that are of the same type but have just slightly different versions of
> hardware. There must be some small differences in the
> computer hardware that, maybe together with some linux driver, causes
> our issues.

Do you also have different Linux Kernel versions on these different 
hardwares?

There are continuous improvements and bugfixes in the CAN drivers. So 
maybe an upgrade of the kernel or - at least - to upgrade to the latest 
of the longterm kernels (e.g. 4.4.13 -> 4.4.60) will catch the fix you need.

Best regards,
Oliver

Re: reading from socket can

[Michael Dressel]

On Wed, 6 Sep 2017, Oliver Hartkopp wrote:
> Do you also have different Linux Kernel versions on these different 
> hardwares?

No.

>
> There are continuous improvements and bugfixes in the CAN drivers. So maybe 
> an upgrade of the kernel or - at least - to upgrade to the latest of the 
> longterm kernels (e.g. 4.4.13 -> 4.4.60) will catch the fix you need.

We use 3.16.0-4-686 on the older and the newer computer.

We have replaced some of the effected computers with up to date computers
and run 4.9.0-3-686 on the newest computers. Here we have not seen
issues yet too.

It could really be not particularly related to the CAN cards maybe PCI
controller or ... ?

Thanks for your help.

Cheers,
Michael

Re: reading from socket can

[Oliver Hartkopp]

On 09/06/2017 12:54 PM, Michael Dressel wrote:
> 
> On Wed, 6 Sep 2017, Oliver Hartkopp wrote:
>> Do you also have different Linux Kernel versions on these different 
>> hardwares?
> 
> No.
> 
>>
>> There are continuous improvements and bugfixes in the CAN drivers. So 
>> maybe an upgrade of the kernel or - at least - to upgrade to the 
>> latest of the longterm kernels (e.g. 4.4.13 -> 4.4.60) will catch the 
>> fix you need.
> 
> We use 3.16.0-4-686 on the older and the newer computer.
> 
> We have replaced some of the effected computers with up to date computers
> and run 4.9.0-3-686 on the newest computers. Here we have not seen
> issues yet too.
> 
> It could really be not particularly related to the CAN cards maybe PCI
> controller or ... ?

You might just take a look into the logged changes for each CAN driver, 
e.g. for the FlexCAN here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/net/can/flexcan.c

 From 3.16 to 4.9 there was a bigger amount of changes/fixes.

Maybe you try the new kernel on the old hardware too ;-)

Best regards,
Oliver

Re: reading from socket can

[Michael Dressel]

On Wed, 6 Sep 2017, Oliver Hartkopp wrote:

> On 09/06/2017 12:54 PM, Michael Dressel wrote:
>
> You might just take a look into the logged changes for each CAN driver, e.g. 
> for the FlexCAN here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/drivers/net/can/flexcan.c
>
> From 3.16 to 4.9 there was a bigger amount of changes/fixes.
>
> Maybe you try the new kernel on the old hardware too ;-)

We already did upgrade the old computer. We have it run in a test
environment. With the old linux we could reproduce the problem. Now we
are letting it run with the new operation  system.  Let's see.

Cheers,
Michael