| Message ID | 1584645676-19248-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev,v3] conntrack: Reset ct_state when entering a new zone. | expand |
Dumitru Ceara <dceara@redhat.com> writes: > When a new conntrack zone is entered, the ct_state field is zeroed in > order to avoid using state information from different zones. > > One such scenario is when a packet is double NATed. Assuming two zones > and 3 flows performing the following actions in order on the packet: > 1. ct(zone=5,nat), recirc > 2. ct(zone=1), recirc > 3. ct(zone=1,nat) > > If at step #1 the packet matches an existing NAT entry, it will get > translated and pkt->md.ct_state is set to CS_DST_NAT or CS_SRC_NAT. > At step #2 the new tuple might match an existing connection and > pkt->md.ct_zone is set to 1. > If at step #3 the packet matches an existing NAT entry in zone 1, > handle_nat() will be called to perform the translation but it will > return early because the packet's zone matches the conntrack zone and > the ct_state field still contains CS_DST_NAT or CS_SRC_NAT from the > translations in zone 5. > > In order to reliably detect when a packet enters a new conntrack zone > we also need to make sure that the pkt->md.ct_zone is properly > initialized if pkt->md.ct_state is non-zero. This already happens for > most cases. The only exception is when matched conntrack connection is > of type CT_CONN_TYPE_UN_NAT and the master connection is missing. To > cover this path we now call write_ct_md() in that case too. Remove > setting the CS_TRACKED flag as in this case as it will be done by the > new call to write_ct_md(). > > CC: Darrell Ball <dlu998@gmail.com> > Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.") > Acked-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > V3: > - Add Ilya's ack and fix "Fixes" tag. > - Remove NULL pointer dereference fix as there's already a patch for it: > https://patchwork.ozlabs.org/patch/1257010/ > V2: > - Address Ilya's comments: > - revert changes to pkt_metadata_init(). > - update ct_state in process_one() only if ct_state is already > non-zero. > - Make sure pkt->md.ct_zone is always initialized when pkt->md.ct_state > is non-zero. > - Fix NULL pointer dereference in process_one() if conn_type is > CT_CONN_TYPE_UN_NAT and master conn is not found. > --- Acked-by: Aaron Conole <aconole@redhat.com>
On 3/20/20 9:44 PM, Aaron Conole wrote: > Dumitru Ceara <dceara@redhat.com> writes: > >> When a new conntrack zone is entered, the ct_state field is zeroed in >> order to avoid using state information from different zones. >> >> One such scenario is when a packet is double NATed. Assuming two zones >> and 3 flows performing the following actions in order on the packet: >> 1. ct(zone=5,nat), recirc >> 2. ct(zone=1), recirc >> 3. ct(zone=1,nat) >> >> If at step #1 the packet matches an existing NAT entry, it will get >> translated and pkt->md.ct_state is set to CS_DST_NAT or CS_SRC_NAT. >> At step #2 the new tuple might match an existing connection and >> pkt->md.ct_zone is set to 1. >> If at step #3 the packet matches an existing NAT entry in zone 1, >> handle_nat() will be called to perform the translation but it will >> return early because the packet's zone matches the conntrack zone and >> the ct_state field still contains CS_DST_NAT or CS_SRC_NAT from the >> translations in zone 5. >> >> In order to reliably detect when a packet enters a new conntrack zone >> we also need to make sure that the pkt->md.ct_zone is properly >> initialized if pkt->md.ct_state is non-zero. This already happens for >> most cases. The only exception is when matched conntrack connection is >> of type CT_CONN_TYPE_UN_NAT and the master connection is missing. To >> cover this path we now call write_ct_md() in that case too. Remove >> setting the CS_TRACKED flag as in this case as it will be done by the >> new call to write_ct_md(). >> >> CC: Darrell Ball <dlu998@gmail.com> >> Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.") >> Acked-by: Ilya Maximets <i.maximets@ovn.org> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> >> --- >> V3: >> - Add Ilya's ack and fix "Fixes" tag. >> - Remove NULL pointer dereference fix as there's already a patch for it: >> https://patchwork.ozlabs.org/patch/1257010/ >> V2: >> - Address Ilya's comments: >> - revert changes to pkt_metadata_init(). >> - update ct_state in process_one() only if ct_state is already >> non-zero. >> - Make sure pkt->md.ct_zone is always initialized when pkt->md.ct_state >> is non-zero. >> - Fix NULL pointer dereference in process_one() if conn_type is >> CT_CONN_TYPE_UN_NAT and master conn is not found. >> --- > > Acked-by: Aaron Conole <aconole@redhat.com> > Thanks, Dumitru and Aaron! Applied to master and backported down to 2.8. Best regards, Ilya Maximets.
On 3/24/20 3:55 PM, Ilya Maximets wrote: > On 3/20/20 9:44 PM, Aaron Conole wrote: >> Dumitru Ceara <dceara@redhat.com> writes: >> >>> When a new conntrack zone is entered, the ct_state field is zeroed in >>> order to avoid using state information from different zones. >>> >>> One such scenario is when a packet is double NATed. Assuming two zones >>> and 3 flows performing the following actions in order on the packet: >>> 1. ct(zone=5,nat), recirc >>> 2. ct(zone=1), recirc >>> 3. ct(zone=1,nat) >>> >>> If at step #1 the packet matches an existing NAT entry, it will get >>> translated and pkt->md.ct_state is set to CS_DST_NAT or CS_SRC_NAT. >>> At step #2 the new tuple might match an existing connection and >>> pkt->md.ct_zone is set to 1. >>> If at step #3 the packet matches an existing NAT entry in zone 1, >>> handle_nat() will be called to perform the translation but it will >>> return early because the packet's zone matches the conntrack zone and >>> the ct_state field still contains CS_DST_NAT or CS_SRC_NAT from the >>> translations in zone 5. >>> >>> In order to reliably detect when a packet enters a new conntrack zone >>> we also need to make sure that the pkt->md.ct_zone is properly >>> initialized if pkt->md.ct_state is non-zero. This already happens for >>> most cases. The only exception is when matched conntrack connection is >>> of type CT_CONN_TYPE_UN_NAT and the master connection is missing. To >>> cover this path we now call write_ct_md() in that case too. Remove >>> setting the CS_TRACKED flag as in this case as it will be done by the >>> new call to write_ct_md(). >>> >>> CC: Darrell Ball <dlu998@gmail.com> >>> Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.") >>> Acked-by: Ilya Maximets <i.maximets@ovn.org> >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> >>> --- >>> V3: >>> - Add Ilya's ack and fix "Fixes" tag. >>> - Remove NULL pointer dereference fix as there's already a patch for it: >>> https://patchwork.ozlabs.org/patch/1257010/ >>> V2: >>> - Address Ilya's comments: >>> - revert changes to pkt_metadata_init(). >>> - update ct_state in process_one() only if ct_state is already >>> non-zero. >>> - Make sure pkt->md.ct_zone is always initialized when pkt->md.ct_state >>> is non-zero. >>> - Fix NULL pointer dereference in process_one() if conn_type is >>> CT_CONN_TYPE_UN_NAT and master conn is not found. >>> --- >> >> Acked-by: Aaron Conole <aconole@redhat.com> >> > > > Thanks, Dumitru and Aaron! > Applied to master and backported down to 2.8. > > Best regards, Ilya Maximets. > Thanks!
diff --git a/lib/conntrack.c b/lib/conntrack.c index ff5a894..0c41664 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -1277,6 +1277,11 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, const struct nat_action_info_t *nat_action_info, ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper) { + /* Reset ct_state whenever entering a new zone. */ + if (pkt->md.ct_state && pkt->md.ct_zone != zone) { + pkt->md.ct_state = 0; + } + bool create_new_conn = false; conn_key_lookup(ct, &ctx->key, ctx->hash, now, &ctx->conn, &ctx->reply); struct conn *conn = ctx->conn; @@ -1300,7 +1305,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt, conn_key_lookup(ct, &ctx->key, hash, now, &conn, &ctx->reply); if (!conn) { - pkt->md.ct_state |= CS_TRACKED | CS_INVALID; + pkt->md.ct_state |= CS_INVALID; + write_ct_md(pkt, zone, NULL, NULL, NULL); char *log_msg = xasprintf("Missing master conn %p", rev_conn); ct_print_conn_info(conn, log_msg, VLL_INFO, true, true); free(log_msg);
When a new conntrack zone is entered, the ct_state field is zeroed in order to avoid using state information from different zones. One such scenario is when a packet is double NATed. Assuming two zones and 3 flows performing the following actions in order on the packet: 1. ct(zone=5,nat), recirc 2. ct(zone=1), recirc 3. ct(zone=1,nat) If at step #1 the packet matches an existing NAT entry, it will get translated and pkt->md.ct_state is set to CS_DST_NAT or CS_SRC_NAT. At step #2 the new tuple might match an existing connection and pkt->md.ct_zone is set to 1. If at step #3 the packet matches an existing NAT entry in zone 1, handle_nat() will be called to perform the translation but it will return early because the packet's zone matches the conntrack zone and the ct_state field still contains CS_DST_NAT or CS_SRC_NAT from the translations in zone 5. In order to reliably detect when a packet enters a new conntrack zone we also need to make sure that the pkt->md.ct_zone is properly initialized if pkt->md.ct_state is non-zero. This already happens for most cases. The only exception is when matched conntrack connection is of type CT_CONN_TYPE_UN_NAT and the master connection is missing. To cover this path we now call write_ct_md() in that case too. Remove setting the CS_TRACKED flag as in this case as it will be done by the new call to write_ct_md(). CC: Darrell Ball <dlu998@gmail.com> Fixes: 286de2729955 ("dpdk: Userspace Datapath: Introduce NAT Support.") Acked-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- V3: - Add Ilya's ack and fix "Fixes" tag. - Remove NULL pointer dereference fix as there's already a patch for it: https://patchwork.ozlabs.org/patch/1257010/ V2: - Address Ilya's comments: - revert changes to pkt_metadata_init(). - update ct_state in process_one() only if ct_state is already non-zero. - Make sure pkt->md.ct_zone is always initialized when pkt->md.ct_state is non-zero. - Fix NULL pointer dereference in process_one() if conn_type is CT_CONN_TYPE_UN_NAT and master conn is not found. --- lib/conntrack.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)