Received: by 2002:ac0:a5a7:0:0:0:0:0 with SMTP id m36-v6csp5310443imm; Sun, 22 Jul 2018 19:06:27 -0700 (PDT) X-Google-Smtp-Source: AAOMgpcjEdp9u+4erkhe6vPb7sGnHthrX2VU3iBYaAlOb+DYpd5UYTKYEiaEsYGhZ2Qqu9iDY1k2 X-Received: by 2002:a62:c0a:: with SMTP id u10-v6mr11263086pfi.43.1532311587659; Sun, 22 Jul 2018 19:06:27 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1532311587; cv=none; d=google.com; s=arc-20160816; b=hini2VpfTNV/Ek8GuNXFI2P804GFW4+rG0T7rcW6gNBo2jBcon673NKrX3oYVkxGZZ I1K+WJexQ5YbjTH94QSYZ5dL8/F0CUSzPcU6Kmuw1Ei4UEPMNDYSzrkHZD297G5/bfID Mivh+6Fk2l+HvRzEFdK8ibg0tIbM/xquyRBjSu6BApeax/3V5ebV76i83ySleTcAEpWM KbMLdA2//AQcICyTVedIMhL/ucHdwEyl+BPgHKisJrTjfvB1x0zEu7Dh9z3aetiJzY1K wqo081E/OVLv1yNlu1XS21+jKp/MBbpQQyM0v7wajrAHNk6c/Y9mwxpQikNnkD+2JztW Z3Cw== 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=VuOiO51I/sASSof6yTorInbnglN6U00o1Ejspg86xfY=; b=vNxUcuFVsquFm38RrlZNRTOWgp/7rpR5vBwfqg3MZ4PZPx5KgSO6P/5S+TyJT2OtzI 4IGysfOKockJ5JCOdcegixv69Nf4+ON8E2iV/YFi/r0f5R+2YTgQtdfUUaMpicIXMfW/ xM2OsL2cAX3bM1YswE7EcHG7oi2E3YpqYBQ7bVE/PCUvbE5uxAYV+dwCEgST7KXqlA1w x5Zb+Bn/7/8S5z5FMZIMtkjzG75iuLNMqxFqxH6vbfe14BqjSIQ42NzgmxGY0gxV4HeA OLseLtPiCf+DhkgkSkn0rEPpWsEUryhT4YMEBE/ugnN9yYaT2wfhgwjas/GCxLo4u/s2 iJ1w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=Pf2wXl0H; 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 u11-v6si7559037pgg.683.2018.07.22.19.06.12; Sun, 22 Jul 2018 19:06:27 -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=Pf2wXl0H; 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 S1731282AbeGWDEH (ORCPT + 99 others); Sun, 22 Jul 2018 23:04:07 -0400 Received: from mail-oi0-f67.google.com ([209.85.218.67]:46791 "EHLO mail-oi0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731162AbeGWDEH (ORCPT ); Sun, 22 Jul 2018 23:04:07 -0400 Received: by mail-oi0-f67.google.com with SMTP id y207-v6so30555968oie.13; Sun, 22 Jul 2018 19:05:19 -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=VuOiO51I/sASSof6yTorInbnglN6U00o1Ejspg86xfY=; b=Pf2wXl0Hybl7JydSMaGu2Q1bKEpuBHlThKxi4FAnm68SVTaUgiezDG1lyER1JPJuWB 1NDJyCfWXVBUbuRLOsK1NHNZevBXlsGTs3t/B3fQoSmy7IdHC8hpE1D/fr++2sXjzWbH enlNNqLgxZLrL8+fqeTAA5hFwca/uNEB8k9gt1capkRVpe6XpJTvrq1iyIKXWYgLG0T5 kwjKUQxLmdSB/55QSfOhzkBE2l+XQM7qefToKndvq3GSGuXI/PePSA8p8bKkO1Pxv/ga +6aOthgMKsjnRRg7kO/NZEYFO8H6OrrwWGbET+zQbz94ld4O1zTSBTM8ndcflVGTSg/4 3/Pw== 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=VuOiO51I/sASSof6yTorInbnglN6U00o1Ejspg86xfY=; b=P0vRukKyouSlYWvMs7V2jTsxxpXcwQwhNP+FKKZdqLZxr9np6w5+vzTDA/9OLC+dX/ vIjKQPLG8eSNwJbu+7+jyAsJsNq5Jodn9CWcCFze5Pr7oFgqtzhuu/KLLlBnu94md6x4 YL1c1/cFxsTZGNxizp9USnjmNl+nh1bEwXeLAYLWUoR8rKHz8dgzlmuEjXsOiM9GPnQe AWqC6KWkloHg9vPzckpF/jB0YcYEMm0Pk9wU/zsP3a4gw5htkXRw3unxLyUzJGXJdgFz ThkQll4NfcFz1Q6mcYiLQuOoEU2gte0n2JVXXdyGJQ4TPzc9zC4PzlkWwcI/LZmCB0Ji SVAg== X-Gm-Message-State: AOUpUlGQ19+VKg05qP8pev+JT1+Znl5DmkvOV4a1OMFPnhDkTQdbicAq d13jl0zMCRw0u18mLxPp5NP3gTHpYhQcS5+sB/s= X-Received: by 2002:aca:afcc:: with SMTP id y195-v6mr7214124oie.322.1532311519000; Sun, 22 Jul 2018 19:05:19 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac9:ca5:0:0:0:0:0 with HTTP; Sun, 22 Jul 2018 19:05:18 -0700 (PDT) In-Reply-To: <20180722180910.wcwhantwpm2nfxet@breakpoint.cc> References: <20180722143354.23722-1-cscnull@gmail.com> <20180722163925.gdfkndldatsoae6x@breakpoint.cc> <20180722.100755.19840167505550163.davem@davemloft.net> <20180722180910.wcwhantwpm2nfxet@breakpoint.cc> From: shaochun chen Date: Mon, 23 Jul 2018 10:05:18 +0800 Message-ID: Subject: Re: [PATCH] netlink: fix memory leak of dump To: Florian Westphal Cc: David Miller , pablo , kadlec , "johannes.berg" , jason , ktkhai , "lucien.xin" , "xiyou.wangcong" , dsahern , netfilter-devel , tom , netdev , linux-kernel Content-Type: multipart/alternative; boundary="000000000000c682c60571a113f2" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --000000000000c682c60571a113f2 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable allocate memory in cb->start(), which means passing 'static' variable through control->data, then allocate memory in cb->start() according to cb->data (cb->data is equal to control->data now), and set the memory back to cb->data which will be used in cb->dump(). It's a bit complicated=EF=BC=8C please see nf_tables_getset. 2018-07-23 2:09 GMT+08:00 Florian Westphal : > David Miller wrote: > > From: Florian Westphal > > Date: Sun, 22 Jul 2018 18:39:25 +0200 > > > > > 3. change meaning of ->done() so its always called once ->start() > > > was invoked (and returned 0), this requires audit of all > > > places that provide .done to make sure they won't trip. > > > > > > 3) seems to be what Tom intended when he added .start, so probably > > > best to investigate that first. > > > > Hmmm... > > > > Any time ->start() succeeds, we set cb_running to true. > > Right. > > > From that point forward, ->done() will be called at some point at all > > of the locations that check if cb_running is true and set it to false. > > Also right, thanks for pointing this out, I missed fact that netlink > core restarts a dump after this. > > So 3) is already true which means we should try to see if we can move > all dump-related extra magic into ->start(). > > Shaochun, can you see if this is possible? > > Something along these lines (totally untested), which makes this > a netfilter fix: > > 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 =3D cb->data; > + struct nft_obj_filter *filter =3D NULL; > + > + if (nla[NFTA_OBJ_TABLE] || > + nla[NFTA_OBJ_TYPE]) { > + filter =3D nft_obj_filter_alloc(nla); > + if (IS_ERR(filter)) > + return -ENOMEM; > + } > + > + cb->data =3D filter; > + return 0; > +} > + > /* 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 =3D { > + .start =3D nf_tables_dump_obj_start, > .dump =3D nf_tables_dump_obj, > .done =3D nf_tables_dump_obj_done, > .module =3D THIS_MODULE, > + .data =3D (void *)nla, > }; > > - if (nla[NFTA_OBJ_TABLE] || > - nla[NFTA_OBJ_TYPE]) { > - struct nft_obj_filter *filter; > - > - filter =3D nft_obj_filter_alloc(nla); > - if (IS_ERR(filter)) > - return -ENOMEM; > - > - c.data =3D filter; > - } > return nft_netlink_dump_start_rcu(nlsk, skb, nlh, &c); > } > > --000000000000c682c60571a113f2 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
allocate memory in cb->start(), which means passing = 9;static' variable through control->data,=C2=A0
then allocate me= mory in cb->start() according to cb->data (cb->data is equal to co= ntrol->data now),
and set the memory back to cb->data which= will be used in cb->dump().=C2=A0
It's a bit complicated= =EF=BC=8C please see=C2=A0nf_tables_getset.

2018-07-23 2:09 GMT+08:00 Florian Wes= tphal <fw@strlen.de>:
David Miller <davem@dave= mloft.net> wrote:
> From: Florian Westphal <fw@strlen.d= e>
> Date: Sun, 22 Jul 2018 18:39:25 +0200
>
> > 3. change meaning of ->done() so its always called once ->s= tart()
> >=C2=A0 =C2=A0 was invoked (and returned 0), this requires audit of= all
> >=C2=A0 =C2=A0 places that provide .done to make sure they won'= t trip.
> >
> > 3) seems to be what Tom intended when he added .start, so probabl= y
> > best to investigate that first.
>
> Hmmm...
>
> Any time ->start() succeeds, we set cb_running to true.

Right.

> From that point forward, ->done() will be called at some point at a= ll
> of the locations that check if cb_running is true and set it to false.=

Also right, thanks for pointing this out, I missed fact that netlink=
core restarts a dump after this.

So 3) is already true which means we should try to see if we can move
all dump-related extra magic into ->start().

Shaochun, can you see if this is possible?

Something along these lines (totally untested), which makes this
a netfilter fix:

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_a= pi.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=A0 =C2=A0 return filter;
=C2=A0}

+static int nf_tables_dump_obj_start(struct netlink_callback *cb)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0const struct nlattr * const *nla =3D cb->dat= a;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct nft_obj_filter *filter = =3D NULL;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (nla[NFTA_OBJ_TABLE] ||
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0nla[NFTA_OBJ_TYPE]) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0filter =3D nft_obj_= filter_alloc(nla);
+=C2=A0 =C2=A0 =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=A0 =C2= =A0 =C2=A0return -ENOMEM;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0cb->data =3D filter;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return 0;
+}
+
=C2=A0/* called with rcu_read_lock held */
=C2=A0static int nf_tables_getobj(struct net *net, struct = sock *nlsk,
=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=A0 =C2=A0 struct sk_buff *skb, const struct nlmsghdr = *nlh,
@@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net, struct= sock *nlsk,

=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (nlh->nlmsg_flags & NLM_F_DUMP) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 struct netlink_dump= _control c =3D {
+=C2=A0 =C2=A0 =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 =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 =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 =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 =C2=A0 =C2=A0.data =3D (void *)nla,
=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=A0 =C2=A0 =C2=A0 =C2=A0if (nla[NFTA_OBJ_TA= BLE] ||
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0nla[N= FTA_OBJ_TYPE]) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0struct nft_obj_filter *filter;
-
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0filter =3D nft_obj_filter_alloc(nla);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =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=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENOMEM;
-
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0c.data =3D filter;
-=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=A0 =C2=A0 =C2=A0 =C2=A0 return nft_netlink_= dump_start_rcu(nlsk, skb, nlh, &c);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }


--000000000000c682c60571a113f2--