[PATCH net-next v4] net: ip: avoid OOM kills with large UDP sends over loopback

Dave observed number of machines hitting OOM on the UDP send
path. The workload seems to be sending large UDP packets over
loopback. Since loopback has MTU of 64k kernel will try to
allocate an skb with up to 64k of head space. This has a good
chance of failing under memory pressure. What's worse if
the message length is <32k the allocation may trigger an
OOM killer.

This is entirely avoidable, we can use an skb with page frags.

af_unix solves a similar problem by limiting the head
length to SKB_MAX_ALLOC. This seems like a good and simple
approach. It means that UDP messages > 16kB will now
use fragments if underlying device supports SG, if extra
allocator pressure causes regressions in real workloads
we can switch to trying the large allocation first and
falling back.

v4: pre-calculate all the additions to alloclen so
    we can be sure it won't go over order-2

Reported-by: Dave Jones <redacted>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ipv4/ip_output.c  | 32 ++++++++++++++++++--------------
 net/ipv6/ip6_output.c | 32 +++++++++++++++++---------------
 2 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index c3efc7d658f6..8d8a8da3ae7e 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c

@@ -1054,7 +1054,7 @@ static int __ip_append_data(struct sock *sk,
 			unsigned int datalen;
 			unsigned int fraglen;
 			unsigned int fraggap;
-			unsigned int alloclen;
+			unsigned int alloclen, alloc_extra;
 			unsigned int pagedlen;
 			struct sk_buff *skb_prev;
 alloc_new_skb:

@@ -1074,35 +1074,39 @@ static int __ip_append_data(struct sock *sk,
 			fraglen = datalen + fragheaderlen;
 			pagedlen = 0;
 
+			alloc_extra = hh_len + 15;
+			alloc_extra += exthdrlen;
+
+			/* The last fragment gets additional space at tail.
+			 * Note, with MSG_MORE we overallocate on fragments,
+			 * because we have no idea what fragment will be
+			 * the last.
+			 */
+			if (datalen == length + fraggap)
+				alloc_extra += rt->dst.trailer_len;
+
 			if ((flags & MSG_MORE) &&
 			    !(rt->dst.dev->features&NETIF_F_SG))
 				alloclen = mtu;
-			else if (!paged)
+			else if (!paged &&
+				 (fraglen + alloc_extra < SKB_MAX_ALLOC ||
+				  !(rt->dst.dev->features & NETIF_F_SG)))
 				alloclen = fraglen;
 			else {
 				alloclen = min_t(int, fraglen, MAX_HEADER);
 				pagedlen = fraglen - alloclen;
 			}
 
-			alloclen += exthdrlen;
-
-			/* The last fragment gets additional space at tail.
-			 * Note, with MSG_MORE we overallocate on fragments,
-			 * because we have no idea what fragment will be
-			 * the last.
-			 */
-			if (datalen == length + fraggap)
-				alloclen += rt->dst.trailer_len;
+			alloclen += alloc_extra;
 
 			if (transhdrlen) {
-				skb = sock_alloc_send_skb(sk,
-						alloclen + hh_len + 15,
+				skb = sock_alloc_send_skb(sk, alloclen,
 						(flags & MSG_DONTWAIT), &err);
 			} else {
 				skb = NULL;
 				if (refcount_read(&sk->sk_wmem_alloc) + wmem_alloc_delta <=
 				    2 * sk->sk_sndbuf)
-					skb = alloc_skb(alloclen + hh_len + 15,
+					skb = alloc_skb(alloclen,
 							sk->sk_allocation);
 				if (unlikely(!skb))
 					err = -ENOBUFS;

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index ff4f9ebcf7f6..497974b4372a 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c

@@ -1555,7 +1555,7 @@ static int __ip6_append_data(struct sock *sk,
 			unsigned int datalen;
 			unsigned int fraglen;
 			unsigned int fraggap;
-			unsigned int alloclen;
+			unsigned int alloclen, alloc_extra;
 			unsigned int pagedlen;
 alloc_new_skb:
 			/* There's no room in the current skb */

@@ -1582,17 +1582,28 @@ static int __ip6_append_data(struct sock *sk,
 			fraglen = datalen + fragheaderlen;
 			pagedlen = 0;
 
+			alloc_extra = hh_len;
+			alloc_extra += dst_exthdrlen;
+			alloc_extra += rt->dst.trailer_len;
+
+			/* We just reserve space for fragment header.
+			 * Note: this may be overallocation if the message
+			 * (without MSG_MORE) fits into the MTU.
+			 */
+			alloc_extra += sizeof(struct frag_hdr);
+
 			if ((flags & MSG_MORE) &&
 			    !(rt->dst.dev->features&NETIF_F_SG))
 				alloclen = mtu;
-			else if (!paged)
+			else if (!paged &&
+				 (fraglen + alloc_extra < SKB_MAX_ALLOC ||
+				  !(rt->dst.dev->features & NETIF_F_SG)))
 				alloclen = fraglen;
 			else {
 				alloclen = min_t(int, fraglen, MAX_HEADER);
 				pagedlen = fraglen - alloclen;
 			}
-
-			alloclen += dst_exthdrlen;
+			alloclen += alloc_extra;
 
 			if (datalen != length + fraggap) {
 				/*

@@ -1602,30 +1613,21 @@ static int __ip6_append_data(struct sock *sk,
 				datalen += rt->dst.trailer_len;
 			}
 
-			alloclen += rt->dst.trailer_len;
 			fraglen = datalen + fragheaderlen;
 
-			/*
-			 * We just reserve space for fragment header.
-			 * Note: this may be overallocation if the message
-			 * (without MSG_MORE) fits into the MTU.
-			 */
-			alloclen += sizeof(struct frag_hdr);
-
 			copy = datalen - transhdrlen - fraggap - pagedlen;
 			if (copy < 0) {
 				err = -EINVAL;
 				goto error;
 			}
 			if (transhdrlen) {
-				skb = sock_alloc_send_skb(sk,
-						alloclen + hh_len,
+				skb = sock_alloc_send_skb(sk, alloclen,
 						(flags & MSG_DONTWAIT), &err);
 			} else {
 				skb = NULL;
 				if (refcount_read(&sk->sk_wmem_alloc) + wmem_alloc_delta <=
 				    2 * sk->sk_sndbuf)
-					skb = alloc_skb(alloclen + hh_len,
+					skb = alloc_skb(alloclen,
 							sk->sk_allocation);
 				if (unlikely(!skb))
 					err = -ENOBUFS;

-- 
2.31.1

On Wed, Jun 23, 2021 at 5:44 PM Jakub Kicinski [off-list ref] wrote:

Separate line?

This perhaps deserves a comment. Something like this?

  /* avoid order-3 allocations where possible: replace with frags if
allowed (sg) */

                                alloclen = fraglen;
                        else {
                                alloclen = min_t(int, fraglen, MAX_HEADER);
                                pagedlen = fraglen - alloclen;
                        }

-                       alloclen += exthdrlen;
-
-                       /* The last fragment gets additional space at tail.
-                        * Note, with MSG_MORE we overallocate on fragments,
-                        * because we have no idea what fragment will be
-                        * the last.
-                        */
-                       if (datalen == length + fraggap)
-                               alloclen += rt->dst.trailer_len;
+                       alloclen += alloc_extra;

                        if (transhdrlen) {
-                               skb = sock_alloc_send_skb(sk,
-                                               alloclen + hh_len + 15,
+                               skb = sock_alloc_send_skb(sk, alloclen,
                                                (flags & MSG_DONTWAIT), &err);
                        } else {
                                skb = NULL;
                                if (refcount_read(&sk->sk_wmem_alloc) + wmem_alloc_delta <=
                                    2 * sk->sk_sndbuf)
-                                       skb = alloc_skb(alloclen + hh_len + 15,
+                                       skb = alloc_skb(alloclen,
                                                        sk->sk_allocation);
                                if (unlikely(!skb))
                                        err = -ENOBUFS;

Is there any risk of regressions? If so, would it be preferable to try
regular alloc and only on failure, just below here, do the size and SG
test and if permitted jump back to the last of the three alloc_len
options?

quoted

                        if (transhdrlen) {
-                               skb = sock_alloc_send_skb(sk,
-                                               alloclen + hh_len + 15,
+                               skb = sock_alloc_send_skb(sk, alloclen,
                                                (flags & MSG_DONTWAIT), &err);
                        } else {
                                skb = NULL;
                                if (refcount_read(&sk->sk_wmem_alloc) + wmem_alloc_delta <=
                                    2 * sk->sk_sndbuf)
-                                       skb = alloc_skb(alloclen + hh_len + 15,
+                                       skb = alloc_skb(alloclen,
                                                        sk->sk_allocation);
                                if (unlikely(!skb))
                                        err = -ENOBUFS;

Is there any risk of regressions? If so, would it be preferable to try
regular alloc and only on failure, just below here, do the size and SG
test and if permitted jump back to the last of the three alloc_len
options?

sk_page_frag_refill when using frags will try also try large allocations first
(SKB_FRAG_PAGE_ORDER == order-3) , but can degrade more gracefully
under memory pressure than this header alloc. Which can only succeed
or fail for the total size. So without memory pressure this only takes two
extra allocations for a 64KB skb.

On Wed, 23 Jun 2021 22:21:11 -0400 Willem de Bruijn wrote:

On Wed, Jun 23, 2021 at 5:44 PM Jakub Kicinski [off-list ref] wrote:

quoted

Dave observed number of machines hitting OOM on the UDP send
path. The workload seems to be sending large UDP packets over
loopback. Since loopback has MTU of 64k kernel will try to
allocate an skb with up to 64k of head space. This has a good
chance of failing under memory pressure. What's worse if
the message length is <32k the allocation may trigger an
OOM killer.

This is entirely avoidable, we can use an skb with page frags.

af_unix solves a similar problem by limiting the head
length to SKB_MAX_ALLOC. This seems like a good and simple
approach. It means that UDP messages > 16kB will now
use fragments if underlying device supports SG, if extra
allocator pressure causes regressions in real workloads
we can switch to trying the large allocation first and
falling back.

v4: pre-calculate all the additions to alloclen so
    we can be sure it won't go over order-2

Reported-by: Dave Jones <redacted>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ipv4/ip_output.c  | 32 ++++++++++++++++++--------------
 net/ipv6/ip6_output.c | 32 +++++++++++++++++---------------
 2 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index c3efc7d658f6..8d8a8da3ae7e 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c

@@ -1054,7 +1054,7 @@ static int __ip_append_data(struct sock *sk,
                        unsigned int datalen;
                        unsigned int fraglen;
                        unsigned int fraggap;
-                       unsigned int alloclen;
+                       unsigned int alloclen, alloc_extra;  

Separate line?

But why? What makes it preferable to have logically connected variables
declared on separate lines? The function is already 300 LoC. I've been
meaning to ask someone about this preference for a while :)

quoted

                        unsigned int pagedlen;
                        struct sk_buff *skb_prev;
 alloc_new_skb:

@@ -1074,35 +1074,39 @@ static int __ip_append_data(struct sock *sk,
                        fraglen = datalen + fragheaderlen;
                        pagedlen = 0;

+                       alloc_extra = hh_len + 15;
+                       alloc_extra += exthdrlen;
+
+                       /* The last fragment gets additional space at tail.
+                        * Note, with MSG_MORE we overallocate on fragments,
+                        * because we have no idea what fragment will be
+                        * the last.
+                        */
+                       if (datalen == length + fraggap)
+                               alloc_extra += rt->dst.trailer_len;
+
                        if ((flags & MSG_MORE) &&
                            !(rt->dst.dev->features&NETIF_F_SG))
                                alloclen = mtu;
-                       else if (!paged)
+                       else if (!paged &&
+                                (fraglen + alloc_extra < SKB_MAX_ALLOC ||
+                                 !(rt->dst.dev->features & NETIF_F_SG)))  

This perhaps deserves a comment. Something like this?

  /* avoid order-3 allocations where possible: replace with frags if
allowed (sg) */

Here I thought comparing skb alloc size to SKB_MAX_ALLOC is explanatory
enough ;)

In the middle of the test, like this, right?

                 else if (!paged &&
                          /* avoid order-3 allocations if device
                           * can handle skb frags (sg)
                           */
                          (fraglen + alloc_extra < SKB_MAX_ALLOC ||
                          !(rt->dst.dev->features & NETIF_F_SG)))  

I should make it less-equal while at it.

quoted

                                alloclen = fraglen;
                        else {
                                alloclen = min_t(int, fraglen, MAX_HEADER);
                                pagedlen = fraglen - alloclen;
                        }

-                       alloclen += exthdrlen;
-
-                       /* The last fragment gets additional space at tail.
-                        * Note, with MSG_MORE we overallocate on fragments,
-                        * because we have no idea what fragment will be
-                        * the last.
-                        */
-                       if (datalen == length + fraggap)
-                               alloclen += rt->dst.trailer_len;
+                       alloclen += alloc_extra;

                        if (transhdrlen) {
-                               skb = sock_alloc_send_skb(sk,
-                                               alloclen + hh_len + 15,
+                               skb = sock_alloc_send_skb(sk, alloclen,
                                                (flags & MSG_DONTWAIT), &err);
                        } else {
                                skb = NULL;
                                if (refcount_read(&sk->sk_wmem_alloc) + wmem_alloc_delta <=
                                    2 * sk->sk_sndbuf)
-                                       skb = alloc_skb(alloclen + hh_len + 15,
+                                       skb = alloc_skb(alloclen,
                                                        sk->sk_allocation);
                                if (unlikely(!skb))
                                        err = -ENOBUFS;  

Is there any risk of regressions? If so, would it be preferable to try
regular alloc and only on failure, just below here, do the size and SG
test and if permitted jump back to the last of the three alloc_len
options?

There is, that's what I tried in v1, Eric pointed out that we can't
modify sk->sk_allocation here because UDP fast path doesn't take the
lock, and pointed out that UNIX code has to handle similar problem. 
So I decided to just copy what AF_UNIX does. In practical terms
MTU > 16k is highly unlikely on physical devices (AFAIK) and with
messages that large hopefully the trip thru the memory allocator won't
be all that noticeable? If we were capping at one page that'd be a
problem, but my gut feeling was that order-2 cap is unlikely to hurt.

But I can go back, I'd have to refactor sock_alloc_send_pskb() to pass
gfp_t explicitly. Probably by creating another layer of helpers
(__sock_alloc_send_pskb()?). sock_alloc_send_pskb() already takes 6
params so I was also thinking of converting it to ERR_PTR() return
(instead of passing the error pointer) (6 is max for register passing).

Should I go back to retry?

On Thu, Jun 24, 2021 at 12:28 PM Jakub Kicinski [off-list ref] wrote:

On Wed, 23 Jun 2021 22:21:11 -0400 Willem de Bruijn wrote:

quoted

On Wed, Jun 23, 2021 at 5:44 PM Jakub Kicinski [off-list ref] wrote:

quoted

Dave observed number of machines hitting OOM on the UDP send
path. The workload seems to be sending large UDP packets over
loopback. Since loopback has MTU of 64k kernel will try to
allocate an skb with up to 64k of head space. This has a good
chance of failing under memory pressure. What's worse if
the message length is <32k the allocation may trigger an
OOM killer.

This is entirely avoidable, we can use an skb with page frags.

af_unix solves a similar problem by limiting the head
length to SKB_MAX_ALLOC. This seems like a good and simple
approach. It means that UDP messages > 16kB will now
use fragments if underlying device supports SG, if extra
allocator pressure causes regressions in real workloads
we can switch to trying the large allocation first and
falling back.

v4: pre-calculate all the additions to alloclen so
    we can be sure it won't go over order-2

Reported-by: Dave Jones <redacted>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ipv4/ip_output.c  | 32 ++++++++++++++++++--------------
 net/ipv6/ip6_output.c | 32 +++++++++++++++++---------------
 2 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index c3efc7d658f6..8d8a8da3ae7e 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c

@@ -1054,7 +1054,7 @@ static int __ip_append_data(struct sock *sk,
                        unsigned int datalen;
                        unsigned int fraglen;
                        unsigned int fraggap;
-                       unsigned int alloclen;
+                       unsigned int alloclen, alloc_extra;

Separate line?

But why? What makes it preferable to have logically connected variables
declared on separate lines? The function is already 300 LoC. I've been
meaning to ask someone about this preference for a while :)

Reverse christmas tree is the norm in netdev. Pointing out for
consistency only. I have no particular opinion on the rule.

Agreed that in this function, multiple entries per line would be preferable.

quoted

quoted

                        unsigned int pagedlen;
                        struct sk_buff *skb_prev;
 alloc_new_skb:

@@ -1074,35 +1074,39 @@ static int __ip_append_data(struct sock *sk,
                        fraglen = datalen + fragheaderlen;
                        pagedlen = 0;

+                       alloc_extra = hh_len + 15;
+                       alloc_extra += exthdrlen;
+
+                       /* The last fragment gets additional space at tail.
+                        * Note, with MSG_MORE we overallocate on fragments,
+                        * because we have no idea what fragment will be
+                        * the last.
+                        */
+                       if (datalen == length + fraggap)
+                               alloc_extra += rt->dst.trailer_len;
+
                        if ((flags & MSG_MORE) &&
                            !(rt->dst.dev->features&NETIF_F_SG))
                                alloclen = mtu;
-                       else if (!paged)
+                       else if (!paged &&
+                                (fraglen + alloc_extra < SKB_MAX_ALLOC ||
+                                 !(rt->dst.dev->features & NETIF_F_SG)))

This perhaps deserves a comment. Something like this?

  /* avoid order-3 allocations where possible: replace with frags if
allowed (sg) */

Here I thought comparing skb alloc size to SKB_MAX_ALLOC is explanatory
enough ;)

Yeah, I guess you're right. The comment only rewords *what* the code
does, so not super informative. Never mind that suggestion.

In the middle of the test, like this, right?

                 else if (!paged &&
                          /* avoid order-3 allocations if device
                           * can handle skb frags (sg)
                           */
                          (fraglen + alloc_extra < SKB_MAX_ALLOC ||
                          !(rt->dst.dev->features & NETIF_F_SG)))

I should make it less-equal while at it.

quoted

quoted

                                alloclen = fraglen;
                        else {
                                alloclen = min_t(int, fraglen, MAX_HEADER);
                                pagedlen = fraglen - alloclen;
                        }

-                       alloclen += exthdrlen;
-
-                       /* The last fragment gets additional space at tail.
-                        * Note, with MSG_MORE we overallocate on fragments,
-                        * because we have no idea what fragment will be
-                        * the last.
-                        */
-                       if (datalen == length + fraggap)
-                               alloclen += rt->dst.trailer_len;
+                       alloclen += alloc_extra;

                        if (transhdrlen) {
-                               skb = sock_alloc_send_skb(sk,
-                                               alloclen + hh_len + 15,
+                               skb = sock_alloc_send_skb(sk, alloclen,
                                                (flags & MSG_DONTWAIT), &err);
                        } else {
                                skb = NULL;
                                if (refcount_read(&sk->sk_wmem_alloc) + wmem_alloc_delta <=
                                    2 * sk->sk_sndbuf)
-                                       skb = alloc_skb(alloclen + hh_len + 15,
+                                       skb = alloc_skb(alloclen,
                                                        sk->sk_allocation);
                                if (unlikely(!skb))
                                        err = -ENOBUFS;

Is there any risk of regressions? If so, would it be preferable to try
regular alloc and only on failure, just below here, do the size and SG
test and if permitted jump back to the last of the three alloc_len
options?

There is, that's what I tried in v1, Eric pointed out that we can't
modify sk->sk_allocation here because UDP fast path doesn't take the
lock, and pointed out that UNIX code has to handle similar problem.
So I decided to just copy what AF_UNIX does. In practical terms
MTU > 16k is highly unlikely on physical devices (AFAIK) and with
messages that large hopefully the trip thru the memory allocator won't
be all that noticeable? If we were capping at one page that'd be a
problem, but my gut feeling was that order-2 cap is unlikely to hurt.

But I can go back, I'd have to refactor sock_alloc_send_pskb() to pass
gfp_t explicitly. Probably by creating another layer of helpers
(__sock_alloc_send_pskb()?). sock_alloc_send_pskb() already takes 6
params so I was also thinking of converting it to ERR_PTR() return
(instead of passing the error pointer) (6 is max for register passing).

Should I go back to retry?

For __GFP_NOWARN? Sorry, I missed that.

Okay, then I understand why this approach is preferable. And LGTM. Thanks!

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Wed, 23 Jun 2021 14:44:38 -0700 you wrote:

Dave observed number of machines hitting OOM on the UDP send
path. The workload seems to be sending large UDP packets over
loopback. Since loopback has MTU of 64k kernel will try to
allocate an skb with up to 64k of head space. This has a good
chance of failing under memory pressure. What's worse if
the message length is <32k the allocation may trigger an
OOM killer.

[...]

Here is the summary with links:
  - [net-next,v4] net: ip: avoid OOM kills with large UDP sends over loopback
    https://git.kernel.org/netdev/net-next/c/6d123b81ac61

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html