* [PATCH net] tcp: fix potential huge kmalloc() calls in TCP_REPAIR
@ 2015-11-19 5:03 Eric Dumazet
2015-11-19 10:51 ` Pavel Emelyanov
2015-11-20 15:58 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Eric Dumazet @ 2015-11-19 5:03 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Pavel Emelyanov
From: Eric Dumazet <edumazet@google.com>
tcp_send_rcvq() is used for re-injecting data into tcp receive queue.
Problems :
- No check against size is performed, allowed user to fool kernel in
attempting very large memory allocations, eventually triggering
OOM when memory is fragmented.
- In case of fault during the copy we do not return correct errno.
Lets use alloc_skb_with_frags() to cook optimal skbs.
Fixes: 292e8d8c8538 ("tcp: Move rcvq sending to tcp_input.c")
Fixes: c0e88ff0f256 ("tcp: Repair socket queues")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
---
net/ipv4/tcp_input.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index fdd88c3803a6..a4a0b6b3bcf2 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -4481,19 +4481,34 @@ static int __must_check tcp_queue_rcv(struct sock *sk, struct sk_buff *skb, int
int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
{
struct sk_buff *skb;
+ int err = -ENOMEM;
+ int data_len = 0;
bool fragstolen;
if (size == 0)
return 0;
- skb = alloc_skb(size, sk->sk_allocation);
+ if (size > PAGE_SIZE) {
+ int npages = min_t(size_t, size >> PAGE_SHIFT, MAX_SKB_FRAGS);
+
+ data_len = npages << PAGE_SHIFT;
+ size = data_len + (size & ~PAGE_MASK);
+ }
+ skb = alloc_skb_with_frags(size - data_len, data_len,
+ PAGE_ALLOC_COSTLY_ORDER,
+ &err, sk->sk_allocation);
if (!skb)
goto err;
+ skb_put(skb, size - data_len);
+ skb->data_len = data_len;
+ skb->len = size;
+
if (tcp_try_rmem_schedule(sk, skb, skb->truesize))
goto err_free;
- if (memcpy_from_msg(skb_put(skb, size), msg, size))
+ err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, size);
+ if (err)
goto err_free;
TCP_SKB_CB(skb)->seq = tcp_sk(sk)->rcv_nxt;
@@ -4509,7 +4524,8 @@ int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
err_free:
kfree_skb(skb);
err:
- return -ENOMEM;
+ return err;
+
}
static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] tcp: fix potential huge kmalloc() calls in TCP_REPAIR
2015-11-19 5:03 [PATCH net] tcp: fix potential huge kmalloc() calls in TCP_REPAIR Eric Dumazet
@ 2015-11-19 10:51 ` Pavel Emelyanov
2015-11-19 12:25 ` Eric Dumazet
2015-11-20 15:58 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: Pavel Emelyanov @ 2015-11-19 10:51 UTC (permalink / raw)
To: Eric Dumazet, David Miller; +Cc: netdev
On 11/19/2015 08:03 AM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> tcp_send_rcvq() is used for re-injecting data into tcp receive queue.
>
> Problems :
>
> - No check against size is performed, allowed user to fool kernel in
> attempting very large memory allocations, eventually triggering
> OOM when memory is fragmented.
Doesn't the tcp_try_rmem_schedule() protect us from doing this "eventually"?
I mean first we would be allowed to do it, but then the sock will be charged
with the previous allocations and will not add more memory to socket.
Nonetheless,
Acked-by: Pavel Emelyanov <xemul@parallels.com>
> - In case of fault during the copy we do not return correct errno.
>
> Lets use alloc_skb_with_frags() to cook optimal skbs.
>
> Fixes: 292e8d8c8538 ("tcp: Move rcvq sending to tcp_input.c")
> Fixes: c0e88ff0f256 ("tcp: Repair socket queues")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Pavel Emelyanov <xemul@parallels.com>
> ---
> net/ipv4/tcp_input.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index fdd88c3803a6..a4a0b6b3bcf2 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -4481,19 +4481,34 @@ static int __must_check tcp_queue_rcv(struct sock *sk, struct sk_buff *skb, int
> int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
> {
> struct sk_buff *skb;
> + int err = -ENOMEM;
> + int data_len = 0;
> bool fragstolen;
>
> if (size == 0)
> return 0;
>
> - skb = alloc_skb(size, sk->sk_allocation);
> + if (size > PAGE_SIZE) {
> + int npages = min_t(size_t, size >> PAGE_SHIFT, MAX_SKB_FRAGS);
> +
> + data_len = npages << PAGE_SHIFT;
> + size = data_len + (size & ~PAGE_MASK);
> + }
> + skb = alloc_skb_with_frags(size - data_len, data_len,
> + PAGE_ALLOC_COSTLY_ORDER,
> + &err, sk->sk_allocation);
> if (!skb)
> goto err;
>
> + skb_put(skb, size - data_len);
> + skb->data_len = data_len;
> + skb->len = size;
> +
> if (tcp_try_rmem_schedule(sk, skb, skb->truesize))
> goto err_free;
>
> - if (memcpy_from_msg(skb_put(skb, size), msg, size))
> + err = skb_copy_datagram_from_iter(skb, 0, &msg->msg_iter, size);
> + if (err)
> goto err_free;
>
> TCP_SKB_CB(skb)->seq = tcp_sk(sk)->rcv_nxt;
> @@ -4509,7 +4524,8 @@ int tcp_send_rcvq(struct sock *sk, struct msghdr *msg, size_t size)
> err_free:
> kfree_skb(skb);
> err:
> - return -ENOMEM;
> + return err;
> +
> }
>
> static void tcp_data_queue(struct sock *sk, struct sk_buff *skb)
>
>
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] tcp: fix potential huge kmalloc() calls in TCP_REPAIR
2015-11-19 10:51 ` Pavel Emelyanov
@ 2015-11-19 12:25 ` Eric Dumazet
2015-11-19 12:31 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2015-11-19 12:25 UTC (permalink / raw)
To: Pavel Emelyanov; +Cc: David Miller, netdev
On Thu, 2015-11-19 at 13:51 +0300, Pavel Emelyanov wrote:
> On 11/19/2015 08:03 AM, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > tcp_send_rcvq() is used for re-injecting data into tcp receive queue.
> >
> > Problems :
> >
> > - No check against size is performed, allowed user to fool kernel in
> > attempting very large memory allocations, eventually triggering
> > OOM when memory is fragmented.
>
> Doesn't the tcp_try_rmem_schedule() protect us from doing this "eventually"?
> I mean first we would be allowed to do it, but then the sock will be charged
> with the previous allocations and will not add more memory to socket.
But this tcp_try_rmem_schedule() is done _after_ allocation was
attempted. This is too late :(
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] tcp: fix potential huge kmalloc() calls in TCP_REPAIR
2015-11-19 12:25 ` Eric Dumazet
@ 2015-11-19 12:31 ` Eric Dumazet
0 siblings, 0 replies; 5+ messages in thread
From: Eric Dumazet @ 2015-11-19 12:31 UTC (permalink / raw)
To: Pavel Emelyanov; +Cc: David Miller, netdev
On Thu, 2015-11-19 at 04:25 -0800, Eric Dumazet wrote:
> On Thu, 2015-11-19 at 13:51 +0300, Pavel Emelyanov wrote:
> > On 11/19/2015 08:03 AM, Eric Dumazet wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > >
> > > tcp_send_rcvq() is used for re-injecting data into tcp receive queue.
> > >
> > > Problems :
> > >
> > > - No check against size is performed, allowed user to fool kernel in
> > > attempting very large memory allocations, eventually triggering
> > > OOM when memory is fragmented.
> >
> > Doesn't the tcp_try_rmem_schedule() protect us from doing this "eventually"?
> > I mean first we would be allowed to do it, but then the sock will be charged
> > with the previous allocations and will not add more memory to socket.
>
> But this tcp_try_rmem_schedule() is done _after_ allocation was
> attempted. This is too late :(
Simple reproducer allocating 2 Mbytes of contiguous kernel memory :
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/tcp.h>
#include <netinet/in.h>
#include <arpa/inet.h>
#include <errno.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>
static void fail(const char *str)
{
perror(str);
exit(1);
}
void usage(int code)
{
exit(1);
}
int main(int argc, char *argv[])
{
int res, c, repair = 1;
int queue = TCP_RECV_QUEUE;
int fd = socket(AF_INET, SOCK_STREAM, 0);
size_t sz = 2*1024*1024 - 1024;
char *buffer;
struct sockaddr_in src, dst;
while ((c = getopt(argc, argv, "m:")) != -1) {
switch (c) {
case 'm' :
sz = atoi(optarg);
break;
default : usage(1);
}
}
if (setsockopt(fd, SOL_TCP, TCP_REPAIR, &repair, sizeof(repair)) == -1)
fail("TCP_REPAIR");
memset(&src, 0, sizeof(src));
src.sin_family = AF_INET;
src.sin_port = htons(8888);
src.sin_addr.s_addr = htonl(0x7f000001);
if (bind(fd, (struct sockaddr *)&src, sizeof(src)) == -1)
fail("bind");
memset(&dst, 0, sizeof(dst));
dst.sin_family = AF_INET;
dst.sin_port = htons(9999);
dst.sin_addr.s_addr = htonl(0x7f000001);
if (connect(fd, (struct sockaddr *)&dst, sizeof(dst)) == -1)
fail("connect");
if (setsockopt(fd, SOL_TCP, TCP_REPAIR_QUEUE, &queue, sizeof(queue)) == -1)
fail("TCP_REPAIR_QUEUE");
buffer = malloc(sz);
if (!buffer)
buffer = malloc(1000);
res = send(fd, buffer, sz, 0);
printf("send() res=%d errno=%d\n", res, errno);
if (res != -1)
sleep(1000);
return 0;
}
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] tcp: fix potential huge kmalloc() calls in TCP_REPAIR
2015-11-19 5:03 [PATCH net] tcp: fix potential huge kmalloc() calls in TCP_REPAIR Eric Dumazet
2015-11-19 10:51 ` Pavel Emelyanov
@ 2015-11-20 15:58 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2015-11-20 15:58 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, xemul
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 18 Nov 2015 21:03:33 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> tcp_send_rcvq() is used for re-injecting data into tcp receive queue.
>
> Problems :
>
> - No check against size is performed, allowed user to fool kernel in
> attempting very large memory allocations, eventually triggering
> OOM when memory is fragmented.
>
> - In case of fault during the copy we do not return correct errno.
>
> Lets use alloc_skb_with_frags() to cook optimal skbs.
>
> Fixes: 292e8d8c8538 ("tcp: Move rcvq sending to tcp_input.c")
> Fixes: c0e88ff0f256 ("tcp: Repair socket queues")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Good catch, applied and queued up for -stable.
Thanks!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-11-20 15:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-19 5:03 [PATCH net] tcp: fix potential huge kmalloc() calls in TCP_REPAIR Eric Dumazet
2015-11-19 10:51 ` Pavel Emelyanov
2015-11-19 12:25 ` Eric Dumazet
2015-11-19 12:31 ` Eric Dumazet
2015-11-20 15:58 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.