From patchwork Tue Nov 5 14:06:12 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Dumazet X-Patchwork-Id: 288532 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 3DD6B2C025E for ; Wed, 6 Nov 2013 01:06:19 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755139Ab3KEOGQ (ORCPT ); Tue, 5 Nov 2013 09:06:16 -0500 Received: from mail-pa0-f54.google.com ([209.85.220.54]:43634 "EHLO mail-pa0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755137Ab3KEOGP (ORCPT ); Tue, 5 Nov 2013 09:06:15 -0500 Received: by mail-pa0-f54.google.com with SMTP id fa1so8836979pad.13 for ; Tue, 05 Nov 2013 06:06:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:subject:from:to:cc:date:in-reply-to:references :content-type:content-transfer-encoding:mime-version; bh=qk46q0Mfhhk1S+0Rx0jx4dgWQ04m3TtBRzgstyBwO9g=; b=xDNtFOKBuptfR/YbeZMsRYlF1zBceFIlSNKipCc21cAAaacBA6ZdYe/s/kWnhSKQh0 mro06Kz6YxclS6w7eueJgoTII0ChKGys9Bwo9qqOniAzM7qW3tMyzByedfWO8qJQMPLZ n3KeAcdB3QeTD1CgX3YPGxAQ6fLthp2r9FfXDXkMxog2gPwFx65RAQSKDtROsNV1BoIr QufIU5SO7Bx0g+oG6mleqwSQrMtcVrwfqMCr/lXQV3qj0i22Ra5lHDLXIhuaUljoIB4v s9XL2THLqTSZYgRhuBBnjE/Hi39nwkCoZWQ1OXsfEMsXp3aZypnpU9GvYjIIWB9JELWm qfjQ== X-Received: by 10.66.148.97 with SMTP id tr1mr7041445pab.163.1383660374875; Tue, 05 Nov 2013 06:06:14 -0800 (PST) Received: from [172.26.52.185] ([172.26.52.185]) by mx.google.com with ESMTPSA id kd1sm40684790pab.20.2013.11.05.06.06.13 for (version=SSLv3 cipher=RC4-SHA bits=128/128); Tue, 05 Nov 2013 06:06:14 -0800 (PST) Message-ID: <1383660372.4291.134.camel@edumazet-glaptop2.roam.corp.google.com> Subject: Re: [patch net-next 3/3] fix skb_morph to preserve skb->sk and skb->destructor pointers From: Eric Dumazet To: Jiri Pirko Cc: netdev@vger.kernel.org, davem@davemloft.net, pablo@netfilter.org, netfilter-devel@vger.kernel.org, yoshfuji@linux-ipv6.org, kadlec@blackhole.kfki.hu, kaber@trash.net, mleitner@redhat.com, kuznet@ms2.inr.ac.ru, jmorris@namei.org, wensong@linux-vs.org, horms@verge.net.au, ja@ssi.bg, edumazet@google.com, pshelar@nicira.com, jasowang@redhat.com, alexander.h.duyck@intel.com, coreteam@netfilter.org, fw@strlen.de Date: Tue, 05 Nov 2013 06:06:12 -0800 In-Reply-To: <1383649333-6321-4-git-send-email-jiri@resnulli.us> References: <1383649333-6321-1-git-send-email-jiri@resnulli.us> <1383649333-6321-4-git-send-email-jiri@resnulli.us> X-Mailer: Evolution 3.2.3-0ubuntu6 Mime-Version: 1.0 Sender: netfilter-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netfilter-devel@vger.kernel.org On Tue, 2013-11-05 at 12:02 +0100, Jiri Pirko wrote: > Currently __skb_clone sets skb->sk and skb->destructor to NULL. This is > not right for skb_morph use case because skb->sk may be previously > set (e. g. by xt_TPROXY). > > Also, during skb_morph the destructor should not be called. It might be > previously set, e. g. by xt_TPROXY to sock_edemux, and that would cause > put sk while skb is still in flight. truesize alert. You should add some documentation that skb_morph() must not be used in transmit path. Its not clear to be how this can happen. skb_morph() being used only from ipv4 defrag (or ipv6 reassembly). Maybe the problem could be fixed by doing this defrag _before_ setting skb->sk ? Also, I would prefer you find a way to put all this logic inside skb_morph() instead of adding such complexity in this already complex code, I fear the compiler will generate slower code with your patch on fast path. Maybe something as simple as following (untested) patch ? Note that the truesize concern might need some care. --- 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/core/skbuff.c b/net/core/skbuff.c index 3735fad5616e..afabfd6ef341 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -793,18 +793,28 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb) /** * skb_morph - morph one skb into another - * @dst: the skb to receive the contents + * @skb: the skb to receive the contents * @src: the skb to supply the contents * * This is identical to skb_clone except that the target skb is - * supplied by the user. + * supplied by the user, and that we keep target skb destructor in place, + * meaning this can not be used in transmit path, as skb->truesize might + * change. * * The target skb is returned upon exit. */ -struct sk_buff *skb_morph(struct sk_buff *dst, struct sk_buff *src) +struct sk_buff *skb_morph(struct sk_buff *skb, struct sk_buff *src) { - skb_release_all(dst); - return __skb_clone(dst, src); + struct sock *save_sk = skb->sk; + void (*save_destructor)(struct sk_buff *) = skb->destructor; + + skb->sk = NULL; + skb->destructor = NULL; + skb_release_all(skb); + __skb_clone(skb, src); + skb->sk = save_sk; + skb->destructor = save_destructor; + return skb; } EXPORT_SYMBOL_GPL(skb_morph);