[ovs-dev,v2,1/2] ipf: Only add fragments to batch of same dl_type.

When conntrack is reassembling packet fragments, the same reassembly
context can be shared across multiple threads handling different packets
simultaneously. Once a full packet is assembled, it is added to a packet
batch for processing, this is most likely the batch that added it in the
first place, but that isn't a guarantee.

The packets in these batches should be segregated by network protocol
version (ipv4 vs ipv6) for conntrack defragmentation to function
appropriately. However, there are conditions where we would add a
reassembled packet of one type to a batch of another.

This change introduces checks to make sure that reassembled or expired
fragments are only added to packet batches of the same type.

Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
Reported-at: https://issues.redhat.com/browse/FDP-560
Signed-off-by: Mike Pattrick <mkp@redhat.com>
---
 lib/ipf.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Mike Pattrick <mkp@redhat.com> writes:

> When conntrack is reassembling packet fragments, the same reassembly
> context can be shared across multiple threads handling different packets
> simultaneously. Once a full packet is assembled, it is added to a packet
> batch for processing, this is most likely the batch that added it in the
> first place, but that isn't a guarantee.
>
> The packets in these batches should be segregated by network protocol
> version (ipv4 vs ipv6) for conntrack defragmentation to function
> appropriately. However, there are conditions where we would add a
> reassembled packet of one type to a batch of another.
>
> This change introduces checks to make sure that reassembled or expired
> fragments are only added to packet batches of the same type.
>
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Reported-at: https://issues.redhat.com/browse/FDP-560
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> ---

Acked-by: Paolo Valerio <pvalerio@redhat.com>

>  lib/ipf.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index 7d74e2c13..3c8960be3 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -1063,6 +1063,9 @@ ipf_send_completed_frags(struct ipf *ipf, struct dp_packet_batch *pb,
>      struct ipf_list *ipf_list;
>  
>      LIST_FOR_EACH_SAFE (ipf_list, list_node, &ipf->frag_complete_list) {
> +        if ((ipf_list->key.dl_type == htons(ETH_TYPE_IPV6)) != v6) {
> +            continue;
> +        }
>          if (ipf_send_frags_in_list(ipf, ipf_list, pb, IPF_FRAG_COMPLETED_LIST,
>                                     v6, now)) {
>              ipf_completed_list_clean(&ipf->frag_lists, ipf_list);
> @@ -1096,6 +1099,9 @@ ipf_send_expired_frags(struct ipf *ipf, struct dp_packet_batch *pb,
>      size_t lists_removed = 0;
>  
>      LIST_FOR_EACH_SAFE (ipf_list, list_node, &ipf->frag_exp_list) {
> +        if ((ipf_list->key.dl_type == htons(ETH_TYPE_IPV6)) != v6) {
> +            continue;
> +        }
>          if (now <= ipf_list->expiration ||
>              lists_removed >= IPF_FRAG_LIST_MAX_EXPIRED) {
>              break;
> @@ -1116,7 +1122,8 @@ ipf_send_expired_frags(struct ipf *ipf, struct dp_packet_batch *pb,
>  /* Adds a reassmebled packet to a packet batch to be processed by the caller.
>   */
>  static void
> -ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb)
> +ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb,
> +                       ovs_be16 dl_type)
>  {
>      if (ovs_list_is_empty(&ipf->reassembled_pkt_list)) {
>          return;
> @@ -1127,6 +1134,7 @@ ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb)
>  
>      LIST_FOR_EACH_SAFE (rp, rp_list_node, &ipf->reassembled_pkt_list) {
>          if (!rp->list->reass_execute_ctx &&
> +            rp->list->key.dl_type == dl_type &&
>              ipf_dp_packet_batch_add(pb, rp->pkt, false)) {
>              rp->list->reass_execute_ctx = rp->pkt;
>          }
> @@ -1237,7 +1245,7 @@ ipf_preprocess_conntrack(struct ipf *ipf, struct dp_packet_batch *pb,
>      }
>  
>      if (ipf_get_enabled(ipf) || atomic_count_get(&ipf->nfrag)) {
> -        ipf_execute_reass_pkts(ipf, pb);
> +        ipf_execute_reass_pkts(ipf, pb, dl_type);
>      }
>  }
>  
> -- 
> 2.39.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Thu, May 16, 2024 at 11:38:31AM -0400, Mike Pattrick wrote:
> When conntrack is reassembling packet fragments, the same reassembly
> context can be shared across multiple threads handling different packets
> simultaneously. Once a full packet is assembled, it is added to a packet
> batch for processing, this is most likely the batch that added it in the
> first place, but that isn't a guarantee.
> 
> The packets in these batches should be segregated by network protocol
> version (ipv4 vs ipv6) for conntrack defragmentation to function
> appropriately. However, there are conditions where we would add a
> reassembled packet of one type to a batch of another.
> 
> This change introduces checks to make sure that reassembled or expired
> fragments are only added to packet batches of the same type.
> 
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Reported-at: https://issues.redhat.com/browse/FDP-560
> Signed-off-by: Mike Pattrick <mkp@redhat.com>

Acked-by: Simon Horman <horms@ovn.org>

Mike Pattrick <mkp@redhat.com> writes:

> When conntrack is reassembling packet fragments, the same reassembly
> context can be shared across multiple threads handling different packets
> simultaneously. Once a full packet is assembled, it is added to a packet
> batch for processing, this is most likely the batch that added it in the
> first place, but that isn't a guarantee.
>
> The packets in these batches should be segregated by network protocol
> version (ipv4 vs ipv6) for conntrack defragmentation to function
> appropriately. However, there are conditions where we would add a
> reassembled packet of one type to a batch of another.
>
> This change introduces checks to make sure that reassembled or expired
> fragments are only added to packet batches of the same type.
>
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> Reported-at: https://issues.redhat.com/browse/FDP-560
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> ---

Applied, and backported to branches down to 2.17