[lkml]   [2018]   [Jul]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patch in this message
SubjectRe: [PATCH] netlink: fix memory leak of dump
<div dir="ltr">allocate memory in -&gt;start(),  it&#39;s not convenient for users.<div>if call -&gt;done() isn&#39;t ok for clean memory when netlink_dump_start() fail,</div><div>maybe we should have another function -&gt;clean() to clean memory.</div></div><div class="gmail_extra"><br><div class="gmail_quote">2018-07-23 17:28 GMT+08:00 Florian Westphal <span dir="ltr">&lt;<a href="" target="_blank"></a>&gt;</span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Pablo Neira Ayuso &lt;<a href=""></a>&gt; wrote:<br>
&gt; &gt; diff --git a/net/netfilter/nf_tables_api.<wbr>c b/net/netfilter/nf_tables_api.<wbr>c<br>
&gt; &gt; --- a/net/netfilter/nf_tables_api.<wbr>c<br>
&gt; &gt; +++ b/net/netfilter/nf_tables_api.<wbr>c<br>
&gt; &gt; @@ -5010,6 +5013,22 @@ nft_obj_filter_alloc(const struct nlattr * const nla[])<br>
&gt; &gt;     return filter;<br>
&gt; &gt;  }<br>
&gt; &gt;  <br>
&gt; &gt; +static int nf_tables_dump_obj_start(<wbr>struct netlink_callback *cb)<br>
&gt; &gt; +{<br>
&gt; &gt; +   const struct nlattr * const *nla = cb-&gt;data;<br>
</span>On-Stack input.<br>
I can&#39;t see how its wrong, -&gt;start() happens from same context as<br>
netlink_dump_start so its valid.<br>
<span class=""><br>
&gt; &gt; +   struct nft_obj_filter *filter = NULL;<br>
&gt; &gt; +<br>
&gt; &gt; +   if (nla[NFTA_OBJ_TABLE] ||<br>
&gt; &gt; +       nla[NFTA_OBJ_TYPE]) {<br>
&gt; &gt; +           filter = nft_obj_filter_alloc(nla);<br>
&gt; &gt; +           if (IS_ERR(filter))<br>
&gt; &gt; +                   return -ENOMEM;<br>
&gt; &gt; +   }<br>
&gt; &gt; +<br>
&gt; &gt; +   cb-&gt;data = filter;<br>
</span>And this replaced the on-stack input with dynamically<br>
allocated one, which will be free&#39;d via -&gt;done().<br>
<span class=""><br>
&gt; &gt;  /* called with rcu_read_lock held */<br>
&gt; &gt;  static int nf_tables_getobj(struct net *net, struct sock *nlsk,<br>
&gt; &gt;                         struct sk_buff *skb, const struct nlmsghdr *nlh,<br>
&gt; &gt; @@ -5028,21 +5047,13 @@ static int nf_tables_getobj(struct net *net, struct sock *nlsk,<br>
&gt; &gt;  <br>
&gt; &gt;     if (nlh-&gt;nlmsg_flags &amp; NLM_F_DUMP) {<br>
&gt; &gt;             struct netlink_dump_control c = {<br>
&gt; &gt; +                   .start = nf_tables_dump_obj_start,<br>
&gt; &gt;                     .dump = nf_tables_dump_obj,<br>
&gt; &gt;                     .done = nf_tables_dump_obj_done,<br>
&gt; &gt;                     .module = THIS_MODULE,<br>
&gt; &gt; +                   .data = (void *)nla,<br>
&gt; <br>
&gt; You cannot do this.<br>
&gt; <br>
&gt; nla is allocated in this stack.<br>
<span class=""><br>
&gt; the nla will not be available in the second recv(), it won&#39;t be there.<br>
</span>Its replaced in -&gt;start().<br>
As David pointed out, once -&gt;start() returns 0 we set cb_running, i.e.<br>
only after successful -&gt;start() netlink core will call -&gt;dump() again.<br>
So I see no problem setting -&gt;data to onstack cookie and then<br>
duplicating it to heap via kmemdup in -&gt;start().<br>
As far as I can see netlink core offers all functionality already,<br>
so we only need to switch netfilter to make use of it.<br>
If you disagree please let me know, otherwise I will cook up<br>
a patch along this pattern for net/netfilter/*.<br>
 \ /
  Last update: 2018-07-23 11:53    [W:0.088 / U:2.692 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site