lkml.org 
[lkml]   [2018]   [Jul]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH] netlink: fix memory leak of dump
<div dir="ltr">I have a question: we will try_module_get in <span style="color:rgb(0,0,0);font-size:13.3333px">__netlink_dump_start(),</span><div><span style="color:rgb(0,0,0);font-size:13.3333px">but why we need to call <span style="color:rgb(34,34,34);font-size:14px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><span>try_module_get again in 

<span style="color:rgb(0,0,0);font-size:13.3333px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><span> </span></span><span style="text-decoration-style:initial;text-decoration-color:initial;background-color:rgb(255,255,255);float:none;display:inline">nft_netlink_dump_start<span> </span></span>??</span></span></span></div></div><div class="gmail_extra"><br><div class="gmail_quote">2018-07-23 17:52 GMT+08:00 shaochun chen <span dir="ltr">&lt;<a href="mailto:cscnull@gmail.com" target="_blank">cscnull@gmail.com</a>&gt;</span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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="HOEnZb"><div class="h5"><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="mailto:fw@strlen.de" target="_blank">fw@strlen.de</a>&gt;</span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>Pablo Neira Ayuso &lt;<a href="mailto:pablo@netfilter.org" target="_blank">pablo@netfilter.org</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(struc<wbr>t netlink_callback *cb)<br>
&gt; &gt; +{<br>
&gt; &gt; +   const struct nlattr * const *nla = cb-&gt;data;<br>
<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><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>
<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><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>
<br>
</span>Yes.<br>
<span><br>
&gt; the nla will not be available in the second recv(), it won&#39;t be there.<br>
<br>
</span>Its replaced in -&gt;start().<br>
<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>
<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>
<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>
<br>
If you disagree please let me know, otherwise I will cook up<br>
a patch along this pattern for net/netfilter/*.<br>
<br>
Thanks.<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>
\
 
 \ /
  Last update: 2018-07-23 12:35    [W:0.096 / U:3.760 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site