From patchwork Tue Oct 20 18:43:18 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joe Stringer X-Patchwork-Id: 533111 X-Patchwork-Delegate: pablo@netfilter.org Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id A1BB81402DD for ; Wed, 21 Oct 2015 05:43:39 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752016AbbJTSni (ORCPT ); Tue, 20 Oct 2015 14:43:38 -0400 Received: from mail-yk0-f174.google.com ([209.85.160.174]:36783 "EHLO mail-yk0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751140AbbJTSnh (ORCPT ); Tue, 20 Oct 2015 14:43:37 -0400 Received: by ykba4 with SMTP id a4so19909012ykb.3 for ; Tue, 20 Oct 2015 11:43:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-type; bh=j9oEQ5GkBnJ0gm8db8kl7iQcm0utsqgKKJ3ZV0KIGtI=; b=eP0fyHjTpD/ZVrzRzhwojPus4e+m8S8xI7764rHcJaTdA6JXdlo4dG53ksrVxdbaAH wlAD0nkA8pX1uipDMujQVfymRmdsKQrnH1Sd/sO311iPACrZq/GXLv2Z0GgA3X5izcoD /sq4+JZmt8M0WJNugKPf8ReReQGpLSqIz1PVeFhfO4T6Wohyg8Ak/VXPk0oFzdSlD7OG 8pmjXRb3aJrKdAFExKVRHc4NddaZvOHuEpPW2lA48x2TGtMgKziP8gw4LoB1Clw6hxzS 8kgNYvA51ori2EWLuzxEY0uFPqFBoPlTkWZcNHQGAWqrWrqWmxyBtqBWGppF6ilCutUu nJug== X-Gm-Message-State: ALoCoQnoFU+EKaBpPq3KDzbLWKxDfkd/CZ8VYaf5kFKXEseWfQ7zktWiDxTZt5SA71EtIYsxvt6X X-Received: by 10.13.225.193 with SMTP id k184mr3465073ywe.101.1445366617328; Tue, 20 Oct 2015 11:43:37 -0700 (PDT) MIME-Version: 1.0 Received: by 10.129.134.132 with HTTP; Tue, 20 Oct 2015 11:43:18 -0700 (PDT) In-Reply-To: <20151020081716.GH4386@breakpoint.cc> References: <1445112865-31523-1-git-send-email-fw@strlen.de> <20151020081716.GH4386@breakpoint.cc> From: Joe Stringer Date: Tue, 20 Oct 2015 11:43:18 -0700 Message-ID: Subject: Re: [PATCH nf-next 0/4] netfilter: rework netfilter ipv6 defrag To: Florian Westphal Cc: netfilter-devel@vger.kernel.org, Linux Netdev List , Andy Zhou Sender: netfilter-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org On 20 October 2015 at 01:17, Florian Westphal wrote: > Joe Stringer wrote: >> On 17 October 2015 at 13:14, Florian Westphal wrote: >> > [ CC netdev since patch #2 isn't nf-specific. Dave, if you want >> > I can resubmit that one after the next nf-pull request; let me know if >> > you would prefer that ]. >> > >> > Openvswitch seems broken wrt. to defragmentation, it doesn't call >> > nf_ct_frag6_consume_orig to free the original fragments. >> >> This will need to be fixed for 'net' as well, do you have a path in >> mind for that? > > Good point. No, I don't. Any suggestions? > I can try to just re-target -nf tree (sans patch #2). Pablo? The smallest change seems to be adding the nf_ct_frag6_consume_orig() call to OVS, plus the morph logic from patch 3. Alternatively if Pablo is fine with having the series re-targeted, then that sounds reasonable to me too. > ipv4 side seems broken as well (ip_defrag frees skb on errors other than > -EINPROGRESS, so it looks like we will double-free in > do_execute_actions) Oh dear. Thanks for the report. I propose wrapping the ip_defrag() with an skb_get()/skb_consume() as this seems to require the least invasive changes: I don't have a test environment to verify this at the moment though. Let me know if you'll take this with your patch series or if you'd like me to send it out. >> Patch 3 when taken independently from patch 4 hides user-visible error >> codes on the OVS side. The OVS conntrack action hides -EINPROGRESS >> from userspace, treating it as a successful execution. All other >> errors are returned up. With that patch, all errors will be hidden. I >> see that it's fixed in Patch 4, so maybe it's not a biggie but those >> two patches should be tightly coupled. > > You're right, we can't signal "skb unchanged". I guess one could > just test wheter skb is a fragment and -EINVAL if it is, not sure > if its worth doing given that such test would be removed again > by the very next patch? On second thought, this would only affect 'net', and if we're considering the solution for that tree too then it really doesn't matter. --- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c index a5ec34f8502f..0d2d24c99fd5 100644 --- a/net/openvswitch/conntrack.c +++ b/net/openvswitch/conntrack.c @@ -303,7 +303,11 @@ static int handle_fragments(struct net *net, struct sw_flow_key *key, int err; memset(IPCB(skb), 0, sizeof(struct inet_skb_parm)); + + skb_get(skb); err = ip_defrag(skb, user); + if (!err || err == -EINPROGRESS) + consume_skb(skb); if (err) return err;