IP fragmentation send path: added TCP/UDP packet splitting to fit within MTU #2380

robertc2000 · 2023-09-08T12:07:57Z

No description provided.

scaprile

The approach seems to look fine for TCP.
However, for UDP, it will only work if the user calls mg_print*() functions that call mg_io_send(). Otherwise, calls to mg_send() will bypass your strategy and be sent without being fragmented. Please write a test that proves or refutes my point and, if I'm right, then solve it.

mongoose/src/net_builtin.c

Lines 1075 to 1088 in d23664a

    
           bool mg_send(struct mg_connection *c, const void *buf, size_t len) { 
        
             struct mg_tcpip_if *ifp = (struct mg_tcpip_if *) c->mgr->priv; 
        
             bool res = false; 
        
             uint32_t rem_ip; 
        
             memcpy(&rem_ip, c->rem.ip, sizeof(uint32_t)); 
        
             if (ifp->ip == 0 || ifp->state != MG_TCPIP_STATE_READY) { 
        
               mg_error(c, "net down"); 
        
             } else if (c->is_udp) { 
        
               struct connstate *s = (struct connstate *) (c + 1); 
        
               tx_udp(ifp, s->mac, ifp->ip, c->loc.port, rem_ip, c->rem.port, buf, len); 
        
               res = true; 
        
             } else { 
        
               res = mg_iobuf_add(&c->send, c->send.len, buf, len); 
        
             }

In case we would need to mg_io_add for UDP, I wouldn't want to delay UDP packets that would fit the MTU, so we should not just buffer, as buffered datagrams would be sent on next poll, not now. For UDP, whatever fits the MTU we should try to send it now. (*)
If I'm right, this fragmentation stuff will have the side effect that we'll now be able to "send UDP datagrams larger than an IP datagram", which will cause them to the splitted into several datagrams and that IMO wouldn't be correct. Please check that, too.

(*): How would this interact with an ARP lookup ? Now our first UDP that requires an ARP lookup is lost, can we make use of this in advance and save the datagram for later ? (If the ISO police OSI enforcement unit comes for you, just tell them I told you to do it)

And now that I think of it again... We are not actually sending fragmented IP, are we ? So if a user requests a 1500-byte UDP datagram and our MTU is 750, we'll actually send two UDP datagrams breaking the very logic foundation of UDP datagrams. I think we should either reject this (force the users to break their messages) and (or at least) log it. Otherwise, we should send IP fragments but we will not receive our own data and that looks bad...
Our current logic is to follow down the path and the Ethernet layer will drop too-long packets.
I think this is turning into a long rambling, if that is the case, let's please follow up at the chat.

src/net_builtin.c

cpq · 2023-09-08T14:52:42Z

src/net.h

@@ -54,6 +54,8 @@ struct mg_connection {
  void *pfn_data;              // Protocol-specific function parameter
  char data[MG_DATA_SIZE];     // Arbitrary connection data
  void *tls;                   // TLS specific data
+#define MTU_DEFAULT_VALUE 1500
+  uint16_t mtu;                // MTU for this connection


I think this should be moved to the tcp/ip stack implementation. Specifically, to the struct connstate -

mongoose/src/net_builtin.c

Lines 16 to 17 in d23664a

struct connstate {

uint32_t seq, ack; // TCP seq/ack counters

. The MTU_DEFAULT_VALUE I'd rename to MG_DEFAULT_MTU, to keep our defines prefixed with MG_ in order to avoid conflicts with customer code.

Yeah, I second, missed that, that is ifp-> stuff not c-> ...

cpq · 2023-09-08T14:56:34Z

src/net_builtin.c

+  if (len + max_headers_len > ifp->tx.len) {
+    len = ifp->tx.len - max_headers_len;
+  }
+  if (len + max_headers_len - eth_h_len > c->mtu) {


Also, mtu must not be too small - it should be bigger that the size of headers. Shall we check that mtu is not less than a minimum possible here, or somewhere else?

cpq

Please move mtu setting to the struct connstate

scaprile

Please address those nitty comments of mine.

scaprile · 2023-09-11T12:41:52Z

src/net_builtin.c

+    size_t max_udp_header_len = 32;
+    if (s->mtu < max_udp_header_len) {
+      s->mtu = MIP_MTU_DEFAULT;
+    }


See above (RFC-791)

scaprile · 2023-09-11T12:53:51Z

src/net_builtin.c

+  if (s->mtu < max_headers_len - eth_h_len) {
+    s->mtu = MIP_MTU_DEFAULT;
+  }


This doesn't seem correct. The user is trying to lower the MTU and we go full throttle to 1500.
I think we should limit to the smallest allowed, IIUC it is 68 bytes per RFC-791: "Every internet module must be able to forward a datagram of 68 octets without further fragmentation"
However, for TCP, we should perhaps limit to the max header value.

scaprile · 2023-09-11T13:00:11Z

src/net_builtin.c

+  if (len + max_headers_len - eth_h_len > s->mtu) {
+    len = s->mtu - max_headers_len + eth_h_len;
+    if (c->is_udp)
+      MG_ERROR(("UDP datagram exceeds MTU"));


Please tell the datagram has been truncated, something like
"Truncated UDP datagram exceeding MTU"

scaprile · 2023-09-11T13:01:09Z

src/net_builtin.c

+    }
+    if (len + max_udp_header_len > s->mtu) {
+      len = s->mtu - max_udp_header_len;
+      MG_ERROR(("UDP datagram exceeds MTU"));


same comment as above (truncation)

scaprile

Changes bring more changes...

scaprile · 2023-09-11T15:27:43Z

src/net_builtin.c

    struct connstate *s = (struct connstate *) (c + 1);
+    size_t min_mtu = 68 /* RFC-791 */;
+    if (ifp->mtu < min_mtu) {
+    MG_ERROR(("MTU is lower than minimum possible value. Setting it to %d.", min_mtu));
+    ifp->mtu = (uint16_t) min_mtu;
+  }
+    if (len + max_udp_header_len > ifp->mtu) {
+      len = ifp->mtu - max_udp_header_len;
+      MG_ERROR(("UDP datagram exceeds MTU. Truncating it."));
+    }
    tx_udp(ifp, s->mac, ifp->ip, c->loc.port, rem_ip, c->rem.port, buf, len);


Now that the MTU is checked against ifp->mtu I think it makes sense moving the UDP case into tx_udp(). This also avoids duplicated code and texts.

scaprile · 2023-09-11T15:30:08Z

src/net_builtin.c

+  size_t eth_h_len = 14, ip_max_h_len = 24, tcp_max_h_len = 60, udp_h_len = 8;
+  size_t max_headers_len = eth_h_len + ip_max_h_len +
+                          (c->is_udp ? udp_h_len : tcp_max_h_len);
+  size_t min_mtu = c->is_udp ? 68 /* RFC-791 */ : max_headers_len - eth_h_len;
  memcpy(&rem_ip, c->rem.ip, sizeof(uint32_t));
+  if (len + max_headers_len > ifp->tx.len) {
+    len = ifp->tx.len - max_headers_len;
+  }
+  if (ifp->mtu < min_mtu) {
+    MG_ERROR(("MTU is lower than minimum possible value. Setting it to %d.", min_mtu));
+    ifp->mtu = (uint16_t) min_mtu;
+  }
+  if (len + max_headers_len - eth_h_len > ifp->mtu) {


Same comment (mg_send())

robertc2000 requested review from cpq and scaprile September 8, 2023 12:07

scaprile requested changes Sep 8, 2023

View reviewed changes

src/net_builtin.c Outdated Show resolved Hide resolved

robertc2000 force-pushed the ip-fragmentation-2 branch from 9506aea to 608ef14 Compare September 8, 2023 13:39

scaprile reviewed Sep 8, 2023

View reviewed changes

src/net_builtin.c Outdated Show resolved Hide resolved

cpq reviewed Sep 8, 2023

View reviewed changes

cpq requested changes Sep 8, 2023

View reviewed changes

robertc2000 force-pushed the ip-fragmentation-2 branch 2 times, most recently from 08bbd07 to 6410d85 Compare September 11, 2023 11:10

cpq approved these changes Sep 11, 2023

View reviewed changes

scaprile approved these changes Sep 11, 2023

View reviewed changes

robertc2000 force-pushed the ip-fragmentation-2 branch from 6410d85 to 220c524 Compare September 11, 2023 15:03

scaprile reviewed Sep 11, 2023

View reviewed changes

cpq approved these changes Sep 11, 2023

View reviewed changes

added TCP/UDP packet splitting to fit within MTU

264026a

robertc2000 force-pushed the ip-fragmentation-2 branch from 220c524 to 264026a Compare September 12, 2023 11:05

cpq merged commit 53baa18 into master Sep 12, 2023
72 of 73 checks passed

cpq deleted the ip-fragmentation-2 branch September 12, 2023 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IP fragmentation send path: added TCP/UDP packet splitting to fit within MTU #2380

IP fragmentation send path: added TCP/UDP packet splitting to fit within MTU #2380

robertc2000 commented Sep 8, 2023

scaprile left a comment •

edited

Loading

cpq Sep 8, 2023

scaprile Sep 8, 2023

cpq Sep 8, 2023

cpq left a comment

scaprile left a comment

scaprile Sep 11, 2023

scaprile Sep 11, 2023

scaprile Sep 11, 2023

scaprile Sep 11, 2023

scaprile left a comment

scaprile Sep 11, 2023

scaprile Sep 11, 2023

	bool mg_send(struct mg_connection c, const void buf, size_t len) {
	struct mg_tcpip_if ifp = (struct mg_tcpip_if ) c->mgr->priv;
	bool res = false;
	uint32_t rem_ip;
	memcpy(&rem_ip, c->rem.ip, sizeof(uint32_t));
	if (ifp->ip == 0 \|\| ifp->state != MG_TCPIP_STATE_READY) {
	mg_error(c, "net down");
	} else if (c->is_udp) {
	struct connstate s = (struct connstate ) (c + 1);
	tx_udp(ifp, s->mac, ifp->ip, c->loc.port, rem_ip, c->rem.port, buf, len);
	res = true;
	} else {
	res = mg_iobuf_add(&c->send, c->send.len, buf, len);
	}

	struct connstate {
	uint32_t seq, ack; // TCP seq/ack counters

IP fragmentation send path: added TCP/UDP packet splitting to fit within MTU #2380

IP fragmentation send path: added TCP/UDP packet splitting to fit within MTU #2380

Conversation

robertc2000 commented Sep 8, 2023

scaprile left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpq left a comment

Choose a reason for hiding this comment

scaprile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scaprile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scaprile left a comment •

edited

Loading