hm2_eth: read / write buffer checks

[hdiethelm]

This generated a segfault when eth_socket_send() fails due to
accumulating data in the buffer.

Issue found while working on #4199 but this doesn't really fit that PR.

[hdiethelm]

^Rebased to master

[hdiethelm]

@BsAtHome ping

[BsAtHome]

Do see the problem. But it also makes me question things a bit.
Why does the data need to be dropped? Is that about timing? Dropping a write command may be much more serious than dropping a read (well, that is round-trip packet write->packet read).
Secondly, what is the maximum packet size? Does local or remote not support (8..16kB) jumbo frames or is that not supposed to happen?

[BsAtHome]

BTW, if I'm barking up the wrong tree, just let me know and try to make me understand the actual story.

[hdiethelm]

As much as I understood from the code, the enqueue queues commands. send_queue finalizes the package and sends it. If this finalizing fails, there is no other way than drop all.

All four size checks should never trigger except something else is wrong. In that case, better show an error and try recover than segfault and gone.

This PR only avoids segfaults. If the size check fails, this would need fixing somewhere else.

In my case, eth_socket_send() had a bug for Xenomai4, so it returned -1. In the next cycle, the data to be sent was 2x the size until segfault. Took me some time to figure out that the reason rtapi_app crashed was here.

An additional barrier would be to drop the data if send failed. But that might have side effects. So I opted to only add checks.

In my case, the packages are ~150 bytes, far away from the max. size.

[BsAtHome]

As much as I understood from the code, the enqueue queues commands. send_queue finalizes the package and sends it. If this finalizing fails, there is no other way than drop all.

Yes, it combines the queued reads/writes for sending in one packet.

All four size checks should never trigger except something else is wrong. In that case, better show an error and try recover than segfault and gone.
This PR only avoids segfaults. If the size check fails, this would need fixing somewhere else.

Well, segfaulting is never the right answer... So, yes, preventing it is good.

Your "fix" does seem to be correct in that it drops the command(s). Also the read command resolution queue is cleared properly on fail.

In my case, eth_socket_send() had a bug for Xenomai4, so it returned -1. In the next cycle, the data to be sent was 2x the size until segfault. Took me some time to figure out that the reason rtapi_app crashed was here.

There may also be a segfault in the read resolution queue. Is saves the address of the buffer(s) where the read data needs to be copied into. If you double up, then that queue is probably going to contain the same address for resolution, which is definitely wrong. So, dropping the queue is the correct action.

An additional barrier would be to drop the data if send failed. But that might have side effects. So I opted to only add checks.

Well, you actually do drop the read queue. So, that data is dropped. The write queue, well, that is a different question. You might want to have a look at the code that coalesces the write-queue and actually sends.

In my case, the packages are ~150 bytes, far away from the max. size.

Yeah, should never be a problem.

[hdiethelm]

You are right, why not improve the code around these sections a bit.

Changes:

• function hm2_eth_reset_queued_reads() added instead of copy&paste
• also check board->queue_reads_count
• less magic numbers
• static_assert for sizeof(read_cnt) / sizeof(write_cnt)
• use memcpy for read_cnt: no guarantee of alignment / same is already in for write_cnt
• write_cnt++ after size check (was a bug from my side)
• remove write_packet_size (was redundant)

Tested again. By dropping packet 10000...10020 in eth_socket_send(), i can trigger 2 of 5 error cases:
hm2_eth: ERROR: enqueue_write: buffer full
hm2_eth: ERROR: send_queued_writes: buffer full, dropping all data

With setp hm2_[HOSTMOT2](BOARD).0.packet-error-limit 1000 it recovers cleanly and the machine continues working.

max packet size = 340 / max queue_reads_count = 10 on my setup.
This will be different for different configs. At leas now, there is an error message instead of a crash if somebody manages to exceed 1400 bytes.

[hdiethelm]

In my case, eth_socket_send() had a bug for Xenomai4, so it returned -1. In the next cycle, the data to be sent was 2x the size until segfault. Took me some time to figure out that the reason rtapi_app crashed was here.

There may also be a segfault in the read resolution queue. Is saves the address of the buffer(s) where the read data needs to be copied into. If you double up, then that queue is probably going to contain the same address for resolution, which is definitely wrong. So, dropping the queue is the correct action.

An additional barrier would be to drop the data if send failed. But that might have side effects. So I opted to only add checks.

Well, you actually do drop the read queue. So, that data is dropped. The write queue, well, that is a different question. You might want to have a look at the code that coalesces the write-queue and actually sends.

In hm2_eth_receive_queued_reads(), the read queue is reset when the read fails, looks good. Probably the reason I can not trigger a read overflow.

However, in hm2_eth_send_queued_writes(), the queue is not reset if eth_socket_send() fails. This is probably the initial reason for the buffer overflow. This would fix it. I have to test if it works.

diff --git a/src/hal/drivers/mesa-hostmot2/hm2_eth.c b/src/hal/drivers/mesa-hostmot2/hm2_eth.c
index 308e442100..2ea22245dc 100644
--- a/src/hal/drivers/mesa-hostmot2/hm2_eth.c
+++ b/src/hal/drivers/mesa-hostmot2/hm2_eth.c
@@ -1232,13 +1232,13 @@ static int hm2_eth_send_queued_writes(hm2_lowlevel_io_t *this) {
     
     t0 = rtapi_get_time();
     send = eth_socket_send(board->sockfd, (void*) &board->write_packet, board->write_packet_ptr - board->write_packet, 0);
+    t1 = rtapi_get_time();
+    LL_PRINT_IF(debug, "enqueue_write(%d) : PACKET SEND [SIZE: %d | TIME: %llu]\n", board->write_cnt, send, t1 - t0);
+    board->write_packet_ptr = board->write_packet;
     if(send < 0) {
         LL_PRINT("ERROR: sending packet: %s\n", strerror(errno));
         return 0;
     }
-    t1 = rtapi_get_time();
-    LL_PRINT_IF(debug, "enqueue_write(%d) : PACKET SEND [SIZE: %d | TIME: %llu]\n", board->write_cnt, send, t1 - t0);
-    board->write_packet_ptr = board->write_packet;
     return 1;
 }

Hmm, I found an other funny looking line:
https://github.com/LinuxCNC/linuxcnc/blame/3e8a44f30acae0cff1eff158f96ebcc6a6ad6220/src/hal/drivers/mesa-hostmot2/hostmot2.c#L1628
No big deal due to hm2_finish_read() will fail anyway when hm2_queue_read() fails...

In my case, the packages are ~150 bytes, far away from the max. size.

Yeah, should never be a problem.

[hdiethelm]

So, the last commit fixes root cause of the growing write buffer and the missing return value check.

Tested: Now there is no buffer full error any more. The size checks make still sense but they are not triggered any more.

[BsAtHome]

The "funny" line is a real bug :-)

[hdiethelm]

The "funny" line is a real bug :-)

Just one that will never creates any issues. Fixed any way.

By:

diff --git a/src/hal/drivers/mesa-hostmot2/hm2_eth.c b/src/hal/drivers/mesa-hostmot2/hm2_eth.c
index 56c4c126de..bce9fc7dda 100644
--- a/src/hal/drivers/mesa-hostmot2/hm2_eth.c
+++ b/src/hal/drivers/mesa-hostmot2/hm2_eth.c
@@ -1114,7 +1114,7 @@ do_recv_packet:
         i++;
     } while (recv != board->queue_buff_size && t2 < read_deadline);
     if(recv != board->queue_buff_size) {
-        hm2_eth_reset_queued_reads(board);
+        //hm2_eth_reset_queued_reads(board);
         if(!record_soft_error(board)) return 0;
         return -EAGAIN;
     }

I was also able to trigger:

hm2_eth: ERROR: enqueue_read: queue_reads full
hm2/hm2_7i96s.0: TRAM read error! (addr=0x2100, size=16, iter=4287)
hm2_eth: ERROR: send_queued_reads: queue_reads full, dropping all data

Recovered nicely.