Kernel memory leak in bnx2x driver with vxlan tunnel

I'm getting what seems to be a kernel memory leak while doing a TCP throughput test between two VMs on identical systems, in order to test a broadcom NIC's performance with a kernel 4.4.0-rc8 and OpenVSwitch version 2.4.90. The host system of the receiving (server) VM leaks memory during the throughput test. The memory leaks fast enough to make the system completely unusable within five minutes. Once I stop the throughput test, the memory stops
leaking. A couple of times, the kernel on the host system has actually killed
the qemu process for me, but this doesn't happen reliably. The leaked memory doesn't become available again even after the VM is killed.

Quoted 11 lines · John

I'm getting what seems to be a kernel memory leak while doing a TCP throughput test between two VMs on identical systems, in order to test a broadcom NIC's performance with a kernel 4.4.0-rc8 and OpenVSwitch version 2.4.90. The host system of the receiving (server) VM leaks memory during the throughput test. The memory leaks fast enough to make the system completely unusable within five minutes. Once I stop the throughput test, the memory stops
leaking. A couple of times, the kernel on the host system has actually killed
the qemu process for me, but this doesn't happen reliably. The leaked memory doesn't become available again even after the VM is killed.

It looks like the problem is in napi_skb_finish(). If we when do GRO_MERGED_FREE we have NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD then we will just free the skb memory itself but not any of the associated elements. Historically, this would have been OK but these days we will have allocated a dst entry already for tunnel metadata, which will get leaked.

If we don't have NAPI_GRO_FREE_STOLEN_HEAD then we'll do a __kfree_skb(), which will release the dst entry. That would explain why some drivers have the problem but not others since the memory is laid out differently.

Quoted 20 lines · Jesse Gross

Quoted 11 lines · John

I'm getting what seems to be a kernel memory leak while doing a TCP throughput test between two VMs on identical systems, in order to test a broadcom NIC's performance with a kernel 4.4.0-rc8 and OpenVSwitch version 2.4.90. The host system of the receiving (server) VM leaks memory during the throughput test. The memory leaks fast enough to make the system completely unusable within five minutes. Once I stop the throughput test, the memory stops
leaking. A couple of times, the kernel on the host system has actually killed
the qemu process for me, but this doesn't happen reliably. The leaked memory doesn't become available again even after the VM is killed.

It looks like the problem is in napi_skb_finish(). If we when do GRO_MERGED_FREE we have NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD then we will just free the skb memory itself but not any of the associated elements. Historically, this would have been OK but these days we will have allocated a dst entry already for tunnel metadata, which will get leaked.

Quoted 4 lines

If we don't have NAPI_GRO_FREE_STOLEN_HEAD then we'll do a __kfree_skb(), which will release the dst entry. That would explain why some drivers have the problem but not others since the memory is laid out differently.

Wow.... What is the purpose of using skb_dst_set() on skb before calling gro_cells_receive() exactly ?

Commit 2e15ea390e6f4466655066d97e22ec66870a042c changelog is not helpful :

Following patch create new tunnel flag which enable tunnel metadata collection on given device.

This is rather strange since later the dst is thrown away with the skb_valid_dst() test.

Note also that IP early demux is broken as well, since it does not use skb_valid_dst() but a simple :

if (sysctl_ip_early_demux && !skb_dst(skb) && !skb->sk) {
...
}

Quoted 33 lines · Eric Dumazet

Quoted 20 lines · Jesse Gross

Quoted 11 lines · John

I'm getting what seems to be a kernel memory leak while doing a TCP throughput test between two VMs on identical systems, in order to test a broadcom NIC's performance with a kernel 4.4.0-rc8 and OpenVSwitch version 2.4.90. The host system of the receiving (server) VM leaks memory during the throughput test. The memory leaks fast enough to make the system completely unusable within five minutes. Once I stop the throughput test, the memory stops
leaking. A couple of times, the kernel on the host system has actually killed
the qemu process for me, but this doesn't happen reliably. The leaked memory doesn't become available again even after the VM is killed.

It looks like the problem is in napi_skb_finish(). If we when do GRO_MERGED_FREE we have NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD then we will just free the skb memory itself but not any of the associated elements. Historically, this would have been OK but these days we will have allocated a dst entry already for tunnel metadata, which will get leaked.

Quoted 4 lines

If we don't have NAPI_GRO_FREE_STOLEN_HEAD then we'll do a __kfree_skb(), which will release the dst entry. That would explain why some drivers have the problem but not others since the memory is laid out differently.

Wow.... What is the purpose of using skb_dst_set() on skb before calling gro_cells_receive() exactly ?

Commit 2e15ea390e6f4466655066d97e22ec66870a042c changelog is not helpful :

Quoted 2 lines

Following patch create new tunnel flag which enable tunnel metadata collection on given device.

Note that this isn't really the problem commit. The general issue is lightweight tunnels - the above commit is just adding support for GRE in a way that I think follows the existing model.

Quoted 34 lines · Jesse Gross

Quoted 33 lines · Eric Dumazet

Quoted 20 lines · Jesse Gross

Quoted 11 lines · John

I'm getting what seems to be a kernel memory leak while doing a TCP throughput test between two VMs on identical systems, in order to test a broadcom NIC's performance with a kernel 4.4.0-rc8 and OpenVSwitch version 2.4.90. The host system of the receiving (server) VM leaks memory during the throughput test. The memory leaks fast enough to make the system completely unusable within five minutes. Once I stop the throughput test, the memory stops
leaking. A couple of times, the kernel on the host system has actually killed
the qemu process for me, but this doesn't happen reliably. The leaked memory doesn't become available again even after the VM is killed.

It looks like the problem is in napi_skb_finish(). If we when do GRO_MERGED_FREE we have NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD then we will just free the skb memory itself but not any of the associated elements. Historically, this would have been OK but these days we will have allocated a dst entry already for tunnel metadata, which will get leaked.

Quoted 4 lines

If we don't have NAPI_GRO_FREE_STOLEN_HEAD then we'll do a __kfree_skb(), which will release the dst entry. That would explain why some drivers have the problem but not others since the memory is laid out differently.

Wow.... What is the purpose of using skb_dst_set() on skb before calling gro_cells_receive() exactly ?

Commit 2e15ea390e6f4466655066d97e22ec66870a042c changelog is not helpful :

Quoted 6 lines

Quoted 2 lines

Following patch create new tunnel flag which enable tunnel metadata collection on given device.

Note that this isn't really the problem commit. The general issue is lightweight tunnels - the above commit is just adding support for GRE in a way that I think follows the existing model.

I believe this commit added the first skb_dst_set() before gro_cells_receive(), in ip_tunnel_rcv().

This is already buggy.

Then Tom in 58ce31cca1ffe057f4744c3f671e3e84606d3d4a added the gro_cells_receive() in vxlan_rcv(), which added another bug, because of existing skb_dst_set() call.

Oh, I wasn't disagreeing that there is a problem. I was just trying to point out the conceptual root of the problem rather than the individual instances in different tunnels implementations.

In any case, I think we can solve this by calling skb_release_head_state() in napi_skb_finish() and switching early demux to use skb_valid_dst().

Quoted 4 lines · Jesse Gross

Oh, I wasn't disagreeing that there is a problem. I was just trying to point out the conceptual root of the problem rather than the individual instances in different tunnels implementations.

Quoted 3 lines

In any case, I think we can solve this by calling skb_release_head_state() in napi_skb_finish() and switching early demux to use skb_valid_dst().

Yes, but it will also have to call skb_dst_drop() or risk a leak of prior one.

So what is the purpose of having a dst if we need to drop it ?

Adding code in GRO would be fine if someone explains me the purpose of doing apparently useless work.

(refcounting on dst is not exactly free)

Quoted 6 lines · Eric Dumazet

So what is the purpose of having a dst if we need to drop it ?

Adding code in GRO would be fine if someone explains me the purpose of doing apparently useless work.

(refcounting on dst is not exactly free)

In the GRO case, the dst is only dropped on the packets which have been merged and therefore need to be freed (the GRO_MERGED_FREE case). It's not being thrown away for the overall frame, just metadata that has been duplicated on each individual frame, similar to the metadata in struct sk_buff itself. And while it is not used by the IP stack there are other consumers (eBPF/OVS/etc.). This entire process is controlled by the COLLECT_METADATA flag on tunnels, so there is no cost in situations where it is not actually used.

Quoted 8 lines · Jesse Gross

Quoted 6 lines · Eric Dumazet

So what is the purpose of having a dst if we need to drop it ?

Adding code in GRO would be fine if someone explains me the purpose of doing apparently useless work.

(refcounting on dst is not exactly free)

Quoted 8 lines

In the GRO case, the dst is only dropped on the packets which have been merged and therefore need to be freed (the GRO_MERGED_FREE case). It's not being thrown away for the overall frame, just metadata that has been duplicated on each individual frame, similar to the metadata in struct sk_buff itself. And while it is not used by the IP stack there are other consumers (eBPF/OVS/etc.). This entire process is controlled by the COLLECT_METADATA flag on tunnels, so there is no cost in situations where it is not actually used.

Right. There were thoughts around leveraging a per CPU scratch buffer without a refcount and turn it into a full reference when the packet gets enqueued somewhere but the need hasn't really come up yet.

Jesse, is this what you have in mind:

Quoted 7 lines · Jesse Gross

Quoted 6 lines · Eric Dumazet

So what is the purpose of having a dst if we need to drop it ?

Adding code in GRO would be fine if someone explains me the purpose of doing apparently useless work.

(refcounting on dst is not exactly free)

Quoted 8 lines

In the GRO case, the dst is only dropped on the packets which have been merged and therefore need to be freed (the GRO_MERGED_FREE case). It's not being thrown away for the overall frame, just metadata that has been duplicated on each individual frame, similar to the metadata in struct sk_buff itself. And while it is not used by the IP stack there are other consumers (eBPF/OVS/etc.). This entire process is controlled by the COLLECT_METADATA flag on tunnels, so there is no cost in situations where it is not actually used.

Right. There were thoughts around leveraging a per CPU scratch buffer without a refcount and turn it into a full reference when the packet gets enqueued somewhere but the need hasn't really come up yet.

Jesse, is this what you have in mind:

net/core/dev.c +0 -0

index cc9e365..3a5e96d 100644
@@ -4548,9 +4548,10 @@ static gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb)
                 break;

Quoted 30 lines · Thomas Graf

Quoted 7 lines · Jesse Gross

Quoted 6 lines · Eric Dumazet

So what is the purpose of having a dst if we need to drop it ?

Adding code in GRO would be fine if someone explains me the purpose of doing apparently useless work.

(refcounting on dst is not exactly free)

Quoted 8 lines

In the GRO case, the dst is only dropped on the packets which have been merged and therefore need to be freed (the GRO_MERGED_FREE case). It's not being thrown away for the overall frame, just metadata that has been duplicated on each individual frame, similar to the metadata in struct sk_buff itself. And while it is not used by the IP stack there are other consumers (eBPF/OVS/etc.). This entire process is controlled by the COLLECT_METADATA flag on tunnels, so there is no cost in situations where it is not actually used.

Right. There were thoughts around leveraging a per CPU scratch buffer without a refcount and turn it into a full reference when the packet gets enqueued somewhere but the need hasn't really come up yet.

Jesse, is this what you have in mind:

net/core/dev.c +0 -0

index cc9e365..3a5e96d 100644
@@ -4548,9 +4548,10 @@ static gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb)
                 break;
 

net/core/dev.c static gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb) · line 4556

Quoted 9 lines

net/core/dev.c +3 -2

         case GRO_MERGED_FREE:
-               if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD)
+               if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD) {
+                       skb_release_head_state(skb);
                         kmem_cache_free(skbuff_head_cache, skb);
-               else
+               } else
                         __kfree_skb(skb);
                 break;

So I've tested the below patch (same as one above with minor modifications made to make it compile) and it worked - no memory leak. Should I submit this or...?

Quoted 32 lines · John

Quoted 30 lines · Thomas Graf

Quoted 7 lines · Jesse Gross

Quoted 6 lines · Eric Dumazet

So what is the purpose of having a dst if we need to drop it ?

Adding code in GRO would be fine if someone explains me the purpose of doing apparently useless work.

(refcounting on dst is not exactly free)

Quoted 8 lines

In the GRO case, the dst is only dropped on the packets which have been merged and therefore need to be freed (the GRO_MERGED_FREE case). It's not being thrown away for the overall frame, just metadata that has been duplicated on each individual frame, similar to the metadata in struct sk_buff itself. And while it is not used by the IP stack there are other consumers (eBPF/OVS/etc.). This entire process is controlled by the COLLECT_METADATA flag on tunnels, so there is no cost in situations where it is not actually used.

Right. There were thoughts around leveraging a per CPU scratch buffer without a refcount and turn it into a full reference when the packet gets enqueued somewhere but the need hasn't really come up yet.

Jesse, is this what you have in mind:

net/core/dev.c +0 -0

index cc9e365..3a5e96d 100644
@@ -4548,9 +4548,10 @@ static gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb)
                 break;
 

Quoted 12 lines

net/core/dev.c static gro_result_t napi_skb_finish(gro_result_t ret, struct sk_buff *skb) · line 4556

Quoted 9 lines

net/core/dev.c +3 -2

         case GRO_MERGED_FREE:
-               if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD)
+               if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD) {
+                       skb_release_head_state(skb);
                         kmem_cache_free(skbuff_head_cache, skb);
-               else
+               } else
                         __kfree_skb(skb);
                 break;

So I've tested the below patch (same as one above with minor modifications made to make it compile) and it worked - no memory leak. Should I submit this or...?

Unfortunately fix is not complete.

As someone mentioned, GRO should not aggregate packets having different dst.

This part is hard to achieve, as a pointer comparison wont be enough :
Each skb has its own meta dst allocation.

Quite frankly, I would rather disable GRO for packets with a dst, instead of making GRO dog slow.

I have a patch that implements the comparison between dsts. For packets without a dst, there isn't really a cost and if we do have a dst then GRO is still a benefit. So it seems like it is worth doing, even if it is more expensive than is ideal.

Quoted 4 lines · Jesse Gross

I have a patch that implements the comparison between dsts. For packets without a dst, there isn't really a cost and if we do have a dst then GRO is still a benefit. So it seems like it is worth doing, even if it is more expensive than is ideal.

You guys really want to kill GRO performance.

Really the aggregation should happen at the first layer (ethernet device), instead of doing it after tunnel decap.

This is already done for GRE, IPIP, SIT, ...

GRO having to access metadata looks wrong, if you think about trying to do the same function in hardware (offload)