diff mbox series

[ovs-dev,1/2] netlink-conntrack: Do not fail to parse if optional TCP protocol attributes are not found.

Message ID 165962922715.2700301.13804491466126298416.stgit@fed.void
State Accepted
Commit 0c4b063190addba46388eefc361e6f40287dea93
Headers show
Series [ovs-dev,1/2] netlink-conntrack: Do not fail to parse if optional TCP protocol attributes are not found. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

Paolo Valerio Aug. 4, 2022, 4:07 p.m. UTC 
Some of the CTA_PROTOINFO_TCP nested attributes are not always
included in the received message, but the parsing logic considers them
as required, failing in case they are not found.

This was observed while monitoring some connections by reading the
events sent by conntrack:

./ovstest test-netlink-conntrack monitor
[...]
2022-08-04T09:39:02Z|00007|netlink_conntrack|ERR|Could not parse nested TCP protoinfo
  options. Possibly incompatible Linux kernel version.
2022-08-04T09:39:02Z|00008|netlink_notifier|WARN|unexpected netlink message contents
[...]

All the TCP DELETE/DESTROY events fail to parse with the message
above.

Fix it by turning the relevant attributes to optional.

Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
---
- [1] is the related piece of code that skips flags and wscale for the
  destroy evts.

[1] https://github.com/torvalds/linux/blob/master/net/netfilter/nf_conntrack_proto_tcp.c#L1202
---
 lib/netlink-conntrack.c |   45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

Comments

Ilya Maximets Aug. 31, 2022, 9:54 a.m. UTC | #1 
On 8/4/22 18:07, Paolo Valerio wrote:
> Some of the CTA_PROTOINFO_TCP nested attributes are not always
> included in the received message, but the parsing logic considers them
> as required, failing in case they are not found.
> 
> This was observed while monitoring some connections by reading the
> events sent by conntrack:
> 
> ./ovstest test-netlink-conntrack monitor
> [...]
> 2022-08-04T09:39:02Z|00007|netlink_conntrack|ERR|Could not parse nested TCP protoinfo
>   options. Possibly incompatible Linux kernel version.
> 2022-08-04T09:39:02Z|00008|netlink_notifier|WARN|unexpected netlink message contents
> [...]
> 
> All the TCP DELETE/DESTROY events fail to parse with the message
> above.
> 
> Fix it by turning the relevant attributes to optional.
> 
> Signed-off-by: Paolo Valerio <pvalerio@redhat.com>
> ---
> - [1] is the related piece of code that skips flags and wscale for the
>   destroy evts.
> 
> [1] https://github.com/torvalds/linux/blob/master/net/netfilter/nf_conntrack_proto_tcp.c#L1202
> ---
>  lib/netlink-conntrack.c |   45 +++++++++++++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 18 deletions(-)

Thanks!  I applied this one patch.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
index 78f1bf60b..4fcde9ba1 100644
--- a/lib/netlink-conntrack.c
+++ b/lib/netlink-conntrack.c
@@ -672,13 +672,13 @@  nl_ct_parse_protoinfo_tcp(struct nlattr *nla,
     static const struct nl_policy policy[] = {
         [CTA_PROTOINFO_TCP_STATE] = { .type = NL_A_U8, .optional = false },
         [CTA_PROTOINFO_TCP_WSCALE_ORIGINAL] = { .type = NL_A_U8,
-                                                .optional = false },
+                                                .optional = true },
         [CTA_PROTOINFO_TCP_WSCALE_REPLY] = { .type = NL_A_U8,
-                                             .optional = false },
+                                             .optional = true },
         [CTA_PROTOINFO_TCP_FLAGS_ORIGINAL] = { .type = NL_A_U16,
-                                               .optional = false },
+                                               .optional = true },
         [CTA_PROTOINFO_TCP_FLAGS_REPLY] = { .type = NL_A_U16,
-                                            .optional = false },
+                                            .optional = true },
     };
     struct nlattr *attrs[ARRAY_SIZE(policy)];
     bool parsed;
@@ -695,20 +695,29 @@  nl_ct_parse_protoinfo_tcp(struct nlattr *nla,
          * connection, but our structures store a separate state for
          * each endpoint.  Here we duplicate the state. */
         protoinfo->tcp.state_orig = protoinfo->tcp.state_reply = state;
-        protoinfo->tcp.wscale_orig = nl_attr_get_u8(
-            attrs[CTA_PROTOINFO_TCP_WSCALE_ORIGINAL]);
-        protoinfo->tcp.wscale_reply = nl_attr_get_u8(
-            attrs[CTA_PROTOINFO_TCP_WSCALE_REPLY]);
-        flags_orig =
-            nl_attr_get_unspec(attrs[CTA_PROTOINFO_TCP_FLAGS_ORIGINAL],
-                               sizeof *flags_orig);
-        protoinfo->tcp.flags_orig =
-            ip_ct_tcp_flags_to_dpif(flags_orig->flags);
-        flags_reply =
-            nl_attr_get_unspec(attrs[CTA_PROTOINFO_TCP_FLAGS_REPLY],
-                               sizeof *flags_reply);
-        protoinfo->tcp.flags_reply =
-            ip_ct_tcp_flags_to_dpif(flags_reply->flags);
+
+        if (attrs[CTA_PROTOINFO_TCP_WSCALE_ORIGINAL]) {
+            protoinfo->tcp.wscale_orig =
+                nl_attr_get_u8(attrs[CTA_PROTOINFO_TCP_WSCALE_ORIGINAL]);
+        }
+        if (attrs[CTA_PROTOINFO_TCP_WSCALE_REPLY]) {
+            protoinfo->tcp.wscale_reply =
+                nl_attr_get_u8(attrs[CTA_PROTOINFO_TCP_WSCALE_REPLY]);
+        }
+        if (attrs[CTA_PROTOINFO_TCP_FLAGS_ORIGINAL]) {
+            flags_orig =
+                nl_attr_get_unspec(attrs[CTA_PROTOINFO_TCP_FLAGS_ORIGINAL],
+                                   sizeof *flags_orig);
+            protoinfo->tcp.flags_orig =
+                ip_ct_tcp_flags_to_dpif(flags_orig->flags);
+        }
+        if (attrs[CTA_PROTOINFO_TCP_FLAGS_REPLY]) {
+            flags_reply =
+                nl_attr_get_unspec(attrs[CTA_PROTOINFO_TCP_FLAGS_REPLY],
+                                   sizeof *flags_reply);
+            protoinfo->tcp.flags_reply =
+                ip_ct_tcp_flags_to_dpif(flags_reply->flags);
+        }
     } else {
         VLOG_ERR_RL(&rl, "Could not parse nested TCP protoinfo options. "
                     "Possibly incompatible Linux kernel version.");