From patchwork Fri May 21 17:59:05 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aaron Conole X-Patchwork-Id: 1482288 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::137; helo=smtp4.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=Uz+XZsd3; dkim-atps=neutral Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4FmvWC6ZXqz9sCD for ; Sat, 22 May 2021 03:59:19 +1000 (AEST) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 820894049C; Fri, 21 May 2021 17:59:17 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 2XMsdXnMW2uM; Fri, 21 May 2021 17:59:16 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [IPv6:2605:bc80:3010:104::8cd3:938]) by smtp4.osuosl.org (Postfix) with ESMTP id 46F5140450; Fri, 21 May 2021 17:59:15 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 198EAC000D; Fri, 21 May 2021 17:59:15 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp3.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id 814EEC0001 for ; Fri, 21 May 2021 17:59:13 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 5A728606A3 for ; Fri, 21 May 2021 17:59:13 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Authentication-Results: smtp3.osuosl.org (amavisd-new); dkim=pass (1024-bit key) header.d=redhat.com Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 8TJ1vriXRWCg for ; Fri, 21 May 2021 17:59:12 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by smtp3.osuosl.org (Postfix) with ESMTPS id 487C260649 for ; Fri, 21 May 2021 17:59:12 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1621619950; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=u/anDHRC9PBsgIZiSb1inX97k/oMyZVnx5GMZ3qHcGE=; b=Uz+XZsd3EuLNK4I0o4Lcv6XhY4uJaeXAHUDjzFD862rnr1YqMM84HUEnOGGNiuS+YsU/Uj y6M71IbeZnUq8Z9NZsMrHXlkYDKjdQc/BZRMTKLXskfoX59h1bBiDuDvcpaj5g9VTn2q4P sjwRfldfe+8ljXkxDM0CxB8SzaW6rHY= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-14-5kPb3F5zMN-G65ajnwLVfQ-1; Fri, 21 May 2021 13:59:08 -0400 X-MC-Unique: 5kPb3F5zMN-G65ajnwLVfQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 025F3108BD0A; Fri, 21 May 2021 17:59:07 +0000 (UTC) Received: from RHTPC1VM0NT.redhat.com (ovpn-118-71.rdu2.redhat.com [10.10.118.71]) by smtp.corp.redhat.com (Postfix) with ESMTP id B061F10027A5; Fri, 21 May 2021 17:59:05 +0000 (UTC) From: Aaron Conole To: dev@openvswitch.org Date: Fri, 21 May 2021 13:59:05 -0400 Message-Id: <20210521175905.165779-1-aconole@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=aconole@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Cc: Wang Liang Subject: [ovs-dev] [PATCH] ipf: Fix a use-after-free error, and remove the 'do_not_steal' flag. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" As reported by Wang Liang, the way packets are passed to the ipf module doesn't allow for use later on in reassembly. Such packets may be get released anyway, such as during cleanup of tx processing. Because the ipf module lacks a way of forcing the dp_packet to be retained, it will later reuse the packet. Instead, just clone the packet and let the ipf queue own the copy until the queue is destroyed. After this change, there are no more in-tree users of the batch 'dnsteal' flag. Thus, we remove it as well. Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.") Fixes: 0b3ff31d35f5 ("dp-packet: Add 'do_not_steal' packet batch flag.") Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-April/382098.html Reported-by: Wang Liang Signed-off-by: Aaron Conole Co-authored-by: Wang Liang Signed-off-by: Wang Liang --- lib/dp-packet.h | 2 -- lib/dpif-netdev.c | 1 - lib/ipf.c | 19 ++++++------------- 3 files changed, 6 insertions(+), 16 deletions(-) diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 246be14d00..08d93c2779 100644 --- a/lib/dp-packet.h +++ b/lib/dp-packet.h @@ -738,7 +738,6 @@ enum { NETDEV_MAX_BURST = 32 }; /* Maximum number packets in a batch. */ struct dp_packet_batch { size_t count; bool trunc; /* true if the batch needs truncate. */ - bool do_not_steal; /* Indicate that the packets should not be stolen. */ struct dp_packet *packets[NETDEV_MAX_BURST]; }; @@ -747,7 +746,6 @@ dp_packet_batch_init(struct dp_packet_batch *batch) { batch->count = 0; batch->trunc = false; - batch->do_not_steal = false; } static inline void diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 650e67ab30..8fa7eb6d4f 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4169,7 +4169,6 @@ dpif_netdev_execute(struct dpif *dpif, struct dpif_execute *execute) } dp_packet_batch_init_packet(&pp, execute->packet); - pp.do_not_steal = true; dp_netdev_execute_actions(pmd, &pp, false, execute->flow, execute->actions, execute->actions_len); dp_netdev_pmd_flush_output_packets(pmd, true); diff --git a/lib/ipf.c b/lib/ipf.c index c20bcc0b33..9c83f1913a 100644 --- a/lib/ipf.c +++ b/lib/ipf.c @@ -93,7 +93,6 @@ struct ipf_frag { struct dp_packet *pkt; uint16_t start_data_byte; uint16_t end_data_byte; - bool dnsteal; /* 'do not steal': if true, ipf should not free packet. */ }; /* The key for a collection of fragments potentially making up an unfragmented @@ -795,8 +794,7 @@ ipf_is_frag_duped(const struct ipf_frag *frag_list, int last_inuse_idx, static bool ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list, struct dp_packet *pkt, uint16_t start_data_byte, - uint16_t end_data_byte, bool ff, bool lf, bool v6, - bool dnsteal) + uint16_t end_data_byte, bool ff, bool lf, bool v6) OVS_REQUIRES(ipf->ipf_lock) { bool duped_frag = ipf_is_frag_duped(ipf_list->frag_list, @@ -811,10 +809,9 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list, * recommend not setting the mempool number of buffers too low * and also clamp the number of fragments. */ struct ipf_frag *frag = &ipf_list->frag_list[last_inuse_idx + 1]; - frag->pkt = pkt; + frag->pkt = dp_packet_clone(pkt); frag->start_data_byte = start_data_byte; frag->end_data_byte = end_data_byte; - frag->dnsteal = dnsteal; ipf_list->last_inuse_idx++; atomic_count_inc(&ipf->nfrag); ipf_count(ipf, v6, IPF_NFRAGS_ACCEPTED); @@ -851,8 +848,7 @@ ipf_list_init(struct ipf_list *ipf_list, struct ipf_list_key *key, * to a list of fragemnts. */ static bool ipf_handle_frag(struct ipf *ipf, struct dp_packet *pkt, ovs_be16 dl_type, - uint16_t zone, long long now, uint32_t hash_basis, - bool dnsteal) + uint16_t zone, long long now, uint32_t hash_basis) OVS_REQUIRES(ipf->ipf_lock) { struct ipf_list_key key; @@ -921,7 +917,7 @@ ipf_handle_frag(struct ipf *ipf, struct dp_packet *pkt, ovs_be16 dl_type, } return ipf_process_frag(ipf, ipf_list, pkt, start_data_byte, - end_data_byte, ff, lf, v6, dnsteal); + end_data_byte, ff, lf, v6); } /* Filters out fragments from a batch of fragments and adjust the batch. */ @@ -942,8 +938,7 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct dp_packet_batch *pb, ipf_is_valid_v6_frag(ipf, pkt)))) { ovs_mutex_lock(&ipf->ipf_lock); - if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, hash_basis, - pb->do_not_steal)) { + if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, hash_basis)) { dp_packet_batch_refill(pb, pkt, pb_idx); } ovs_mutex_unlock(&ipf->ipf_lock); @@ -1338,9 +1333,7 @@ ipf_destroy(struct ipf *ipf) while (ipf_list->last_sent_idx < ipf_list->last_inuse_idx) { struct dp_packet *pkt = ipf_list->frag_list[ipf_list->last_sent_idx + 1].pkt; - if (!ipf_list->frag_list[ipf_list->last_sent_idx + 1].dnsteal) { - dp_packet_delete(pkt); - } + dp_packet_delete(pkt); atomic_count_dec(&ipf->nfrag); ipf_list->last_sent_idx++; }