From mboxrd@z Thu Jan 1 00:00:00 1970 From: shaochun chen Subject: Re: [PATCH] netlink: fix memory leak of dump Date: Mon, 23 Jul 2018 10:05:18 +0800 Message-ID: References: <20180722143354.23722-1-cscnull@gmail.com> <20180722163925.gdfkndldatsoae6x@breakpoint.cc> <20180722.100755.19840167505550163.davem@davemloft.net> <20180722180910.wcwhantwpm2nfxet@breakpoint.cc> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="000000000000c682c60571a113f2" Cc: David Miller , pablo , kadlec , "johannes.berg" , jason , ktkhai , "lucien.xin" , "xiyou.wangcong" , dsahern , netfilter-devel , tom , netdev , linux-kernel To: Florian Westphal Return-path: In-Reply-To: <20180722180910.wcwhantwpm2nfxet@breakpoint.cc> Sender: linux-kernel-owner@vger.kernel.org List-Id: netfilter-devel.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--