[PATCH net-next 0/3] Three possible UDP fixes.

[PATCH net-next 0/3] Three possible UDP fixes.

[Paul Gortmaker]

These were originally found on a 2.6.34 baseline, but I looked
at them and couldn't see any reason why they wouldn't be valid
fixes on net-next.  But I'll feel better when someone like
Dave and/or Eric sanity checks them too.

There was one thing that was a consideration.  In the 3rd patch,
where we clear MSG_TRUNC bit -- is there anything in there that
we really need to be concerned about preserving on the retry,
or could we just unconditionally do "msg->msg_flags = 0" ?
I wasn't sure, and so sticking with clearing the offending bit
seemed like the most cautious approach.

Thanks,
Paul.

---

Mark Asselstine (1):
  net: ipv4: fix potential memory leak by assigning uhash_entries

Xufeng Zhang (2):
  ipv6/udp: Use the correct variable to determine non-blocking condition
  udp/recvmsg: Clear MSG_TRUNC flag when starting over for a new packet

 net/ipv4/udp.c |    5 ++++-
 net/ipv6/udp.c |    5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
1.7.4.4

[PATCH net-next 1/3] net: ipv4: fix potential memory leak by assigning uhash_entries

[Paul Gortmaker]

From: Mark Asselstine <mark.asselstine@windriver.com>

Commit f86dcc5a [udp: dynamically size hash tables at boot time]
introduced the uhash_entries boot option and made sure to keep
it set within acceptable limits -- if used.  It did not assign a
default value, however, so it defaults to zero.  This results in
alloc_large_system_hash() being relied upon to specify an acceptable
number of hash entries, something it can't be relied on to always do
correctly. For example, when it fails to set an acceptable minimum
(UDP_HTABLE_SIZE_MIN) we get a second allocation and a memory leak.
So we need to set a default value for uhash_entries to ensure we get
the required minimum and prevent a second allocation.

This was found by using DEBUG_KMEMLEAK, producing the following log:

unreferenced object 0xc1b0d000 (size 4096):
  comm "swapper", pid 1, jiffies 4294667562 (age 136.225s)
  hex dump (first 32 bytes):
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  backtrace:
    [<c10e9027>] create_object+0xd7/0x210
    [<c15d73d7>] kmemleak_alloc+0x27/0x50
    [<c1877032>] alloc_large_system_hash+0x16d/0x1f7
    [<c189121d>] udp_table_init+0x43/0xf8
    [<c18912e4>] udp_init+0x12/0x74
    [<c1891637>] inet_init+0x179/0x250
    [<c10011f0>] do_one_initcall+0x30/0x160
    [<c18607c9>] kernel_init+0xb9/0x14e
    [<c15fcff6>] kernel_thread_helper+0x6/0xd
    [<ffffffff>] 0xffffffff

This is fairly easy to reproduce using ARCH=x86 defconfig (i386_defconfig)
enabling DEBUG_KMEMLEAK and running on a system with 32MB of memory
(qemu -m 32). With systems with larger amounts of memory we may not
see this leak since the logic in alloc_large_system_hash() will result
in a large enough (>UDP_HTABLE_SIZE_MIN) number of entries being set.

Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 net/ipv4/udp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index abca870..6f53a5a 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -2155,7 +2155,7 @@ void udp4_proc_exit(void)
 }
 #endif /* CONFIG_PROC_FS */
 
-static __initdata unsigned long uhash_entries;
+static __initdata unsigned long uhash_entries = UDP_HTABLE_SIZE_MIN;
 static int __init set_uhash_entries(char *str)
 {
 	if (!str)
-- 
1.7.4.4

Re: [PATCH net-next 1/3] net: ipv4: fix potential memory leak by assigning uhash_entries

[Eric Dumazet]

Le mardi 21 juin 2011 à 16:43 -0400, Paul Gortmaker a écrit :
> From: Mark Asselstine <mark.asselstine@windriver.com>
> 
> Commit f86dcc5a [udp: dynamically size hash tables at boot time]
> introduced the uhash_entries boot option and made sure to keep
> it set within acceptable limits -- if used.  It did not assign a
> default value, however, so it defaults to zero.  This results in
> alloc_large_system_hash() being relied upon to specify an acceptable
> number of hash entries, something it can't be relied on to always do
> correctly. For example, when it fails to set an acceptable minimum
> (UDP_HTABLE_SIZE_MIN) we get a second allocation and a memory leak.
> So we need to set a default value for uhash_entries to ensure we get
> the required minimum and prevent a second allocation.
> 
> This was found by using DEBUG_KMEMLEAK, producing the following log:
> 
> unreferenced object 0xc1b0d000 (size 4096):
>   comm "swapper", pid 1, jiffies 4294667562 (age 136.225s)
>   hex dump (first 32 bytes):
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<c10e9027>] create_object+0xd7/0x210
>     [<c15d73d7>] kmemleak_alloc+0x27/0x50
>     [<c1877032>] alloc_large_system_hash+0x16d/0x1f7
>     [<c189121d>] udp_table_init+0x43/0xf8
>     [<c18912e4>] udp_init+0x12/0x74
>     [<c1891637>] inet_init+0x179/0x250
>     [<c10011f0>] do_one_initcall+0x30/0x160
>     [<c18607c9>] kernel_init+0xb9/0x14e
>     [<c15fcff6>] kernel_thread_helper+0x6/0xd
>     [<ffffffff>] 0xffffffff
> 
> This is fairly easy to reproduce using ARCH=x86 defconfig (i386_defconfig)
> enabling DEBUG_KMEMLEAK and running on a system with 32MB of memory
> (qemu -m 32). With systems with larger amounts of memory we may not
> see this leak since the logic in alloc_large_system_hash() will result
> in a large enough (>UDP_HTABLE_SIZE_MIN) number of entries being set.
> 
> Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  net/ipv4/udp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index abca870..6f53a5a 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -2155,7 +2155,7 @@ void udp4_proc_exit(void)
>  }
>  #endif /* CONFIG_PROC_FS */
>  
> -static __initdata unsigned long uhash_entries;
> +static __initdata unsigned long uhash_entries = UDP_HTABLE_SIZE_MIN;
>  static int __init set_uhash_entries(char *str)
>  {
>  	if (!str)

Arg no, I really wanted to get more hash slots in my 32bit machines,
with 4Gbytes of memory.

Here is what I currently have (without your patch)

[    1.903086] UDP hash table entries: 512 (order: 2, 16384 bytes)


I mean, this kmemleak was already reported.

32MB machines are things of the past.

If you really care, please add a change to alloc_large_system_hash() ?

Re: [PATCH net-next 1/3] net: ipv4: fix potential memory leak by assigning uhash_entries

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 22 Jun 2011 07:04:46 +0200

> Arg no, I really wanted to get more hash slots in my 32bit machines,
> with 4Gbytes of memory.
> 
> Here is what I currently have (without your patch)
> 
> [    1.903086] UDP hash table entries: 512 (order: 2, 16384 bytes)
> 
> 
> I mean, this kmemleak was already reported.
> 
> 32MB machines are things of the past.
> 
> If you really care, please add a change to alloc_large_system_hash() ?

Crap, I thought that argument to alloc_large_system_hash() provided a
lower bound.

Indeed, I'm going to revert this patch.

Re: [PATCH net-next 1/3] net: ipv4: fix potential memory leak by assigning uhash_entries

[Paul Gortmaker]

On 11-06-22 01:04 AM, Eric Dumazet wrote:
> Le mardi 21 juin 2011 à 16:43 -0400, Paul Gortmaker a écrit :

[...]

>> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
>> index abca870..6f53a5a 100644
>> --- a/net/ipv4/udp.c
>> +++ b/net/ipv4/udp.c
>> @@ -2155,7 +2155,7 @@ void udp4_proc_exit(void)
>>  }
>>  #endif /* CONFIG_PROC_FS */
>>  
>> -static __initdata unsigned long uhash_entries;
>> +static __initdata unsigned long uhash_entries = UDP_HTABLE_SIZE_MIN;
>>  static int __init set_uhash_entries(char *str)
>>  {
>>  	if (!str)
> 
> Arg no, I really wanted to get more hash slots in my 32bit machines,
> with 4Gbytes of memory.
> 
> Here is what I currently have (without your patch)
> 
> [    1.903086] UDP hash table entries: 512 (order: 2, 16384 bytes)
> 
> 
> I mean, this kmemleak was already reported.

Ah, I'd not read that thread - here is the link if anyone else
is following along:

http://lists-archives.org/linux-kernel/27460513-kmemleak-for-mips.html

> 
> 32MB machines are things of the past.
> 
> If you really care, please add a change to alloc_large_system_hash() ?

Given the user list (dcache.c, inode.c, pid.c, ...) it probably
isn't worth the churn of adding a min argument to the calls.
I'm fine with dropping this patch (and glad I'd CC'd you on it)

Thanks,
Paul.

> 
> 
> 

[PATCH net-next 2/3] ipv6/udp: Use the correct variable to determine non-blocking condition

[Paul Gortmaker]

From: Xufeng Zhang <xufeng.zhang@windriver.com>

udpv6_recvmsg() function is not using the correct variable to determine
whether or not the socket is in non-blocking operation, this will lead
to unexpected behavior when a UDP checksum error occurs.

Consider a non-blocking udp receive scenario: when udpv6_recvmsg() is
called by sock_common_recvmsg(), MSG_DONTWAIT bit of flags variable in
udpv6_recvmsg() is cleared by "flags & ~MSG_DONTWAIT" in this call:

    err = sk->sk_prot->recvmsg(iocb, sk, msg, size, flags & MSG_DONTWAIT,
                   flags & ~MSG_DONTWAIT, &addr_len);

i.e. with udpv6_recvmsg() getting these values:

	int noblock = flags & MSG_DONTWAIT
	int flags = flags & ~MSG_DONTWAIT

So, when udp checksum error occurs, the execution will go to
csum_copy_err, and then the problem happens:

    csum_copy_err:
            ...............
            if (flags & MSG_DONTWAIT)
                    return -EAGAIN;
            goto try_again;
            ...............

But it will always go to try_again as MSG_DONTWAIT has been cleared
from flags at call time -- only noblock contains the original value
of MSG_DONTWAIT, so the test should be:

            if (noblock)
                    return -EAGAIN;

This is also consistent with what the ipv4/udp code does.

Signed-off-by: Xufeng Zhang <xufeng.zhang@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 net/ipv6/udp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 41f8c9c..1e7a43f 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -453,7 +453,7 @@ csum_copy_err:
 	}
 	unlock_sock_fast(sk, slow);
 
-	if (flags & MSG_DONTWAIT)
+	if (noblock)
 		return -EAGAIN;
 	goto try_again;
 }
-- 
1.7.4.4

Re: [PATCH net-next 2/3] ipv6/udp: Use the correct variable to determine non-blocking condition

[Eric Dumazet]

Le mardi 21 juin 2011 à 16:43 -0400, Paul Gortmaker a écrit :
> From: Xufeng Zhang <xufeng.zhang@windriver.com>
> 
> udpv6_recvmsg() function is not using the correct variable to determine
> whether or not the socket is in non-blocking operation, this will lead
> to unexpected behavior when a UDP checksum error occurs.
> 
> Consider a non-blocking udp receive scenario: when udpv6_recvmsg() is
> called by sock_common_recvmsg(), MSG_DONTWAIT bit of flags variable in
> udpv6_recvmsg() is cleared by "flags & ~MSG_DONTWAIT" in this call:
> 
>     err = sk->sk_prot->recvmsg(iocb, sk, msg, size, flags & MSG_DONTWAIT,
>                    flags & ~MSG_DONTWAIT, &addr_len);
> 
> i.e. with udpv6_recvmsg() getting these values:
> 
> 	int noblock = flags & MSG_DONTWAIT
> 	int flags = flags & ~MSG_DONTWAIT
> 
> So, when udp checksum error occurs, the execution will go to
> csum_copy_err, and then the problem happens:
> 
>     csum_copy_err:
>             ...............
>             if (flags & MSG_DONTWAIT)
>                     return -EAGAIN;
>             goto try_again;
>             ...............
> 
> But it will always go to try_again as MSG_DONTWAIT has been cleared
> from flags at call time -- only noblock contains the original value
> of MSG_DONTWAIT, so the test should be:
> 
>             if (noblock)
>                     return -EAGAIN;
> 
> This is also consistent with what the ipv4/udp code does.
> 
> Signed-off-by: Xufeng Zhang <xufeng.zhang@windriver.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

[PATCH net-next 3/3] udp/recvmsg: Clear MSG_TRUNC flag when starting over for a new packet

[Paul Gortmaker]

From: Xufeng Zhang <xufeng.zhang@windriver.com>

Consider this scenario: When the size of the first received udp packet
is bigger than the receive buffer, MSG_TRUNC bit is set in msg->msg_flags.
However, if checksum error happens and this is a blocking socket, it will
goto try_again loop to receive the next packet.  But if the size of the
next udp packet is smaller than receive buffer, MSG_TRUNC flag should not
be set, but because MSG_TRUNC bit is not cleared in msg->msg_flags before
receive the next packet, MSG_TRUNC is still set, which is wrong.

Fix this problem by clearing MSG_TRUNC flag when starting over for a
new packet.

Signed-off-by: Xufeng Zhang <xufeng.zhang@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 net/ipv4/udp.c |    3 +++
 net/ipv6/udp.c |    3 +++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 6f53a5a..15d550c 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1249,6 +1249,9 @@ csum_copy_err:
 
 	if (noblock)
 		return -EAGAIN;
+
+	/* starting over for a new packet */
+	msg->msg_flags &= ~MSG_TRUNC;
 	goto try_again;
 }
 
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index 1e7a43f..328985c 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -455,6 +455,9 @@ csum_copy_err:
 
 	if (noblock)
 		return -EAGAIN;
+
+	/* starting over for a new packet */
+	msg->msg_flags &= ~MSG_TRUNC;
 	goto try_again;
 }
 
-- 
1.7.4.4

Re: [PATCH net-next 3/3] udp/recvmsg: Clear MSG_TRUNC flag when starting over for a new packet

[Eric Dumazet]

Le mardi 21 juin 2011 à 16:43 -0400, Paul Gortmaker a écrit :
> From: Xufeng Zhang <xufeng.zhang@windriver.com>
> 
> Consider this scenario: When the size of the first received udp packet
> is bigger than the receive buffer, MSG_TRUNC bit is set in msg->msg_flags.
> However, if checksum error happens and this is a blocking socket, it will
> goto try_again loop to receive the next packet.  But if the size of the
> next udp packet is smaller than receive buffer, MSG_TRUNC flag should not
> be set, but because MSG_TRUNC bit is not cleared in msg->msg_flags before
> receive the next packet, MSG_TRUNC is still set, which is wrong.
> 
> Fix this problem by clearing MSG_TRUNC flag when starting over for a
> new packet.
> 
> Signed-off-by: Xufeng Zhang <xufeng.zhang@windriver.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> ---
>  net/ipv4/udp.c |    3 +++
>  net/ipv6/udp.c |    3 +++
>  2 files changed, 6 insertions(+), 0 deletions(-)

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: [PATCH net-next 0/3] Three possible UDP fixes.

[David Miller]

From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Tue, 21 Jun 2011 16:43:37 -0400

> These were originally found on a 2.6.34 baseline, but I looked
> at them and couldn't see any reason why they wouldn't be valid
> fixes on net-next.  But I'll feel better when someone like
> Dave and/or Eric sanity checks them too.
> 
> There was one thing that was a consideration.  In the 3rd patch,
> where we clear MSG_TRUNC bit -- is there anything in there that
> we really need to be concerned about preserving on the retry,
> or could we just unconditionally do "msg->msg_flags = 0" ?
> I wasn't sure, and so sticking with clearing the offending bit
> seemed like the most cautious approach.

All applied and queued up for -stable, thanks!