Merge #542 #544

542: More TCP fixes r=Dirbaio a=Dirbaio

See individual commit messages for details.

544: ARP fixes r=Dirbaio a=Dirbaio



Co-authored-by: Dario Nieuwenhuis <dirbaio@dirbaio.net>

src/iface/interface.rs

@@ -952,43 +952,12 @@ impl<'a> InterfaceInner<'a> {
             #[cfg(feature = "proto-ipv4")]
             EthernetProtocol::Ipv4 => {
                 let ipv4_packet = Ipv4Packet::new_checked(eth_frame.payload())?;
-                if eth_frame.src_addr().is_unicast() && ipv4_packet.src_addr().is_unicast() {
-                    // Fill the neighbor cache from IP header of unicast frames.
-                    let ip_addr = IpAddress::Ipv4(ipv4_packet.src_addr());
-                    if self.in_same_network(&ip_addr) {
-                        self.neighbor_cache.as_mut().unwrap().fill(
-                            ip_addr,
-                            eth_frame.src_addr(),
-                            cx.now,
-                        );
-                    }
-                }
-
                 self.process_ipv4(cx, sockets, &ipv4_packet)
                     .map(|o| o.map(EthernetPacket::Ip))
             }
             #[cfg(feature = "proto-ipv6")]
             EthernetProtocol::Ipv6 => {
                 let ipv6_packet = Ipv6Packet::new_checked(eth_frame.payload())?;
-                if eth_frame.src_addr().is_unicast() && ipv6_packet.src_addr().is_unicast() {
-                    // Fill the neighbor cache from IP header of unicast frames.
-                    let ip_addr = IpAddress::Ipv6(ipv6_packet.src_addr());
-                    if self.in_same_network(&ip_addr)
-                        && self
-                            .neighbor_cache
-                            .as_mut()
-                            .unwrap()
-                            .lookup(&ip_addr, cx.now)
-                            .found()
-                    {
-                        self.neighbor_cache.as_mut().unwrap().fill(
-                            ip_addr,
-                            eth_frame.src_addr(),
-                            cx.now,
-                        );
-                    }
-                }
-
                 self.process_ipv6(cx, sockets, &ipv6_packet)
                     .map(|o| o.map(EthernetPacket::Ip))
             }
@@ -1030,9 +999,6 @@ impl<'a> InterfaceInner<'a> {
         let arp_repr = ArpRepr::parse(&arp_packet)?;
 
         match arp_repr {
-            // Respond to ARP requests aimed at us, and fill the ARP cache from all ARP
-            // requests and replies, to minimize the chance that we have to perform
-            // an explicit ARP request.
             ArpRepr::EthernetIpv4 {
                 operation,
                 source_hardware_addr,
@@ -1040,19 +1006,39 @@ impl<'a> InterfaceInner<'a> {
                 target_protocol_addr,
                 ..
             } => {
-                if source_protocol_addr.is_unicast() && source_hardware_addr.is_unicast() {
-                    self.neighbor_cache.as_mut().unwrap().fill(
-                        source_protocol_addr.into(),
-                        source_hardware_addr,
-                        timestamp,
-                    );
-                } else {
-                    // Discard packets with non-unicast source addresses.
-                    net_debug!("non-unicast source address");
+                // Only process ARP packets for us.
+                if !self.has_ip_addr(target_protocol_addr) {
+                    return Ok(None);
+                }
+
+                // Only process REQUEST and RESPONSE.
+                if let ArpOperation::Unknown(_) = operation {
+                    net_debug!("arp: unknown operation code");
                     return Err(Error::Malformed);
                 }
 
-                if operation == ArpOperation::Request && self.has_ip_addr(target_protocol_addr) {
+                // Discard packets with non-unicast source addresses.
+                if !source_protocol_addr.is_unicast() || !source_hardware_addr.is_unicast() {
+                    net_debug!("arp: non-unicast source address");
+                    return Err(Error::Malformed);
+                }
+
+                if !self.in_same_network(&IpAddress::Ipv4(source_protocol_addr)) {
+                    net_debug!("arp: source IP address not in same network as us");
+                    return Err(Error::Malformed);
+                }
+
+                // Fill the ARP cache from any ARP packet aimed at us (both request or response).
+                // We fill from requests too because if someone is requesting our address they
+                // are probably going to talk to us, so we avoid having to request their address
+                // when we later reply to them.
+                self.neighbor_cache.as_mut().unwrap().fill(
+                    source_protocol_addr.into(),
+                    source_hardware_addr,
+                    timestamp,
+                );
+
+                if operation == ArpOperation::Request {
                     Ok(Some(EthernetPacket::Arp(ArpRepr::EthernetIpv4 {
                         operation: ArpOperation::Reply,
                         source_hardware_addr: self.ethernet_addr.unwrap(),
@@ -2885,7 +2871,7 @@ mod test {
             Ok(None)
         );
 
-        // Ensure the address of the requestor was entered in the cache
+        // Ensure the address of the requestor was NOT entered in the cache
         assert_eq!(
             iface.inner.lookup_hardware_addr(
                 &cx,
@@ -2893,7 +2879,7 @@ mod test {
                 &IpAddress::Ipv4(Ipv4Address([0x7f, 0x00, 0x00, 0x01])),
                 &IpAddress::Ipv4(remote_ip_addr)
             ),
-            Ok((remote_hw_addr, MockTxToken))
+            Err(Error::Unaddressable)
         );
     }
 

src/socket/tcp.rs

@@ -382,6 +382,9 @@ pub struct TcpSocket<'a> {
     /// ACK or window updates (ie, no data) won't be sent until expiry.
     ack_delay_timer: AckDelayTimer,
 
+    /// Used for rate-limiting: No more challenge ACKs will be sent until this instant.
+    challenge_ack_timer: Instant,
+
     /// Nagle's Algorithm enabled.
     nagle: bool,
 
@@ -443,6 +446,7 @@ impl<'a> TcpSocket<'a> {
             local_rx_dup_acks: 0,
             ack_delay: Some(ACK_DELAY_DEFAULT),
             ack_delay_timer: AckDelayTimer::Idle,
+            challenge_ack_timer: Instant::from_secs(0),
             nagle: true,
 
             #[cfg(feature = "async")]
@@ -573,7 +577,7 @@ impl<'a> TcpSocket<'a> {
 
     /// Set the keep-alive interval.
     ///
-    /// An idle socket with a keep-alive interval set will transmit a "challenge ACK" packet
+    /// An idle socket with a keep-alive interval set will transmit a "keep-alive ACK" packet
     /// every time it receives no communication during that interval. As a result, three things
     /// may happen:
     ///
@@ -666,6 +670,8 @@ impl<'a> TcpSocket<'a> {
         self.remote_last_ts = None;
         self.ack_delay = Some(ACK_DELAY_DEFAULT);
         self.ack_delay_timer = AckDelayTimer::Idle;
+        self.challenge_ack_timer = Instant::from_secs(0);
+
         self.nagle = true;
 
         #[cfg(feature = "async")]
@@ -1159,7 +1165,7 @@ impl<'a> TcpSocket<'a> {
         // of why we sometimes send an RST and sometimes an RST|ACK
         reply_repr.control = TcpControl::Rst;
         reply_repr.seq_number = repr.ack_number.unwrap_or_default();
-        if repr.control == TcpControl::Syn {
+        if repr.control == TcpControl::Syn && repr.ack_number.is_none() {
             reply_repr.ack_number = Some(repr.seq_number + repr.segment_len());
         }
 
@@ -1223,6 +1229,22 @@ impl<'a> TcpSocket<'a> {
         (ip_reply_repr, reply_repr)
     }
 
+    fn challenge_ack_reply(
+        &mut self,
+        cx: &Context,
+        ip_repr: &IpRepr,
+        repr: &TcpRepr,
+    ) -> Option<(IpRepr, TcpRepr<'static>)> {
+        if cx.now < self.challenge_ack_timer {
+            return None;
+        }
+
+        // Rate-limit to 1 per second max.
+        self.challenge_ack_timer = cx.now + Duration::from_secs(1);
+
+        return Some(self.ack_reply(ip_repr, repr));
+    }
+
     pub(crate) fn accepts(&self, ip_repr: &IpRepr, repr: &TcpRepr) -> bool {
         if self.state == State::Closed {
             return false;
@@ -1279,17 +1301,10 @@ impl<'a> TcpSocket<'a> {
         let control_len = (sent_syn as usize) + (sent_fin as usize);
 
         // Reject unacceptable acknowledgements.
-        match (self.state, repr) {
+        match (self.state, repr.control, repr.ack_number) {
             // An RST received in response to initial SYN is acceptable if it acknowledges
             // the initial SYN.
-            (
-                State::SynSent,
-                &TcpRepr {
-                    control: TcpControl::Rst,
-                    ack_number: None,
-                    ..
-                },
-            ) => {
+            (State::SynSent, TcpControl::Rst, None) => {
                 net_debug!(
                     "{}:{}:{}: unacceptable RST (expecting RST|ACK) \
                             in response to initial SYN",
@@ -1299,14 +1314,7 @@ impl<'a> TcpSocket<'a> {
                 );
                 return Err(Error::Dropped);
             }
-            (
-                State::SynSent,
-                &TcpRepr {
-                    control: TcpControl::Rst,
-                    ack_number: Some(ack_number),
-                    ..
-                },
-            ) => {
+            (State::SynSent, TcpControl::Rst, Some(ack_number)) => {
                 if ack_number != self.local_seq_no + 1 {
                     net_debug!(
                         "{}:{}:{}: unacceptable RST|ACK in response to initial SYN",
@@ -1318,35 +1326,13 @@ impl<'a> TcpSocket<'a> {
                 }
             }
             // Any other RST need only have a valid sequence number.
-            (
-                _,
-                &TcpRepr {
-                    control: TcpControl::Rst,
-                    ..
-                },
-            ) => (),
+            (_, TcpControl::Rst, _) => (),
             // The initial SYN cannot contain an acknowledgement.
-            (
-                State::Listen,
-                &TcpRepr {
-                    ack_number: None, ..
-                },
-            ) => (),
-            // This case is handled above.
-            (
-                State::Listen,
-                &TcpRepr {
-                    ack_number: Some(_),
-                    ..
-                },
-            ) => unreachable!(),
+            (State::Listen, _, None) => (),
+            // This case is handled in `accepts()`.
+            (State::Listen, _, Some(_)) => unreachable!(),
             // Every packet after the initial SYN must be an acknowledgement.
-            (
-                _,
-                &TcpRepr {
-                    ack_number: None, ..
-                },
-            ) => {
+            (_, _, None) => {
                 net_debug!(
                     "{}:{}:{}: expecting an ACK",
                     self.meta.handle,
@@ -1356,14 +1342,7 @@ impl<'a> TcpSocket<'a> {
                 return Err(Error::Dropped);
             }
             // SYN|ACK in the SYN-SENT state must have the exact ACK number.
-            (
-                State::SynSent,
-                &TcpRepr {
-                    control: TcpControl::Syn,
-                    ack_number: Some(ack_number),
-                    ..
-                },
-            ) => {
+            (State::SynSent, TcpControl::Syn, Some(ack_number)) => {
                 if ack_number != self.local_seq_no + 1 {
                     net_debug!(
                         "{}:{}:{}: unacceptable SYN|ACK in response to initial SYN",
@@ -1371,28 +1350,33 @@ impl<'a> TcpSocket<'a> {
                         self.local_endpoint,
                         self.remote_endpoint
                     );
-                    return Err(Error::Dropped);
+                    return Ok(Some(Self::rst_reply(ip_repr, repr)));
                 }
             }
             // Anything else in the SYN-SENT state is invalid.
-            (State::SynSent, _) => {
+            (State::SynSent, _, _) => {
                 net_debug!(
                     "{}:{}:{}: expecting a SYN|ACK",
                     self.meta.handle,
                     self.local_endpoint,
                     self.remote_endpoint
                 );
-                self.abort();
                 return Err(Error::Dropped);
             }
+            // ACK in the SYN-RECEIVED state must have the exact ACK number, or we RST it.
+            (State::SynReceived, _, Some(ack_number)) => {
+                if ack_number != self.local_seq_no + 1 {
+                    net_debug!(
+                        "{}:{}:{}: unacceptable ACK in response to SYN|ACK",
+                        self.meta.handle,
+                        self.local_endpoint,
+                        self.remote_endpoint
+                    );
+                    return Ok(Some(Self::rst_reply(ip_repr, repr)));
+                }
+            }
             // Every acknowledgement must be for transmitted but unacknowledged data.
-            (
-                _,
-                &TcpRepr {
-                    ack_number: Some(ack_number),
-                    ..
-                },
-            ) => {
+            (_, _, Some(ack_number)) => {
                 let unacknowledged = self.tx_buffer.len() + control_len;
 
                 // Acceptable ACK range (both inclusive)
@@ -1427,7 +1411,7 @@ impl<'a> TcpSocket<'a> {
                         ack_min,
                         ack_max
                     );
-                    return Ok(Some(self.ack_reply(ip_repr, repr)));
+                    return Ok(self.challenge_ack_reply(cx, ip_repr, repr));
                 }
             }
         }
@@ -1493,7 +1477,7 @@ impl<'a> TcpSocket<'a> {
                         self.timer.set_for_close(cx.now);
                     }
 
-                    return Ok(Some(self.ack_reply(ip_repr, repr)));
+                    return Ok(self.challenge_ack_reply(cx, ip_repr, repr));
                 }
             }
         }
@@ -3067,7 +3051,13 @@ mod test {
                 ack_number: Some(LOCAL_SEQ), // wrong
                 ..SEND_TEMPL
             },
-            Err(Error::Dropped)
+            Ok(Some(TcpRepr {
+                control: TcpControl::Rst,
+                seq_number: LOCAL_SEQ,
+                ack_number: None,
+                window_len: 0,
+                ..RECV_TEMPL
+            }))
         );
         assert_eq!(s.state, State::SynReceived);
     }
@@ -3092,11 +3082,11 @@ mod test {
                 ack_number: Some(LOCAL_SEQ + 2), // wrong
                 ..SEND_TEMPL
             },
-            // TODO is this correct? probably not
             Ok(Some(TcpRepr {
-                control: TcpControl::None,
-                seq_number: LOCAL_SEQ + 1,
-                ack_number: Some(REMOTE_SEQ + 1),
+                control: TcpControl::Rst,
+                seq_number: LOCAL_SEQ + 2,
+                ack_number: None,
+                window_len: 0,
                 ..RECV_TEMPL
             }))
         );
@@ -3424,7 +3414,13 @@ mod test {
                 window_scale: Some(0),
                 ..SEND_TEMPL
             },
-            Err(Error::Dropped)
+            Ok(Some(TcpRepr {
+                control: TcpControl::Rst,
+                seq_number: LOCAL_SEQ,
+                ack_number: None,
+                window_len: 0,
+                ..RECV_TEMPL
+            }))
         );
         assert_eq!(s.state, State::SynSent);
     }
@@ -3488,7 +3484,7 @@ mod test {
             },
             Err(Error::Dropped)
         );
-        assert_eq!(s.state, State::Closed);
+        assert_eq!(s.state, State::SynSent);
     }
 
     #[test]
@@ -3998,6 +3994,35 @@ mod test {
             }))
         );
         assert_eq!(s.remote_seq_no, REMOTE_SEQ + 1);
+
+        // Challenge ACKs are rate-limited, we don't get a second one immediately.
+        send!(
+            s,
+            time 100,
+            TcpRepr {
+                seq_number: REMOTE_SEQ + 1 + 256,
+                ack_number: Some(LOCAL_SEQ + 1),
+                ..SEND_TEMPL
+            },
+            Ok(None)
+        );
+
+        // If we wait a bit, we do get a new one.
+        send!(
+            s,
+            time 2000,
+            TcpRepr {
+                seq_number: REMOTE_SEQ + 1 + 256,
+                ack_number: Some(LOCAL_SEQ + 1),
+                ..SEND_TEMPL
+            },
+            Ok(Some(TcpRepr {
+                seq_number: LOCAL_SEQ + 1,
+                ack_number: Some(REMOTE_SEQ + 1),
+                ..RECV_TEMPL
+            }))
+        );
+        assert_eq!(s.remote_seq_no, REMOTE_SEQ + 1);
     }
 
     #[test]
@@ -4176,6 +4201,7 @@ mod test {
         // See https://github.com/smoltcp-rs/smoltcp/issues/338
         send!(
             s,
+            time 2000,
             TcpRepr {
                 control: TcpControl::Rst,
                 seq_number: REMOTE_SEQ, // Wrong seq