[ovs-dev] ipf: bail out when ipf state is COMPLETED
diff mbox series

Message ID 1573723098-15174-1-git-send-email-lirongqing@baidu.com
State New
Headers show
Series
  • [ovs-dev] ipf: bail out when ipf state is COMPLETED
Related show

Commit Message

Li RongQing Nov. 14, 2019, 9:18 a.m. UTC
it is easy to crash ovs when a packet with same id
hits a list that already reassembled completedly
but have not been sent out yet, and this packet is
not duplicate with this hit ipf list due to bigger
offset

    1  0x00007f9fef0ae2d9 in __GI_abort () at abort.c:89
    2  0x0000000000464042 in ipf_list_state_transition at lib/ipf.c:545

Co-authored-by: Wang Li <wangli39@baidu.com>
Signed-off-by: Wang Li <wangli39@baidu.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 lib/ipf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Darrell Ball Nov. 14, 2019, 3:38 p.m. UTC | #1
Hi RongQing/Wang

On Thu, Nov 14, 2019 at 1:26 AM Li RongQing <lirongqing@baidu.com> wrote:

> it is easy to crash ovs when a packet with same id
> hits a list that already reassembled completedly
> but have not been sent out yet, and this packet is
> not duplicate with this hit ipf list due to bigger
> offset
>
>     1  0x00007f9fef0ae2d9 in __GI_abort () at abort.c:89
>

Good DOS test.
Fix is correct.

This needs a 'Fixes' tag.
Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")

This will need to be backported to 2.10.

Thanks Darrell

    2  0x0000000000464042 in ipf_list_state_transition at lib/ipf.c:545
>
> Co-authored-by: Wang Li <wangli39@baidu.com>
> Signed-off-by: Wang Li <wangli39@baidu.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  lib/ipf.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index 4cc0f2df6..45c489122 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -899,7 +899,8 @@ ipf_handle_frag(struct ipf *ipf, struct dp_packet
> *pkt, ovs_be16 dl_type,
>                        MIN(max_frag_list_size,
> IPF_FRAG_LIST_MIN_INCREMENT));
>          hmap_insert(&ipf->frag_lists, &ipf_list->node, hash);
>          ipf_expiry_list_add(&ipf->frag_exp_list, ipf_list, now);
> -    } else if (ipf_list->state == IPF_LIST_STATE_REASS_FAIL) {
> +    } else if (ipf_list->state == IPF_LIST_STATE_REASS_FAIL ||
> +               ipf_list->state == IPF_LIST_STATE_COMPLETED) {
>          /* Bail out as early as possible. */
>          return false;
>      } else if (ipf_list->last_inuse_idx + 1 >= ipf_list->size) {
> --
> 2.16.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Ben Pfaff Nov. 22, 2019, 12:47 a.m. UTC | #2
On Thu, Nov 14, 2019 at 07:38:55AM -0800, Darrell Ball wrote:
> Hi RongQing/Wang
> 
> On Thu, Nov 14, 2019 at 1:26 AM Li RongQing <lirongqing@baidu.com> wrote:
> 
> > it is easy to crash ovs when a packet with same id
> > hits a list that already reassembled completedly
> > but have not been sent out yet, and this packet is
> > not duplicate with this hit ipf list due to bigger
> > offset
> >
> >     1  0x00007f9fef0ae2d9 in __GI_abort () at abort.c:89
> >
> 
> Good DOS test.
> Fix is correct.
> 
> This needs a 'Fixes' tag.
> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
> 
> This will need to be backported to 2.10.

I pushed to master and branch-2.12.  2.11 and 2.10 need some kind of
conflict resolution.

Patch
diff mbox series

diff --git a/lib/ipf.c b/lib/ipf.c
index 4cc0f2df6..45c489122 100644
--- a/lib/ipf.c
+++ b/lib/ipf.c
@@ -899,7 +899,8 @@  ipf_handle_frag(struct ipf *ipf, struct dp_packet *pkt, ovs_be16 dl_type,
                       MIN(max_frag_list_size, IPF_FRAG_LIST_MIN_INCREMENT));
         hmap_insert(&ipf->frag_lists, &ipf_list->node, hash);
         ipf_expiry_list_add(&ipf->frag_exp_list, ipf_list, now);
-    } else if (ipf_list->state == IPF_LIST_STATE_REASS_FAIL) {
+    } else if (ipf_list->state == IPF_LIST_STATE_REASS_FAIL ||
+               ipf_list->state == IPF_LIST_STATE_COMPLETED) {
         /* Bail out as early as possible. */
         return false;
     } else if (ipf_list->last_inuse_idx + 1 >= ipf_list->size) {