Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp5610640imm; Mon, 23 Jul 2018 02:53:48 -0700 (PDT) X-Google-Smtp-Source: AAOMgpfXOEQCMhd+6IPXatroh63+iGEk3OJJE36LTf3Zzo+T0sS8d4nGSKmEr2ocgPsvyTHfpA6+ X-Received: by 2002:a17:902:5857:: with SMTP id f23-v6mr12293458plj.206.1532339628304; Mon, 23 Jul 2018 02:53:48 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1532339628; cv=none; d=google.com; s=arc-20160816; b=E+6fLLLDfeYfvvRHO1eedHW/KZEFfkRbxvfUlG5cwRl8d2Do5Day+Mndz5D76o2hiS JkqHFtbwqwbi/ypyk4sJN/pSIGI/XwopMSN8LjoCEw0XzgSqGzYtJ4gTFS/hVWyPfyRl 3+Q1w8F1J/0JsNrROQOsOJCYYKGtmNgS9iT2XHXwZqjiPYxs3KJRv/3UPKrLNIjOefBI tLpwovKUtLh0cpF6zTk2ucvDT0ESWr4ojA0OwUmYbvf5EVnyaJycjqRKaYdjI4cwDmdC /xjJUkrsaQ9UjXzkiupxFONo9kHhyLpczhnfrldaWj3nrdr+VnXj7xX8S/EKrjR9pa1F SP6A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:dkim-signature :arc-authentication-results; bh=wRWZD5KrvKdxIX3CP2up024xe1jf7Qn4M6keO8UtdmM=; b=SuXNyWUbm97UwkiW4WpW0o5Os5TimifJ2NppXGaGVcNO2gEUE0nn7fsaUsQyRhaPos CY151jpYaTiS7ggK3MRy5E5XYAJw6nSSlw95FLrAtlNeIYohADuhPuSLs7JHl+ILZwTl Ox/kq+XwMRnOVRfLPj9f5qYhPH20/5Uny76ztsbZe/HqLVkp0GEpLoNgMOJokNTPsmDJ Ueo9jOn36sFj/SrFqZJWYx/ZlXWt1r0he5udp3P5AgAOjb2ObCpnEvxcoZ5jiD3CjdCp XoprtyJFiJZjq9xHaze3VCKxUuFOY7xqrgK+TNjWTawf0udRsg8neT6jAQuTGM0LNNRV 9tAA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Nj5sIIoi; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id f5-v6si8298123pga.340.2018.07.23.02.53.33; Mon, 23 Jul 2018 02:53:48 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Nj5sIIoi; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2388276AbeGWKxI (ORCPT + 99 others); Mon, 23 Jul 2018 06:53:08 -0400 Received: from mail-oi0-f65.google.com ([209.85.218.65]:39279 "EHLO mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388043AbeGWKxH (ORCPT ); Mon, 23 Jul 2018 06:53:07 -0400 Received: by mail-oi0-f65.google.com with SMTP id d189-v6so51811oib.6; Mon, 23 Jul 2018 02:52:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=wRWZD5KrvKdxIX3CP2up024xe1jf7Qn4M6keO8UtdmM=; b=Nj5sIIoiWiaL2tYmiN04mBP593aw8DlEO4DbQ7zBqIZbhUbQHmav3w80QFiir7PX4y RvgWA2OQImszIXFrxs6eVGJIHLyKqKEY1U3iwrqOrYs2fV9lubVTgFg3qq1yqfZj7uDU 5ZhtS1X3oCLOm7pFAyFBSiflHurNTFTp/SSjZas4cFvOyADun6OwE8I8OTJE3Sg8qV4I A+O4t+gRAPmf2EQtEtuKyDMZQPRMVVladBP7QiWfIUdnPo0hHnAwTmfFmgLeR6tpkFts q9n4xbv79UWQGR9jCIEZqA3/xg8zAo7ljrA2PjMcw+pe06FrBIA37PweOA0JH4qeQepI LChA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=wRWZD5KrvKdxIX3CP2up024xe1jf7Qn4M6keO8UtdmM=; b=rOcW8mNJj8ixXDSB1+kywt2xhBvQpsodeCHouAq8VXnSHIuC4hJTtzkyanWbWM7rRt s+Xs+sYg5zbbIPvjJyZ2F8QbP4mAzh7UBarbpOlNW5U3ifqqAcRqb+9Y+bomIspZ1cyu 2L4Qr/zp+nsYDbl6ZrvL+Lslu87qpZKw1bV1MYxJBhX3aJHBLADibJDJHBhHO8CQu6Kr huRvPwakCNamDEvJcX6Osbc0gVk6QSAe60PvJNfwd/SG0avk4jKHf4ZkfAh+ZR5OjX2I lNzOrNcm23LAeetthabkkL5AsrcKhqwwV4D+n6yd1lNPweOTP9sar1VTASGI2g5sd/1x /psQ== X-Gm-Message-State: AOUpUlEzHtnCQrxdkSDtyjMtlMoqxdHIRqUtkBN/B9a8Xwu+QFdtxMyd OdzjlzRBvrfVzEDb7CNZwrpygd4prPSCPuEZ1SE= X-Received: by 2002:aca:56d1:: with SMTP id k200-v6mr7104476oib.319.1532339564484; Mon, 23 Jul 2018 02:52:44 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac9:ca5:0:0:0:0:0 with HTTP; Mon, 23 Jul 2018 02:52:43 -0700 (PDT) In-Reply-To: <20180723092818.ztsfsnqzxgzrauim@breakpoint.cc> References: <20180722143354.23722-1-cscnull@gmail.com> <20180722163925.gdfkndldatsoae6x@breakpoint.cc> <20180722.100755.19840167505550163.davem@davemloft.net> <20180722180910.wcwhantwpm2nfxet@breakpoint.cc> <20180723091551.mwhltw4ujm4bylvj@salvia> <20180723092818.ztsfsnqzxgzrauim@breakpoint.cc> From: shaochun chen Date: Mon, 23 Jul 2018 17:52:44 +0800 Message-ID: Subject: Re: [PATCH] netlink: fix memory leak of dump To: Florian Westphal Cc: Pablo Neira Ayuso , David Miller , kadlec , "johannes.berg" , jason , ktkhai , "lucien.xin" , "xiyou.wangcong" , dsahern , netfilter-devel , tom , netdev , linux-kernel Content-Type: multipart/alternative; boundary="0000000000006a9e5c0571a79bfd" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --0000000000006a9e5c0571a79bfd Content-Type: text/plain; charset="UTF-8" allocate memory in ->start(), it's not convenient for users. if call ->done() isn't ok for clean memory when netlink_dump_start() fail, maybe we should have another function ->clean() to clean memory. 2018-07-23 17:28 GMT+08:00 Florian Westphal : > Pablo Neira Ayuso wrote: > > > diff --git a/net/netfilter/nf_tables_api.c > b/net/netfilter/nf_tables_api.c > > > --- a/net/netfilter/nf_tables_api.c > > > +++ b/net/netfilter/nf_tables_api.c > > > @@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr * > const nla[]) > > > return filter; > > > } > > > > > > +static int nf_tables_dump_obj_start(struct netlink_callback *cb) > > > +{ > > > + const struct nlattr * const *nla = cb->data; > > On-Stack input. > I can't see how its wrong, ->start() happens from same context as > netlink_dump_start so its valid. > > > > + struct nft_obj_filter *filter = NULL; > > > + > > > + if (nla[NFTA_OBJ_TABLE] || > > > + nla[NFTA_OBJ_TYPE]) { > > > + filter = nft_obj_filter_alloc(nla); > > > + if (IS_ERR(filter)) > > > + return -ENOMEM; > > > + } > > > + > > > + cb->data = filter; > > And this replaced the on-stack input with dynamically > allocated one, which will be free'd via ->done(). > > > > /* called with rcu_read_lock held */ > > > static int nf_tables_getobj(struct net *net, struct sock *nlsk, > > > struct sk_buff *skb, const struct nlmsghdr > *nlh, > > > @@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net, > struct sock *nlsk, > > > > > > if (nlh->nlmsg_flags & NLM_F_DUMP) { > > > struct netlink_dump_control c = { > > > + .start = nf_tables_dump_obj_start, > > > .dump = nf_tables_dump_obj, > > > .done = nf_tables_dump_obj_done, > > > .module = THIS_MODULE, > > > + .data = (void *)nla, > > > > You cannot do this. > > > > nla is allocated in this stack. > > Yes. > > > the nla will not be available in the second recv(), it won't be there. > > Its replaced in ->start(). > > As David pointed out, once ->start() returns 0 we set cb_running, i.e. > only after successful ->start() netlink core will call ->dump() again. > > So I see no problem setting ->data to onstack cookie and then > duplicating it to heap via kmemdup in ->start(). > > As far as I can see netlink core offers all functionality already, > so we only need to switch netfilter to make use of it. > > If you disagree please let me know, otherwise I will cook up > a patch along this pattern for net/netfilter/*. > > Thanks. > --0000000000006a9e5c0571a79bfd Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
allocate memory in ->start(),=C2=A0 it's not conven= ient for users.
if call ->done() isn't ok for clean memory when = netlink_dump_start() fail,
maybe we should have another function = ->clean() to clean memory.

2018-07-23 17:28 GMT+08:00 Florian Westphal <fw@strlen.= de>:
Pablo= Neira Ayuso <pablo@netfilter.org= > wrote:
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/n= f_tables_api.c
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr *= const nla[])
> >=C2=A0 =C2=A0 =C2=A0return filter;
> >=C2=A0 }
> >=C2=A0
> > +static int nf_tables_dump_obj_start(struct netlink_callback= *cb)
> > +{
> > +=C2=A0 =C2=A0const struct nlattr * const *nla =3D cb->data;
On-Stack input.
I can't see how its wrong, ->start() happens from same context as netlink_dump_start so its valid.

> > +=C2=A0 =C2=A0struct nft_obj_filter *filter =3D NULL;
> > +
> > +=C2=A0 =C2=A0if (nla[NFTA_OBJ_TABLE] ||
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0nla[NFTA_OBJ_TYPE]) {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0filter =3D nft_obj_filt= er_alloc(nla);
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (IS_ERR(filter))
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0return -ENOMEM;
> > +=C2=A0 =C2=A0}
> > +
> > +=C2=A0 =C2=A0cb->data =3D filter;

And this replaced the on-stack input with dynamically
allocated one, which will be free'd via ->done().

> >=C2=A0 /* called with rcu_read_lock held */
> >=C2=A0 static int nf_tables_getobj(struct net *net, struct sock *n= lsk,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0struct sk_buff *skb, const struct nlmsghdr *nlh, > > @@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *n= et, struct sock *nlsk,
> >=C2=A0
> >=C2=A0 =C2=A0 =C2=A0if (nlh->nlmsg_flags & NLM_F_DUMP) { > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct netlink_dum= p_control c =3D {
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0.start =3D nf_tables_dump_obj_start,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0.dump =3D nf_tables_dump_obj,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0.done =3D nf_tables_dump_obj_done,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0.module =3D THIS_MODULE,
> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0.data =3D (void *)nla,
>
> You cannot do this.
>
> nla is allocated in this stack.

Yes.

> the nla will not be available in the second recv(), it won't be th= ere.

Its replaced in ->start().

As David pointed out, once ->start() returns 0 we set cb_running, i.e. only after successful ->start() netlink core will call ->dump() again= .

So I see no problem setting ->data to onstack cookie and then
duplicating it to heap via kmemdup in ->start().

As far as I can see netlink core offers all functionality already,
so we only need to switch netfilter to make use of it.

If you disagree please let me know, otherwise I will cook up
a patch along this pattern for net/netfilter/*.

Thanks.

--0000000000006a9e5c0571a79bfd--