Re:Re: [PATCH net] net: Fix one possible memleak in ip_setup_cork

[Gao Feng]

At 2018-04-17 05:18:25, "Eric Dumazet"  wrote:
>
>
>On 04/16/2018 09:58 AM, David Miller wrote:
>> From: gfree.w...@vip.163.com
>> Date: Mon, 16 Apr 2018 10:16:45 +0800
>> 
>>> From: Gao Feng 
>>>
>>> It would allocate memory in this function when the cork->opt is NULL. But
>>> the memory isn't freed if failed in the latter rt check, and return error
>>> directly. It causes the memleak if its caller is ip_make_skb which also
>>> doesn't free the cork->opt when meet a error.
>>>
>>> Now move the rt check ahead to avoid the memleak.
>>>
>>> Signed-off-by: Gao Feng 
>> 
>> Looks good, applied and queued up for -stable.
>> 
>> I guess in the other code paths, ip_flush_pending_frames() or similar
>> would clean up the in-sock cork information.
>> 
>
>I am not sure ip_make_skb() can be called with a NULL rt.
>
>Patch makes no harm, but does not seem to fix a bug.
>

Thanks Eric.

I just look up current all callers of ip_make_skb and ip_append_data, they check
if the rt is valid ahead. So current codes won't pass one NULL rt to 
ip_setup_cork indeed.

Then this patch is just as an enhancement, not a fix. 
As the programming rule, the function should free the mem which is allocated by 
itself when
it failed.

Best Regards
Feng

Re: [PATCH net] net: Fix one possible memleak in ip_setup_cork

[Eric Dumazet]

On 04/16/2018 09:58 AM, David Miller wrote:
> From: gfree.w...@vip.163.com
> Date: Mon, 16 Apr 2018 10:16:45 +0800
> 
>> From: Gao Feng 
>>
>> It would allocate memory in this function when the cork->opt is NULL. But
>> the memory isn't freed if failed in the latter rt check, and return error
>> directly. It causes the memleak if its caller is ip_make_skb which also
>> doesn't free the cork->opt when meet a error.
>>
>> Now move the rt check ahead to avoid the memleak.
>>
>> Signed-off-by: Gao Feng 
> 
> Looks good, applied and queued up for -stable.
> 
> I guess in the other code paths, ip_flush_pending_frames() or similar
> would clean up the in-sock cork information.
> 

I am not sure ip_make_skb() can be called with a NULL rt.

Patch makes no harm, but does not seem to fix a bug.

Re: [PATCH net] net: Fix one possible memleak in ip_setup_cork

[David Miller]

From: gfree.w...@vip.163.com
Date: Mon, 16 Apr 2018 10:16:45 +0800

> From: Gao Feng 
> 
> It would allocate memory in this function when the cork->opt is NULL. But
> the memory isn't freed if failed in the latter rt check, and return error
> directly. It causes the memleak if its caller is ip_make_skb which also
> doesn't free the cork->opt when meet a error.
> 
> Now move the rt check ahead to avoid the memleak.
> 
> Signed-off-by: Gao Feng 

Looks good, applied and queued up for -stable.

I guess in the other code paths, ip_flush_pending_frames() or similar
would clean up the in-sock cork information.

Re:Re: [PATCH net] net: Fix one possible memleak in ip_setup_cork

[Gao Feng]

At 2018-04-16 10:55:56, "David Miller"  wrote:
>From: gfree.w...@vip.163.com
>Date: Mon, 16 Apr 2018 10:16:45 +0800
>
>> From: Gao Feng 
>> 
>> It would allocate memory in this function when the cork->opt is NULL. But
>> the memory isn't freed if failed in the latter rt check, and return error
>> directly. It causes the memleak if its caller is ip_make_skb which also
>> doesn't free the cork->opt when meet a error.
>> 
>> Now move the rt check ahead to avoid the memleak.
>> 
>> Signed-off-by: Gao Feng 
>
>Why did you post this patch twice?

Sorry, it is my input error. I typed "yes" not "all" at the first time when 
execute git-send-email.
Then I corrected it as the second time.

Best Regards
Feng

Re: [PATCH net] net: Fix one possible memleak in ip_setup_cork

[David Miller]

From: gfree.w...@vip.163.com
Date: Mon, 16 Apr 2018 10:16:45 +0800

> From: Gao Feng 
> 
> It would allocate memory in this function when the cork->opt is NULL. But
> the memory isn't freed if failed in the latter rt check, and return error
> directly. It causes the memleak if its caller is ip_make_skb which also
> doesn't free the cork->opt when meet a error.
> 
> Now move the rt check ahead to avoid the memleak.
> 
> Signed-off-by: Gao Feng 

Why did you post this patch twice?

[PATCH net] net: Fix one possible memleak in ip_setup_cork

[gfree . wind]

From: Gao Feng 

It would allocate memory in this function when the cork->opt is NULL. But
the memory isn't freed if failed in the latter rt check, and return error
directly. It causes the memleak if its caller is ip_make_skb which also
doesn't free the cork->opt when meet a error.

Now move the rt check ahead to avoid the memleak.

Signed-off-by: Gao Feng 
---
 net/ipv4/ip_output.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 4c11b81..83c73ba 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1109,6 +1109,10 @@ static int ip_setup_cork(struct sock *sk, struct 
inet_cork *cork,
struct ip_options_rcu *opt;
struct rtable *rt;
 
+   rt = *rtp;
+   if (unlikely(!rt))
+   return -EFAULT;
+
/*
 * setup for corking.
 */
@@ -1124,9 +1128,7 @@ static int ip_setup_cork(struct sock *sk, struct 
inet_cork *cork,
cork->flags |= IPCORK_OPT;
cork->addr = ipc->addr;
}
-   rt = *rtp;
-   if (unlikely(!rt))
-   return -EFAULT;
+
/*
 * We steal reference to this route, caller should not release it
 */
-- 
1.9.1

[PATCH net] net: Fix one possible memleak in ip_setup_cork

[gfree . wind]

From: Gao Feng 

It would allocate memory in this function when the cork->opt is NULL. But
the memory isn't freed if failed in the latter rt check, and return error
directly. It causes the memleak if its caller is ip_make_skb which also
doesn't free the cork->opt when meet a error.

Now move the rt check ahead to avoid the memleak.

Signed-off-by: Gao Feng 
---
 net/ipv4/ip_output.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 4c11b81..83c73ba 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1109,6 +1109,10 @@ static int ip_setup_cork(struct sock *sk, struct 
inet_cork *cork,
struct ip_options_rcu *opt;
struct rtable *rt;
 
+   rt = *rtp;
+   if (unlikely(!rt))
+   return -EFAULT;
+
/*
 * setup for corking.
 */
@@ -1124,9 +1128,7 @@ static int ip_setup_cork(struct sock *sk, struct 
inet_cork *cork,
cork->flags |= IPCORK_OPT;
cork->addr = ipc->addr;
}
-   rt = *rtp;
-   if (unlikely(!rt))
-   return -EFAULT;
+
/*
 * We steal reference to this route, caller should not release it
 */
-- 
1.9.1