acassen / keepalived

Keepalived
https://www.keepalived.org
GNU General Public License v2.0
4.01k stars 736 forks source link

ip_total_len and received length mismatch due to padding #2422

Closed subhajit-cdot closed 4 months ago

subhajit-cdot commented 5 months ago

Hi,

In VRRP backup node getting below message,

May 21 13:09:25 node Keepalived_vrrp[9541]: (vyatta-enp12s0.2023-12): ip_tot_len mismatch against received length. 40 and received 42
May 21 13:09:25 node Keepalived_vrrp[9541]: bogus VRRP packet received on enp12s0.2023 !!!
May 21 13:09:25 node Keepalived_vrrp[9541]: VRRP_Instance(vyatta-enp12s0.2023-12) ignoring received advertisment...

Now in the problematic interface, generating length 58 but adding 2 bytes pad (00, 00)to make it 60 bytes, but total length is still same ( 40 bytes)

16:59:41.774725 de:3e:eb:c1:50:60 > 01:00:5e:00:00:12, ethertype 802.1Q (0x8100), length 58: vlan 2023, p 0, ethertype IPv4, (tos 0xc0, ttl 255, id 14, offset 0, flags [none], proto VRRP (112), length 40)
    12.34.12.10 > 224.0.0.18: vrrp 12.34.12.10 > 224.0.0.18: VRRPv2, Advertisement, vrid 12, prio 100, authtype none, intvl 1s, length 20, addrs: 30.30.30.30
        0x0000:  45c0 0028 000e 0000 ff70 c259 0c22 0c0a
        0x0010:  e000 0012 210c 6401 0001 3eb5 1e1e 1e1e
        0x0020:  0000 0000 0000 0000 0000

When I have added two virtual addresses (30.30.30.30,40.40.40.40), the issue was gone....

16:20:29.767554 de:3e:eb:c1:50:60 > 01:00:5e:00:00:12, ethertype 802.1Q (0x8100), length 62: vlan 2023, p 0, ethertype IPv4, (tos 0xc0, ttl 255, id 57562, offset 0, flags [none], proto VRRP (112), length 44)
    12.34.12.10 > 224.0.0.18: vrrp 12.34.12.10 > 224.0.0.18: VRRPv2, Advertisement, vrid 12, prio 100, authtype none, intvl 1s, length 24, addrs(2): 30.30.30.30,40.40.40.40
        0x0000:  45c0 002c e0da 0000 ff70 e188 0c22 0c0a
        0x0010:  e000 0012 210c 6402 0001 ee63 1e1e 1e1e
        0x0020:  2828 2828 0000 0000 0000 0000

Interface type is mellanox virtual interface ...

root@node:/home/cfwha# ethtool -i enp12s0
driver: mlx5_core
version: 5.4-1.0.3
firmware-version: 24.31.1014 (MT_0000000476)
expansion-rom-version:
bus-info: 0000:0c:00.0
supports-statistics: yes
supports-test: yes
supports-eeprom-access: no
supports-register-dump: no
supports-priv-flags: yes

How to handle this issue? As ignoring advertisement may create issue in vrrp state machine ... vrrp version 1.2.24

Thanks Subhajit

pqarmitage commented 5 months ago

@subhajit-cdot You are running an extremely old version of keepalived - v1.2.24 which is now nearly 8 years old.

I have run a configuration that makes keepalived send exactly the same packets as your are seeing, but using a current version of keepalived, and I do not see the problem of the mismatched lengths that you are getting logged.

v1.2.24 used recvfrom() to receive packets, whereas keepalived nowadays uses recvmsg(), but I am not convinced that is likely to be the cause of the problem. recvmsg() does however only return 40 bytes, and not the 42 bytes that you are seeing. I do have a vague recollection of seeing this problem years ago, but it has clearly been resolved in the meantime.

What kernel version are you using? I would suggest that you update to a recent version of keepalived, but it may not build on such an old system.

Perhaps your simplest solution is just to add the dummy VIP, as you have seen that that works (if you weren't using a vlan then there would need to be at least three VIPs in the packet to avoid that padding problem being seen).

subhajit-cdot commented 5 months ago

@pqarmitage

What kernel version are you using? I would suggest that you update to a recent version of keepalived, but it may not build on such an old system.

Answer: 5.4

Interestingly issue didn't come with the virtio type interface, whereas issue observed with e1000, rtl, mlx5 type interfaces. The padding is somehow related to interface driver in kernel.

I tried with keepalived 2.2.0, same issue observed. What version should I pick? Latest is 2.3.1 I think.

pqarmitage commented 5 months ago

I wouldn't expect padding to be added when using a virtual driver, since it is not encountering the minimum frame length for Ethernet packets on the wire. I tested it using an RTL810xE adapter for receiving and recvmsg() didn't receive the padding, but wireshark showed the packet with padding. I'll have to try testing it with some other adapters. It is interesting that this hasn't been reported before.

I would suggest that you use keepalived v2.3.1 as it is indeed the latest. It should build OK on linux v5.4.

pqarmitage commented 5 months ago

@subhajit-cdot I have now run keepalived v1.2.24 and I still do not see the padding bytes being returned by recvfrom().

I have produced a patch which should resolve the issue, but obviously I can't test it. If you could try it and report back, that would be very helpful (it is produced against the v2.3.1 code, but it should apply to the v1.2.24 code, even if it needs a bit of manual intervention). 010-handle-padding.patch.txt

subhajit-cdot commented 5 months ago

@pqarmitage Not able to open this patch.

pqarmitage commented 5 months ago

@subhajit-cdot If I click on the link for the patch above, it downloads the file.

The patch looks like:

diff --git a/keepalived/vrrp/vrrp.c b/keepalived/vrrp/vrrp.c
index 9486f935..db5776f4 100644
--- a/keepalived/vrrp/vrrp.c
+++ b/keepalived/vrrp/vrrp.c
@@ -40,6 +40,7 @@
 #include <netinet/ip6.h>
 #include <stdint.h>
 #include <net/if_arp.h>
+#include <linux/if_ether.h>
 #include <net/ethernet.h>
 #ifdef _NETWORK_TIMESTAMP_
 #include <linux/net_tstamp.h>
@@ -974,11 +975,22 @@ vrrp_check_packet(vrrp_t *vrrp, const vrrphdr_t *hd, const char *buffer, ssize_t

        /* Check the IP header total packet length matches what we received */
        if (vrrp->family == AF_INET && ntohs(ip->tot_len) != buflen) {
-               log_message(LOG_INFO,
-                      "(%s) ip_tot_len mismatch against received length. %d and received %zu",
-                      vrrp->iname, ntohs(ip->tot_len), buflen);
-               ++vrrp->stats->packet_len_err;
-               return VRRP_PACKET_KO;
+               /* Allow for Ethernet frame padding. If there is padding, the
+                * frame length (excluding FCS) is 60 octets (ETH_ZLEN).
+                * The Ethernet header (14 bytes - ETH_HLEN) and any Vlan
+                * headers (4 bytes each) are removed before we receive the
+                * packet. */
+               if (buflen <= ETH_ZLEN - ETH_HLEN &&
+                   ntohs(ip->tot_len) < buflen &&
+                   (buflen - ntohs(ip->tot_len)) % 4 == 2) {
+                       /* This is OK, there is some padding */
+               } else {
+                       log_message(LOG_INFO,
+                              "(%s) ip_tot_len mismatch against received length. %d and received %zu",
+                              vrrp->iname, ntohs(ip->tot_len), buflen);
+                       ++vrrp->stats->packet_len_err;
+                       return VRRP_PACKET_KO;
+               }
        }

        /* MUST verify the VRRP checksum. Kernel takes care of checksum mismatch incase of IPv6. */

but if you copy and paste it from here you would need to convert spaces to tabs. If might be easier to copy the following and replace the '?'s with tabs.

diff --git a/keepalived/vrrp/vrrp.c b/keepalived/vrrp/vrrp.c
index 9486f935..db5776f4 100644
--- a/keepalived/vrrp/vrrp.c
+++ b/keepalived/vrrp/vrrp.c
@@ -40,6 +40,7 @@
 #include <netinet/ip6.h>
 #include <stdint.h>
 #include <net/if_arp.h>
+#include <linux/if_ether.h>
 #include <net/ethernet.h>
 #ifdef _NETWORK_TIMESTAMP_
 #include <linux/net_tstamp.h>
@@ -974,11 +975,22 @@ vrrp_check_packet(vrrp_t *vrrp, const vrrphdr_t *hd, const char *buffer, ssize_t

 ?/* Check the IP header total packet length matches what we received */
 ?if (vrrp->family == AF_INET && ntohs(ip->tot_len) != buflen) {
-??log_message(LOG_INFO,
-??       "(%s) ip_tot_len mismatch against received length. %d and received %zu",
-??       vrrp->iname, ntohs(ip->tot_len), buflen);
-??++vrrp->stats->packet_len_err;
-??return VRRP_PACKET_KO;
+??/* Allow for Ethernet frame padding. If there is padding, the
+?? * frame length (excluding FCS) is 60 octets (ETH_ZLEN).
+?? * The Ethernet header (14 bytes - ETH_HLEN) and any Vlan
+?? * headers (4 bytes each) are removed before we receive the
+?? * packet. */
+??if (buflen <= ETH_ZLEN - ETH_HLEN &&
+??    ntohs(ip->tot_len) < buflen &&
+??    (buflen - ntohs(ip->tot_len)) % 4 == 2) {
+???/* This is OK, there is some padding */
+??} else {
+???log_message(LOG_INFO,
+???       "(%s) ip_tot_len mismatch against received length. %d and received %zu",
+???       vrrp->iname, ntohs(ip->tot_len), buflen);
+???++vrrp->stats->packet_len_err;
+???return VRRP_PACKET_KO;
+??}
 ?}

 ?/* MUST verify the VRRP checksum. Kernel takes care of checksum mismatch incase of IPv6. */
pqarmitage commented 5 months ago

@subhajit-cdot Can you confirm whether the patch above worked for you or not? If you can confirm it does work I can merge it.

subhajit-cdot commented 4 months ago

@pqarmitage checked this patch ... working with ip_tot_len issue .. but in the next check between expected_len and buflen .. it was giving issue .. so added same logic there and it's working fine .

`

    if (expected_len != buflen) {
            /* Allow for Ethernet frame padding. If there is padding, the
             * frame length (excluding FCS) is 60 octets (ETH_ZLEN).
             * The Ethernet header (14 bytes - ETH_HLEN) and any Vlan
             * headers (4 bytes each) are removed before we receive the
             * packet. */
            if (buflen <= ETH_ZLEN - ETH_HLEN &&
                expected_len < buflen &&
                (buflen - expected_len) % 4 == 2) {
                    /* This is OK, there is some padding */
           } else {
                   log_message(LOG_INFO,
                   "(%s): Received packet length mismatch against expected. %zu and expect %zu",
                   vrrp->iname, buflen, expected_len);
                   ++vrrp->stats->packet_len_err;
                   return VRRP_PACKET_KO;
           }
    }

`

pqarmitage commented 4 months ago

@subhajit-cdot I have merge my original patch along with your suggested additional code.

I slightly tweaked the code, so please let me know if you find any problems with it.