From: Antoine Tenart <atenart@kernel.org> Date: 2021-03-25 15:36:26
Hi,
The series fixes an issue were a shared ip_tunnel_info is modified when
PMTU triggers an ICMP reply in vxlan and geneve, making following
packets in that flow to have a wrong destination address if the flow
isn't updated. A detailled information is given in each of the two
commits.
This was tested manually with OVS and I ran the PTMU selftests with
kmemleak enabled (all OK, none was skipped).
Thanks!
Antoine
Antoine Tenart (2):
vxlan: do not modify the shared tunnel info when PMTU triggers an ICMP
reply
geneve: do not modify the shared tunnel info when PMTU triggers an
ICMP reply
drivers/net/geneve.c | 24 ++++++++++++++++++++----
drivers/net/vxlan.c | 18 ++++++++++++++----
2 files changed, 34 insertions(+), 8 deletions(-)
--
2.30.2
From: Antoine Tenart <atenart@kernel.org> Date: 2021-03-25 15:36:26
When the interface is part of a bridge or an Open vSwitch port and a
packet exceed a PMTU estimate, an ICMP reply is sent to the sender. When
using the external mode (collect metadata) the source and destination
addresses are reversed, so that Open vSwitch can match the packet
against an existing (reverse) flow.
But inverting the source and destination addresses in the shared
ip_tunnel_info will make following packets of the flow to use a wrong
destination address (packets will be tunnelled to itself), if the flow
isn't updated. Which happens with Open vSwitch, until the flow times
out.
Fixes this by uncloning the skb's ip_tunnel_info before inverting its
source and destination addresses, so that the modification will only be
made for the PTMU packet, not the following ones.
Fixes: c1a800e88dbf ("geneve: Support for PMTU discovery on directly bridged links")
Tested-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
drivers/net/geneve.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
From: Antoine Tenart <atenart@kernel.org> Date: 2021-03-25 15:36:26
When the interface is part of a bridge or an Open vSwitch port and a
packet exceed a PMTU estimate, an ICMP reply is sent to the sender. When
using the external mode (collect metadata) the source and destination
addresses are reversed, so that Open vSwitch can match the packet
against an existing (reverse) flow.
But inverting the source and destination addresses in the shared
ip_tunnel_info will make following packets of the flow to use a wrong
destination address (packets will be tunnelled to itself), if the flow
isn't updated. Which happens with Open vSwitch, until the flow times
out.
Fixes this by uncloning the skb's ip_tunnel_info before inverting its
source and destination addresses, so that the modification will only be
made for the PTMU packet, not the following ones.
Fixes: fc68c99577cc ("vxlan: Support for PMTU discovery on directly bridged links")
Tested-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
drivers/net/vxlan.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
On Thu, 25 Mar 2021 16:35:31 +0100
Antoine Tenart [off-list ref] wrote:
Hi,
The series fixes an issue were a shared ip_tunnel_info is modified when
PMTU triggers an ICMP reply in vxlan and geneve, making following
packets in that flow to have a wrong destination address if the flow
isn't updated. A detailled information is given in each of the two
commits.
This was tested manually with OVS and I ran the PTMU selftests with
kmemleak enabled (all OK, none was skipped).
Thanks!
Antoine
Antoine Tenart (2):
vxlan: do not modify the shared tunnel info when PMTU triggers an ICMP
reply
geneve: do not modify the shared tunnel info when PMTU triggers an
ICMP reply
For the series,
Reviewed-by: Stefano Brivio <redacted>
Thanks for fixing this!
--
Stefano
Hello:
This series was applied to netdev/net.git (refs/heads/master):
On Thu, 25 Mar 2021 16:35:31 +0100 you wrote:
Hi,
The series fixes an issue were a shared ip_tunnel_info is modified when
PMTU triggers an ICMP reply in vxlan and geneve, making following
packets in that flow to have a wrong destination address if the flow
isn't updated. A detailled information is given in each of the two
commits.
[...]
Hello Antoine,
On Thu 25 Mar 2021 at 17:35, Antoine Tenart [off-list ref] wrote:
quoted hunk
When the interface is part of a bridge or an Open vSwitch port and a
packet exceed a PMTU estimate, an ICMP reply is sent to the sender. When
using the external mode (collect metadata) the source and destination
addresses are reversed, so that Open vSwitch can match the packet
against an existing (reverse) flow.
But inverting the source and destination addresses in the shared
ip_tunnel_info will make following packets of the flow to use a wrong
destination address (packets will be tunnelled to itself), if the flow
isn't updated. Which happens with Open vSwitch, until the flow times
out.
Fixes this by uncloning the skb's ip_tunnel_info before inverting its
source and destination addresses, so that the modification will only be
made for the PTMU packet, not the following ones.
Fixes: fc68c99577cc ("vxlan: Support for PMTU discovery on directly bridged links")
Tested-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Eelco Chaudron <echaudro@redhat.com>
Signed-off-by: Antoine Tenart <atenart@kernel.org>
---
drivers/net/vxlan.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
We have been getting memleaks in one of our tests that point to this
code (test deletes vxlan device while running traffic redirected by OvS
TC at the same time):
unreferenced object 0xffff8882d0114200 (size 256):
comm "softirq", pid 0, jiffies 4296140292 (age 1435.992s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 3b 85 84 ff ff ff ff .........;......
a1 26 b7 83 ff ff ff ff 00 00 00 00 00 00 00 00 .&..............
backtrace:
[<0000000097659d47>] metadata_dst_alloc+0x1f/0x470
[<000000007571c30f>] tun_dst_unclone+0xee/0x360 [vxlan]
[<00000000d2dcfd00>] vxlan_xmit_one+0x131d/0x2a00 [vxlan]
[<00000000281572b6>] vxlan_xmit+0x8e6/0x4cd0 [vxlan]
[<00000000d49d33fe>] dev_hard_start_xmit+0x1ba/0x710
[<00000000eac444f5>] __dev_queue_xmit+0x17c5/0x25f0
[<000000005fbd8585>] tcf_mirred_act+0xb1d/0xf70 [act_mirred]
[<0000000064b6eb2d>] tcf_action_exec+0x10e/0x350
[<00000000352821e8>] fl_classify+0x4e3/0x610 [cls_flower]
[<0000000011d3f765>] tcf_classify+0x33d/0x800
[<000000006c69b225>] __netif_receive_skb_core+0x18d6/0x2ae0
[<00000000dd256fe3>] __netif_receive_skb_one_core+0xaf/0x180
[<0000000065d43bd6>] process_backlog+0x2e3/0x710
[<00000000964357ae>] __napi_poll+0x9f/0x560
[<0000000059a93cf6>] net_rx_action+0x357/0xa60
[<00000000766481bc>] __do_softirq+0x282/0x94e
Looking at the code the potential issue seems to be that
tun_dst_unclone() creates new metadata_dst instance with refcount==1,
increments the refcount with dst_hold() to value 2, then returns it.
This seems to imply that caller is expected to release one of the
references (second one if for skb), but none of the callers (including
original dev_fill_metadata_dst()) do that, so I guess I'm
misunderstanding something here.
Any tips or suggestions?
We have been getting memleaks in one of our tests that point to this
code (test deletes vxlan device while running traffic redirected by OvS
TC at the same time):
unreferenced object 0xffff8882d0114200 (size 256):
comm "softirq", pid 0, jiffies 4296140292 (age 1435.992s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 3b 85 84 ff ff ff ff .........;......
a1 26 b7 83 ff ff ff ff 00 00 00 00 00 00 00 00 .&..............
backtrace:
[<0000000097659d47>] metadata_dst_alloc+0x1f/0x470
[<000000007571c30f>] tun_dst_unclone+0xee/0x360 [vxlan]
[<00000000d2dcfd00>] vxlan_xmit_one+0x131d/0x2a00 [vxlan]
[<00000000281572b6>] vxlan_xmit+0x8e6/0x4cd0 [vxlan]
[<00000000d49d33fe>] dev_hard_start_xmit+0x1ba/0x710
[<00000000eac444f5>] __dev_queue_xmit+0x17c5/0x25f0
[<000000005fbd8585>] tcf_mirred_act+0xb1d/0xf70 [act_mirred]
[<0000000064b6eb2d>] tcf_action_exec+0x10e/0x350
[<00000000352821e8>] fl_classify+0x4e3/0x610 [cls_flower]
[<0000000011d3f765>] tcf_classify+0x33d/0x800
[<000000006c69b225>] __netif_receive_skb_core+0x18d6/0x2ae0
[<00000000dd256fe3>] __netif_receive_skb_one_core+0xaf/0x180
[<0000000065d43bd6>] process_backlog+0x2e3/0x710
[<00000000964357ae>] __napi_poll+0x9f/0x560
[<0000000059a93cf6>] net_rx_action+0x357/0xa60
[<00000000766481bc>] __do_softirq+0x282/0x94e
Looking at the code the potential issue seems to be that
tun_dst_unclone() creates new metadata_dst instance with refcount==1,
increments the refcount with dst_hold() to value 2, then returns it.
This seems to imply that caller is expected to release one of the
references (second one if for skb), but none of the callers (including
original dev_fill_metadata_dst()) do that, so I guess I'm
misunderstanding something here.
Any tips or suggestions?
I'd say there is no need to increase the dst refcount here after calling
metadata_dst_alloc, as the metadata is local to the skb and the dst
refcount was already initialized to 1. This might be an issue with
commit fc4099f17240 ("openvswitch: Fix egress tunnel info."); I CCed
Pravin, he might recall if there was a reason to increase the refcount.
Thanks,
Antoine
We have been getting memleaks in one of our tests that point to this
code (test deletes vxlan device while running traffic redirected by OvS
TC at the same time):
unreferenced object 0xffff8882d0114200 (size 256):
comm "softirq", pid 0, jiffies 4296140292 (age 1435.992s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 3b 85 84 ff ff ff ff .........;......
a1 26 b7 83 ff ff ff ff 00 00 00 00 00 00 00 00 .&..............
backtrace:
[<0000000097659d47>] metadata_dst_alloc+0x1f/0x470
[<000000007571c30f>] tun_dst_unclone+0xee/0x360 [vxlan]
[<00000000d2dcfd00>] vxlan_xmit_one+0x131d/0x2a00 [vxlan]
[<00000000281572b6>] vxlan_xmit+0x8e6/0x4cd0 [vxlan]
[<00000000d49d33fe>] dev_hard_start_xmit+0x1ba/0x710
[<00000000eac444f5>] __dev_queue_xmit+0x17c5/0x25f0
[<000000005fbd8585>] tcf_mirred_act+0xb1d/0xf70 [act_mirred]
[<0000000064b6eb2d>] tcf_action_exec+0x10e/0x350
[<00000000352821e8>] fl_classify+0x4e3/0x610 [cls_flower]
[<0000000011d3f765>] tcf_classify+0x33d/0x800
[<000000006c69b225>] __netif_receive_skb_core+0x18d6/0x2ae0
[<00000000dd256fe3>] __netif_receive_skb_one_core+0xaf/0x180
[<0000000065d43bd6>] process_backlog+0x2e3/0x710
[<00000000964357ae>] __napi_poll+0x9f/0x560
[<0000000059a93cf6>] net_rx_action+0x357/0xa60
[<00000000766481bc>] __do_softirq+0x282/0x94e
Looking at the code the potential issue seems to be that
tun_dst_unclone() creates new metadata_dst instance with refcount==1,
increments the refcount with dst_hold() to value 2, then returns it.
This seems to imply that caller is expected to release one of the
references (second one if for skb), but none of the callers (including
original dev_fill_metadata_dst()) do that, so I guess I'm
misunderstanding something here.
Any tips or suggestions?
I'd say there is no need to increase the dst refcount here after calling
metadata_dst_alloc, as the metadata is local to the skb and the dst
refcount was already initialized to 1. This might be an issue with
commit fc4099f17240 ("openvswitch: Fix egress tunnel info."); I CCed
Pravin, he might recall if there was a reason to increase the refcount.
From: Antoine Tenart <atenart@kernel.org> Date: 2022-01-28 17:01:42
Hi Vlad,
Quoting Vlad Buslov (2022-01-20 13:58:18)
On Thu 20 Jan 2022 at 12:27, Antoine Tenart [off-list ref] wrote:
quoted
Quoting Vlad Buslov (2022-01-20 08:38:05)
quoted
We have been getting memleaks in one of our tests that point to this
code (test deletes vxlan device while running traffic redirected by OvS
TC at the same time):
unreferenced object 0xffff8882d0114200 (size 256):
[<0000000097659d47>] metadata_dst_alloc+0x1f/0x470
[<000000007571c30f>] tun_dst_unclone+0xee/0x360 [vxlan]
[<00000000d2dcfd00>] vxlan_xmit_one+0x131d/0x2a00 [vxlan]
[...]
quoted
quoted
Looking at the code the potential issue seems to be that
tun_dst_unclone() creates new metadata_dst instance with refcount==1,
increments the refcount with dst_hold() to value 2, then returns it.
This seems to imply that caller is expected to release one of the
references (second one if for skb), but none of the callers (including
original dev_fill_metadata_dst()) do that, so I guess I'm
misunderstanding something here.
Any tips or suggestions?
I'd say there is no need to increase the dst refcount here after calling
metadata_dst_alloc, as the metadata is local to the skb and the dst
refcount was already initialized to 1. This might be an issue with
commit fc4099f17240 ("openvswitch: Fix egress tunnel info."); I CCed
Pravin, he might recall if there was a reason to increase the refcount.
I tried to remove the dst_hold(), but that caused underflows[0], so I
guess the current reference counting is required at least for some
use-cases.
[0]:
[ 118.803011] dst_release: dst:000000001fc13e61 refcnt:-2
[...]
I finally had some time to look at this. Does the diff below fix your
issue?
On Fri 28 Jan 2022 at 19:01, Antoine Tenart [off-list ref] wrote:
Hi Vlad,
Quoting Vlad Buslov (2022-01-20 13:58:18)
quoted
On Thu 20 Jan 2022 at 12:27, Antoine Tenart [off-list ref] wrote:
quoted
Quoting Vlad Buslov (2022-01-20 08:38:05)
quoted
We have been getting memleaks in one of our tests that point to this
code (test deletes vxlan device while running traffic redirected by OvS
TC at the same time):
unreferenced object 0xffff8882d0114200 (size 256):
[<0000000097659d47>] metadata_dst_alloc+0x1f/0x470
[<000000007571c30f>] tun_dst_unclone+0xee/0x360 [vxlan]
[<00000000d2dcfd00>] vxlan_xmit_one+0x131d/0x2a00 [vxlan]
[...]
quoted
quoted
quoted
Looking at the code the potential issue seems to be that
tun_dst_unclone() creates new metadata_dst instance with refcount==1,
increments the refcount with dst_hold() to value 2, then returns it.
This seems to imply that caller is expected to release one of the
references (second one if for skb), but none of the callers (including
original dev_fill_metadata_dst()) do that, so I guess I'm
misunderstanding something here.
Any tips or suggestions?
I'd say there is no need to increase the dst refcount here after calling
metadata_dst_alloc, as the metadata is local to the skb and the dst
refcount was already initialized to 1. This might be an issue with
commit fc4099f17240 ("openvswitch: Fix egress tunnel info."); I CCed
Pravin, he might recall if there was a reason to increase the refcount.
I tried to remove the dst_hold(), but that caused underflows[0], so I
guess the current reference counting is required at least for some
use-cases.
[0]:
[ 118.803011] dst_release: dst:000000001fc13e61 refcnt:-2
[...]
I finally had some time to look at this. Does the diff below fix your
issue?
Yes, with the patch applied I'm no longer able to reproduce memory leak.
Thanks for fixing this!
From: Antoine Tenart <atenart@kernel.org> Date: 2022-01-31 13:26:54
Quoting Vlad Buslov (2022-01-31 12:26:47)
On Fri 28 Jan 2022 at 19:01, Antoine Tenart [off-list ref] wrote:
quoted
I finally had some time to look at this. Does the diff below fix your
issue?
Yes, with the patch applied I'm no longer able to reproduce memory leak.
Thanks for fixing this!
Thanks for testing. I'll send a formal patch, can I add your Tested-by?
Also, do you know how to trigger the following code path in OVS
https://elixir.bootlin.com/linux/latest/source/net/openvswitch/actions.c#L944
? Would be good (not required) to test it, to ensure the fix doesn't
break it.
Thanks,
Antoine
Thanks for the suggestion! I did run all 8 ptmu_*_ovs_* tests, they all
passed but didn't trigger a call to dev_fill_metadata_dst in
net/openvswitch/actions.c.
To be sure there wasn't a misunderstanding: I did test the PTMU code
path in Geneve/VXLAN (while one of the endpoint is an OVS port); but the
net/openvswitch/actions.c code path is something different, used to
retrieve tunnel egress info. I don't know when/how this is used by OVS.
Thanks,
Antoine