From: Oz Shlomo <hidden> Date: 2022-05-12 18:28:41
Connections leaving the established state (due to RST / FIN TCP packets)
set the flow table teardown flag. The packet path continues to set lower
timeout value as per the new TCP state but the offload flag remains set.
Hence, the conntrack garbage collector may race to undo the timeout
adjustment of the packet path, leaving the conntrack entry in place with
the internal offload timeout (one day).
Avoid ct gc timeout overwrite by flagging teared down flowtable
connections.
On the nftables side we only need to allow established TCP connections to
create a flow offload entry. Since we can not guaruantee that
flow_offload_teardown is called by a TCP FIN packet we also need to make
sure that flow_offload_fixup_ct is also called in flow_offload_del
and only fixes up established TCP connections.
Fixes: 1e5b2471bcc4 ("netfilter: nf_flow_table: teardown flow timeout race")
Signed-off-by: Oz Shlomo <redacted>
Signed-off-by: Sven Auhagen <redacted>
------------
v1 -> v2 changes
- Add flow_teardown flag
- Add nftables handling
- Fixup timeout according to the current ct state
---
include/uapi/linux/netfilter/nf_conntrack_common.h | 6 +++-
net/netfilter/nf_conntrack_core.c | 3 +-
net/netfilter/nf_flow_table_core.c | 42 +++++++++-------------
net/netfilter/nft_flow_offload.c | 2 ++
4 files changed, 25 insertions(+), 28 deletions(-)
@@ -118,6 +118,10 @@ enum ip_conntrack_status {IPS_HW_OFFLOAD_BIT=15,IPS_HW_OFFLOAD=(1<<IPS_HW_OFFLOAD_BIT),+/* offloaded conntrack entry is marked for deletion. */+IPS_OFFLOAD_TEARDOWN_BIT=16,+IPS_OFFLOAD_TEARDOWN=(1<<IPS_OFFLOAD_TEARDOWN_BIT),+/* Be careful here, modifying these bits can make things messy,*sodon'tletusersmodifythemdirectly.*/
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2022-05-16 10:56:45
On Thu, May 12, 2022 at 09:28:03PM +0300, Oz Shlomo wrote:
Connections leaving the established state (due to RST / FIN TCP packets)
set the flow table teardown flag. The packet path continues to set lower
timeout value as per the new TCP state but the offload flag remains set.
Hence, the conntrack garbage collector may race to undo the timeout
adjustment of the packet path, leaving the conntrack entry in place with
the internal offload timeout (one day).
Avoid ct gc timeout overwrite by flagging teared down flowtable
connections.
On the nftables side we only need to allow established TCP connections to
create a flow offload entry. Since we can not guaruantee that
flow_offload_teardown is called by a TCP FIN packet we also need to make
sure that flow_offload_fixup_ct is also called in flow_offload_del
and only fixes up established TCP connections.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
If this does work, could you keep IPS_OFFLOAD_TEARDOWN_BIT internal,
ie. no in uapi? Define it at include/net/netfilter/nf_conntrack.h and
add a comment regarding this to avoid an overlap in the future.
quoted hunk
+ if (!test_bit(IPS_OFFLOAD_TEARDOWN_BIT, &tmp->status))
+ nf_ct_offload_timeout(tmp);
continue;
}
Very unlikely, but race might still happen between fixup and
clear IPS_OFFLOAD_BIT with gc below?
Without checking from the packet path, the conntrack gc might race to
refresh the timeout, I don't see a 100% race free solution.
Probably update the nf_ct_offload_timeout to a shorter value than a
day would mitigate this issue too.
From: Sven Auhagen <hidden> Date: 2022-05-16 11:18:34
On Mon, May 16, 2022 at 12:56:38PM +0200, Pablo Neira Ayuso wrote:
On Thu, May 12, 2022 at 09:28:03PM +0300, Oz Shlomo wrote:
quoted
Connections leaving the established state (due to RST / FIN TCP packets)
set the flow table teardown flag. The packet path continues to set lower
timeout value as per the new TCP state but the offload flag remains set.
Hence, the conntrack garbage collector may race to undo the timeout
adjustment of the packet path, leaving the conntrack entry in place with
the internal offload timeout (one day).
Avoid ct gc timeout overwrite by flagging teared down flowtable
connections.
On the nftables side we only need to allow established TCP connections to
create a flow offload entry. Since we can not guaruantee that
flow_offload_teardown is called by a TCP FIN packet we also need to make
sure that flow_offload_fixup_ct is also called in flow_offload_del
and only fixes up established TCP connections.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
If this does work, could you keep IPS_OFFLOAD_TEARDOWN_BIT internal,
ie. no in uapi? Define it at include/net/netfilter/nf_conntrack.h and
add a comment regarding this to avoid an overlap in the future.
quoted
+ if (!test_bit(IPS_OFFLOAD_TEARDOWN_BIT, &tmp->status))
+ nf_ct_offload_timeout(tmp);
continue;
}
Very unlikely, but race might still happen between fixup and
clear IPS_OFFLOAD_BIT with gc below?
Without checking from the packet path, the conntrack gc might race to
refresh the timeout, I don't see a 100% race free solution.
Probably update the nf_ct_offload_timeout to a shorter value than a
day would mitigate this issue too.
This section of the code is now protected by IPS_OFFLOAD_TEARDOWN_BIT
which will prevent the update via nf_ct_offload_timeout.
We set it at the beginning of flow_offload_del and flow_offload_teardown.
Since flow_offload_teardown is only called on TCP packets
we also need to set it at flow_offload_del to prevent the race.
This should prevent the race at this point.
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2022-05-16 11:37:46
On Mon, May 16, 2022 at 01:18:17PM +0200, Sven Auhagen wrote:
On Mon, May 16, 2022 at 12:56:38PM +0200, Pablo Neira Ayuso wrote:
quoted
On Thu, May 12, 2022 at 09:28:03PM +0300, Oz Shlomo wrote:
quoted
Connections leaving the established state (due to RST / FIN TCP packets)
set the flow table teardown flag. The packet path continues to set lower
timeout value as per the new TCP state but the offload flag remains set.
Hence, the conntrack garbage collector may race to undo the timeout
adjustment of the packet path, leaving the conntrack entry in place with
the internal offload timeout (one day).
Avoid ct gc timeout overwrite by flagging teared down flowtable
connections.
On the nftables side we only need to allow established TCP connections to
create a flow offload entry. Since we can not guaruantee that
flow_offload_teardown is called by a TCP FIN packet we also need to make
sure that flow_offload_fixup_ct is also called in flow_offload_del
and only fixes up established TCP connections.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
quoted
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
If this does work, could you keep IPS_OFFLOAD_TEARDOWN_BIT internal,
ie. no in uapi? Define it at include/net/netfilter/nf_conntrack.h and
add a comment regarding this to avoid an overlap in the future.
quoted
+ if (!test_bit(IPS_OFFLOAD_TEARDOWN_BIT, &tmp->status))
+ nf_ct_offload_timeout(tmp);
continue;
}
Very unlikely, but race might still happen between fixup and
clear IPS_OFFLOAD_BIT with gc below?
Without checking from the packet path, the conntrack gc might race to
refresh the timeout, I don't see a 100% race free solution.
Probably update the nf_ct_offload_timeout to a shorter value than a
day would mitigate this issue too.
This section of the code is now protected by IPS_OFFLOAD_TEARDOWN_BIT
which will prevent the update via nf_ct_offload_timeout.
We set it at the beginning of flow_offload_del and flow_offload_teardown.
Since flow_offload_teardown is only called on TCP packets
we also need to set it at flow_offload_del to prevent the race.
This should prevent the race at this point.
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2022-05-16 12:06:54
On Mon, May 16, 2022 at 01:37:40PM +0200, Pablo Neira Ayuso wrote:
On Mon, May 16, 2022 at 01:18:17PM +0200, Sven Auhagen wrote:
quoted
On Mon, May 16, 2022 at 12:56:38PM +0200, Pablo Neira Ayuso wrote:
quoted
On Thu, May 12, 2022 at 09:28:03PM +0300, Oz Shlomo wrote:
quoted
Connections leaving the established state (due to RST / FIN TCP packets)
set the flow table teardown flag. The packet path continues to set lower
timeout value as per the new TCP state but the offload flag remains set.
Hence, the conntrack garbage collector may race to undo the timeout
adjustment of the packet path, leaving the conntrack entry in place with
the internal offload timeout (one day).
Avoid ct gc timeout overwrite by flagging teared down flowtable
connections.
On the nftables side we only need to allow established TCP connections to
create a flow offload entry. Since we can not guaruantee that
flow_offload_teardown is called by a TCP FIN packet we also need to make
sure that flow_offload_fixup_ct is also called in flow_offload_del
and only fixes up established TCP connections.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
quoted
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
If this does work, could you keep IPS_OFFLOAD_TEARDOWN_BIT internal,
ie. no in uapi? Define it at include/net/netfilter/nf_conntrack.h and
add a comment regarding this to avoid an overlap in the future.
quoted
+ if (!test_bit(IPS_OFFLOAD_TEARDOWN_BIT, &tmp->status))
+ nf_ct_offload_timeout(tmp);
continue;
}
Very unlikely, but race might still happen between fixup and
clear IPS_OFFLOAD_BIT with gc below?
Without checking from the packet path, the conntrack gc might race to
refresh the timeout, I don't see a 100% race free solution.
Probably update the nf_ct_offload_timeout to a shorter value than a
day would mitigate this issue too.
This section of the code is now protected by IPS_OFFLOAD_TEARDOWN_BIT
which will prevent the update via nf_ct_offload_timeout.
We set it at the beginning of flow_offload_del and flow_offload_teardown.
Since flow_offload_teardown is only called on TCP packets
we also need to set it at flow_offload_del to prevent the race.
This should prevent the race at this point.
This chunk is not required, from ruleset users can do
... ct status assured ...
instead.
Maybe this should be mentioned in the manual or wiki if
it is not necessary in the flow offload code.
Yes, documentation and wiki can be updated.
Users might want to offload the flow at a later stage in the TCP
connection.
Well, actually there is not later stage than established, anything
after established are TCP teardown states.
What's the issue with allowing to offload from SYN_RECV state?
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2022-05-16 12:13:13
On Mon, May 16, 2022 at 12:56:41PM +0200, Pablo Neira Ayuso wrote:
On Thu, May 12, 2022 at 09:28:03PM +0300, Oz Shlomo wrote:
quoted
Connections leaving the established state (due to RST / FIN TCP packets)
set the flow table teardown flag. The packet path continues to set lower
timeout value as per the new TCP state but the offload flag remains set.
Hence, the conntrack garbage collector may race to undo the timeout
adjustment of the packet path, leaving the conntrack entry in place with
the internal offload timeout (one day).
Avoid ct gc timeout overwrite by flagging teared down flowtable
connections.
On the nftables side we only need to allow established TCP connections to
create a flow offload entry. Since we can not guaruantee that
flow_offload_teardown is called by a TCP FIN packet we also need to make
sure that flow_offload_fixup_ct is also called in flow_offload_del
and only fixes up established TCP connections.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
I'm attaching an incomplete sketch patch. My goal is to avoid the
extra IPS_ bit.
From: Sven Auhagen <hidden> Date: 2022-05-16 12:17:12
On Mon, May 16, 2022 at 02:06:28PM +0200, Pablo Neira Ayuso wrote:
On Mon, May 16, 2022 at 01:37:40PM +0200, Pablo Neira Ayuso wrote:
quoted
On Mon, May 16, 2022 at 01:18:17PM +0200, Sven Auhagen wrote:
quoted
On Mon, May 16, 2022 at 12:56:38PM +0200, Pablo Neira Ayuso wrote:
quoted
On Thu, May 12, 2022 at 09:28:03PM +0300, Oz Shlomo wrote:
quoted
Connections leaving the established state (due to RST / FIN TCP packets)
set the flow table teardown flag. The packet path continues to set lower
timeout value as per the new TCP state but the offload flag remains set.
Hence, the conntrack garbage collector may race to undo the timeout
adjustment of the packet path, leaving the conntrack entry in place with
the internal offload timeout (one day).
Avoid ct gc timeout overwrite by flagging teared down flowtable
connections.
On the nftables side we only need to allow established TCP connections to
create a flow offload entry. Since we can not guaruantee that
flow_offload_teardown is called by a TCP FIN packet we also need to make
sure that flow_offload_fixup_ct is also called in flow_offload_del
and only fixes up established TCP connections.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
quoted
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
If this does work, could you keep IPS_OFFLOAD_TEARDOWN_BIT internal,
ie. no in uapi? Define it at include/net/netfilter/nf_conntrack.h and
add a comment regarding this to avoid an overlap in the future.
quoted
+ if (!test_bit(IPS_OFFLOAD_TEARDOWN_BIT, &tmp->status))
+ nf_ct_offload_timeout(tmp);
continue;
}
Very unlikely, but race might still happen between fixup and
clear IPS_OFFLOAD_BIT with gc below?
Without checking from the packet path, the conntrack gc might race to
refresh the timeout, I don't see a 100% race free solution.
Probably update the nf_ct_offload_timeout to a shorter value than a
day would mitigate this issue too.
This section of the code is now protected by IPS_OFFLOAD_TEARDOWN_BIT
which will prevent the update via nf_ct_offload_timeout.
We set it at the beginning of flow_offload_del and flow_offload_teardown.
Since flow_offload_teardown is only called on TCP packets
we also need to set it at flow_offload_del to prevent the race.
This should prevent the race at this point.
This chunk is not required, from ruleset users can do
... ct status assured ...
instead.
Maybe this should be mentioned in the manual or wiki if
it is not necessary in the flow offload code.
Yes, documentation and wiki can be updated.
Users might want to offload the flow at a later stage in the TCP
connection.
Well, actually there is not later stage than established, anything
after established are TCP teardown states.
What's the issue with allowing to offload from SYN_RECV state?
There were multiple problem in general with the code.
flow_offload_fixup_tcp always moves a TCP connection
to established even if it is in FIN or CLOSE.
The flowoffload_del function was always setting the TCP timeout
to ESTABLISHED timeout even when the state was in CLOSE and therefore
creating a very long lasting dead state.
Since we might miss or bump packets to slow path, we do not know
what will happen there when we are still in SYN_RECV.
We will have a better knowledge of the TCP state when we are in
established first and we know that we are either still in it or
we have moved past it to a closing state.
From: Sven Auhagen <hidden> Date: 2022-05-16 12:23:11
On Mon, May 16, 2022 at 02:13:03PM +0200, Pablo Neira Ayuso wrote:
On Mon, May 16, 2022 at 12:56:41PM +0200, Pablo Neira Ayuso wrote:
quoted
On Thu, May 12, 2022 at 09:28:03PM +0300, Oz Shlomo wrote:
quoted
Connections leaving the established state (due to RST / FIN TCP packets)
set the flow table teardown flag. The packet path continues to set lower
timeout value as per the new TCP state but the offload flag remains set.
Hence, the conntrack garbage collector may race to undo the timeout
adjustment of the packet path, leaving the conntrack entry in place with
the internal offload timeout (one day).
Avoid ct gc timeout overwrite by flagging teared down flowtable
connections.
On the nftables side we only need to allow established TCP connections to
create a flow offload entry. Since we can not guaruantee that
flow_offload_teardown is called by a TCP FIN packet we also need to make
sure that flow_offload_fixup_ct is also called in flow_offload_del
and only fixes up established TCP connections.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
I'm attaching an incomplete sketch patch. My goal is to avoid the
extra IPS_ bit.
You might create a race with ct gc that will remove the ct
if it is in close or end of close and before flow offload teardown is running
so flow offload teardown might access memory that was freed.
It is not a very likely scenario but never the less it might happen now
since the IPS_OFFLOAD_BIT is not set and the state might just time out.
If someone sets a very small TCP CLOSE timeout it gets more likely.
So Oz and myself were debatting about three possible cases/problems:
1. ct gc sets timeout even though the state is in CLOSE/FIN because the
IPS_OFFLOAD is still set but the flow is in teardown
2. ct gc removes the ct because the IPS_OFFLOAD is not set and
the CLOSE timeout is reached before the flow offload del
3. tcp ct is always set to ESTABLISHED with a very long timeout
in flow offload teardown/delete even though the state is already
CLOSED.
Also as a remark we can not assume that the FIN or RST packet is hitting
flow table teardown as the packet might get bumped to the slow path in
nftables.
Best
Sven
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2022-05-16 12:43:18
On Mon, May 16, 2022 at 02:23:00PM +0200, Sven Auhagen wrote:
On Mon, May 16, 2022 at 02:13:03PM +0200, Pablo Neira Ayuso wrote:
quoted
On Mon, May 16, 2022 at 12:56:41PM +0200, Pablo Neira Ayuso wrote:
quoted
On Thu, May 12, 2022 at 09:28:03PM +0300, Oz Shlomo wrote:
quoted
Connections leaving the established state (due to RST / FIN TCP packets)
set the flow table teardown flag. The packet path continues to set lower
timeout value as per the new TCP state but the offload flag remains set.
Hence, the conntrack garbage collector may race to undo the timeout
adjustment of the packet path, leaving the conntrack entry in place with
the internal offload timeout (one day).
Avoid ct gc timeout overwrite by flagging teared down flowtable
connections.
On the nftables side we only need to allow established TCP connections to
create a flow offload entry. Since we can not guaruantee that
flow_offload_teardown is called by a TCP FIN packet we also need to make
sure that flow_offload_fixup_ct is also called in flow_offload_del
and only fixes up established TCP connections.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
I'm attaching an incomplete sketch patch. My goal is to avoid the
extra IPS_ bit.
You might create a race with ct gc that will remove the ct
if it is in close or end of close and before flow offload teardown is running
so flow offload teardown might access memory that was freed.
flow object holds a reference to the ct object until it is released,
no use-after-free can happen.
It is not a very likely scenario but never the less it might happen now
since the IPS_OFFLOAD_BIT is not set and the state might just time out.
If someone sets a very small TCP CLOSE timeout it gets more likely.
So Oz and myself were debatting about three possible cases/problems:
1. ct gc sets timeout even though the state is in CLOSE/FIN because the
IPS_OFFLOAD is still set but the flow is in teardown
2. ct gc removes the ct because the IPS_OFFLOAD is not set and
the CLOSE timeout is reached before the flow offload del
OK.
3. tcp ct is always set to ESTABLISHED with a very long timeout
in flow offload teardown/delete even though the state is already
CLOSED.
Also as a remark we can not assume that the FIN or RST packet is hitting
flow table teardown as the packet might get bumped to the slow path in
nftables.
I assume this remark is related to 3.?
if IPS_OFFLOAD is unset, then conntrack would update the state
according to this FIN or RST.
Thanks for the summary.
From: Sven Auhagen <hidden> Date: 2022-05-16 13:02:28
On Mon, May 16, 2022 at 02:43:06PM +0200, Pablo Neira Ayuso wrote:
On Mon, May 16, 2022 at 02:23:00PM +0200, Sven Auhagen wrote:
quoted
On Mon, May 16, 2022 at 02:13:03PM +0200, Pablo Neira Ayuso wrote:
quoted
On Mon, May 16, 2022 at 12:56:41PM +0200, Pablo Neira Ayuso wrote:
quoted
On Thu, May 12, 2022 at 09:28:03PM +0300, Oz Shlomo wrote:
quoted
Connections leaving the established state (due to RST / FIN TCP packets)
set the flow table teardown flag. The packet path continues to set lower
timeout value as per the new TCP state but the offload flag remains set.
Hence, the conntrack garbage collector may race to undo the timeout
adjustment of the packet path, leaving the conntrack entry in place with
the internal offload timeout (one day).
Avoid ct gc timeout overwrite by flagging teared down flowtable
connections.
On the nftables side we only need to allow established TCP connections to
create a flow offload entry. Since we can not guaruantee that
flow_offload_teardown is called by a TCP FIN packet we also need to make
sure that flow_offload_fixup_ct is also called in flow_offload_del
and only fixes up established TCP connections.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
I'm attaching an incomplete sketch patch. My goal is to avoid the
extra IPS_ bit.
You might create a race with ct gc that will remove the ct
if it is in close or end of close and before flow offload teardown is running
so flow offload teardown might access memory that was freed.
flow object holds a reference to the ct object until it is released,
no use-after-free can happen.
Also if nf_ct_delete is called before flowtable delete?
Can you let me know why?
quoted
It is not a very likely scenario but never the less it might happen now
since the IPS_OFFLOAD_BIT is not set and the state might just time out.
If someone sets a very small TCP CLOSE timeout it gets more likely.
So Oz and myself were debatting about three possible cases/problems:
1. ct gc sets timeout even though the state is in CLOSE/FIN because the
IPS_OFFLOAD is still set but the flow is in teardown
2. ct gc removes the ct because the IPS_OFFLOAD is not set and
the CLOSE timeout is reached before the flow offload del
OK.
quoted
3. tcp ct is always set to ESTABLISHED with a very long timeout
in flow offload teardown/delete even though the state is already
CLOSED.
Also as a remark we can not assume that the FIN or RST packet is hitting
flow table teardown as the packet might get bumped to the slow path in
nftables.
I assume this remark is related to 3.?
Yes, exactly.
if IPS_OFFLOAD is unset, then conntrack would update the state
according to this FIN or RST.
It will move to a different TCP state anyways only the ct state
will be at IPS_OFFLOAD_BIT and prevent it from beeing garbage collected.
The timeout will be bumped back up as long as IPS_OFFLOAD_BIT is set
even though TCP might already be CLOSED.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
I'm attaching an incomplete sketch patch. My goal is to avoid the
extra IPS_ bit.
You might create a race with ct gc that will remove the ct
if it is in close or end of close and before flow offload teardown is running
so flow offload teardown might access memory that was freed.
flow object holds a reference to the ct object until it is released,
no use-after-free can happen.
Also if nf_ct_delete is called before flowtable delete?
Can you let me know why?
nf_ct_delete() removes the conntrack object from lists and it
decrements the reference counter by one.
flow_offload_free() also calls nf_ct_put(). flow_offload_alloc() bumps
the reference count on the conntrack object before creating the flow.
quoted
quoted
It is not a very likely scenario but never the less it might happen now
since the IPS_OFFLOAD_BIT is not set and the state might just time out.
If someone sets a very small TCP CLOSE timeout it gets more likely.
So Oz and myself were debatting about three possible cases/problems:
1. ct gc sets timeout even though the state is in CLOSE/FIN because the
IPS_OFFLOAD is still set but the flow is in teardown
2. ct gc removes the ct because the IPS_OFFLOAD is not set and
the CLOSE timeout is reached before the flow offload del
OK.
quoted
3. tcp ct is always set to ESTABLISHED with a very long timeout
in flow offload teardown/delete even though the state is already
CLOSED.
Also as a remark we can not assume that the FIN or RST packet is hitting
flow table teardown as the packet might get bumped to the slow path in
nftables.
I assume this remark is related to 3.?
Yes, exactly.
quoted
if IPS_OFFLOAD is unset, then conntrack would update the state
according to this FIN or RST.
It will move to a different TCP state anyways only the ct state
will be at IPS_OFFLOAD_BIT and prevent it from beeing garbage collected.
The timeout will be bumped back up as long as IPS_OFFLOAD_BIT is set
even though TCP might already be CLOSED.
If teardown fixes the ct state and timeout to established, and IPS_OFFLOAD is
unset, then the packet is passed up in a consistent state.
I made a patch, it is based on yours, it's attached:
- If flow timeout expires or rst/fin is seen, ct state and timeout is
fixed up (to established state) and IPS_OFFLOAD is unset.
- If rst/fin packet is seen, ct state and timeout is fixed up (to
established state) and IPS_OFFLOAD is unset. The packet continues
its travel up to the classic path, so conntrack triggers the
transition from established to one of the close states.
For the case 1., IPS_OFFLOAD is not set anymore, so conntrack gc
cannot race to reset the ct timeout anymore.
For the case 2., if gc conntrack ever removes the ct entry, then the
IPS_DYING bit is set, which implicitly triggers the teardown state
from the flowtable gc. The flowtable still holds a reference to the
ct object, so no UAF can happen.
For the case 3. the conntrack is set to ESTABLISHED with a long
timeout, yes. This is to deal with the two possible cases:
a) flowtable timeout expired, so conntrack recovers control on the
flow.
b) tcp rst/fin will take back the packet to slow path. The ct has been
fixed up to established state so it will trasition to one of the
close states.
Am I missing anything?
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2022-05-16 17:54:18
On Mon, May 16, 2022 at 02:17:00PM +0200, Sven Auhagen wrote:
On Mon, May 16, 2022 at 02:06:28PM +0200, Pablo Neira Ayuso wrote:
quoted
On Mon, May 16, 2022 at 01:37:40PM +0200, Pablo Neira Ayuso wrote:
quoted
On Mon, May 16, 2022 at 01:18:17PM +0200, Sven Auhagen wrote:
quoted
On Mon, May 16, 2022 at 12:56:38PM +0200, Pablo Neira Ayuso wrote:
quoted
On Thu, May 12, 2022 at 09:28:03PM +0300, Oz Shlomo wrote:
quoted
Connections leaving the established state (due to RST / FIN TCP packets)
set the flow table teardown flag. The packet path continues to set lower
timeout value as per the new TCP state but the offload flag remains set.
Hence, the conntrack garbage collector may race to undo the timeout
adjustment of the packet path, leaving the conntrack entry in place with
the internal offload timeout (one day).
Avoid ct gc timeout overwrite by flagging teared down flowtable
connections.
On the nftables side we only need to allow established TCP connections to
create a flow offload entry. Since we can not guaruantee that
flow_offload_teardown is called by a TCP FIN packet we also need to make
sure that flow_offload_fixup_ct is also called in flow_offload_del
and only fixes up established TCP connections.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
quoted
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
If this does work, could you keep IPS_OFFLOAD_TEARDOWN_BIT internal,
ie. no in uapi? Define it at include/net/netfilter/nf_conntrack.h and
add a comment regarding this to avoid an overlap in the future.
quoted
+ if (!test_bit(IPS_OFFLOAD_TEARDOWN_BIT, &tmp->status))
+ nf_ct_offload_timeout(tmp);
continue;
}
Very unlikely, but race might still happen between fixup and
clear IPS_OFFLOAD_BIT with gc below?
Without checking from the packet path, the conntrack gc might race to
refresh the timeout, I don't see a 100% race free solution.
Probably update the nf_ct_offload_timeout to a shorter value than a
day would mitigate this issue too.
This section of the code is now protected by IPS_OFFLOAD_TEARDOWN_BIT
which will prevent the update via nf_ct_offload_timeout.
We set it at the beginning of flow_offload_del and flow_offload_teardown.
Since flow_offload_teardown is only called on TCP packets
we also need to set it at flow_offload_del to prevent the race.
This should prevent the race at this point.
This chunk is not required, from ruleset users can do
... ct status assured ...
instead.
Maybe this should be mentioned in the manual or wiki if
it is not necessary in the flow offload code.
Yes, documentation and wiki can be updated.
Users might want to offload the flow at a later stage in the TCP
connection.
Well, actually there is not later stage than established, anything
after established are TCP teardown states.
What's the issue with allowing to offload from SYN_RECV state?
There were multiple problem in general with the code.
flow_offload_fixup_tcp always moves a TCP connection
to established even if it is in FIN or CLOSE.
The flowoffload_del function was always setting the TCP timeout
to ESTABLISHED timeout even when the state was in CLOSE and therefore
creating a very long lasting dead state.
Since we might miss or bump packets to slow path, we do not know
what will happen there when we are still in SYN_RECV.
Right.
We will have a better knowledge of the TCP state when we are in
established first and we know that we are either still in it or
we have moved past it to a closing state.
It makes sense to restrict this to TCP established only.
Oz and Paul already do this for tc-ct.
Thanks for explaining.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
I'm attaching an incomplete sketch patch. My goal is to avoid the
extra IPS_ bit.
You might create a race with ct gc that will remove the ct
if it is in close or end of close and before flow offload teardown is running
so flow offload teardown might access memory that was freed.
flow object holds a reference to the ct object until it is released,
no use-after-free can happen.
Also if nf_ct_delete is called before flowtable delete?
Can you let me know why?
nf_ct_delete() removes the conntrack object from lists and it
decrements the reference counter by one.
flow_offload_free() also calls nf_ct_put(). flow_offload_alloc() bumps
the reference count on the conntrack object before creating the flow.
quoted
quoted
quoted
It is not a very likely scenario but never the less it might happen now
since the IPS_OFFLOAD_BIT is not set and the state might just time out.
If someone sets a very small TCP CLOSE timeout it gets more likely.
So Oz and myself were debatting about three possible cases/problems:
1. ct gc sets timeout even though the state is in CLOSE/FIN because the
IPS_OFFLOAD is still set but the flow is in teardown
2. ct gc removes the ct because the IPS_OFFLOAD is not set and
the CLOSE timeout is reached before the flow offload del
OK.
quoted
3. tcp ct is always set to ESTABLISHED with a very long timeout
in flow offload teardown/delete even though the state is already
CLOSED.
Also as a remark we can not assume that the FIN or RST packet is hitting
flow table teardown as the packet might get bumped to the slow path in
nftables.
I assume this remark is related to 3.?
Yes, exactly.
quoted
if IPS_OFFLOAD is unset, then conntrack would update the state
according to this FIN or RST.
It will move to a different TCP state anyways only the ct state
will be at IPS_OFFLOAD_BIT and prevent it from beeing garbage collected.
The timeout will be bumped back up as long as IPS_OFFLOAD_BIT is set
even though TCP might already be CLOSED.
I see what you are trying to do here, I have some remarks:
If teardown fixes the ct state and timeout to established, and IPS_OFFLOAD is
unset, then the packet is passed up in a consistent state.
I made a patch, it is based on yours, it's attached:
- If flow timeout expires or rst/fin is seen, ct state and timeout is
fixed up (to established state) and IPS_OFFLOAD is unset.
- If rst/fin packet is seen, ct state and timeout is fixed up (to
established state) and IPS_OFFLOAD is unset. The packet continues
its travel up to the classic path, so conntrack triggers the
transition from established to one of the close states.
For the case 1., IPS_OFFLOAD is not set anymore, so conntrack gc
cannot race to reset the ct timeout anymore.
For the case 2., if gc conntrack ever removes the ct entry, then the
IPS_DYING bit is set, which implicitly triggers the teardown state
from the flowtable gc. The flowtable still holds a reference to the
ct object, so no UAF can happen.
For the case 3. the conntrack is set to ESTABLISHED with a long
timeout, yes. This is to deal with the two possible cases:
a) flowtable timeout expired, so conntrack recovers control on the
flow.
b) tcp rst/fin will take back the packet to slow path. The ct has been
fixed up to established state so it will trasition to one of the
close states.
Am I missing anything?
You should not fixup the tcp state back to established.
If flow_offload_teardown is not called because a packet got bumped up to the slow path
and you call flow_offload_teardown from nf_flow_offload_gc_step, the tcp state might already
be in CLOSE state and you just moved it back to established.
The entire function flow_offload_fixup_tcp can go away if we only allow established tcp states
in the flowtable.
Same goes for the timeout. The timeout should really be set to the current tcp state
ct->proto.tcp->state which might not be established anymore.
For me the question remains, why can the ct gc not remove the ct when nf_ct_delete
is called before flow_offload_del is called?
Also you probably want to move the IPS_OFFLOAD_BIT to the beginning of
flow_offload_teardown just to make sure that the ct gc is not bumping up the ct timeout
while it is changed in flow_offload_fixup_ct.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
I'm attaching an incomplete sketch patch. My goal is to avoid the
extra IPS_ bit.
You might create a race with ct gc that will remove the ct
if it is in close or end of close and before flow offload teardown is running
so flow offload teardown might access memory that was freed.
flow object holds a reference to the ct object until it is released,
no use-after-free can happen.
Also if nf_ct_delete is called before flowtable delete?
Can you let me know why?
nf_ct_delete() removes the conntrack object from lists and it
decrements the reference counter by one.
flow_offload_free() also calls nf_ct_put(). flow_offload_alloc() bumps
the reference count on the conntrack object before creating the flow.
quoted
quoted
quoted
It is not a very likely scenario but never the less it might happen now
since the IPS_OFFLOAD_BIT is not set and the state might just time out.
If someone sets a very small TCP CLOSE timeout it gets more likely.
So Oz and myself were debatting about three possible cases/problems:
1. ct gc sets timeout even though the state is in CLOSE/FIN because the
IPS_OFFLOAD is still set but the flow is in teardown
2. ct gc removes the ct because the IPS_OFFLOAD is not set and
the CLOSE timeout is reached before the flow offload del
OK.
quoted
3. tcp ct is always set to ESTABLISHED with a very long timeout
in flow offload teardown/delete even though the state is already
CLOSED.
Also as a remark we can not assume that the FIN or RST packet is hitting
flow table teardown as the packet might get bumped to the slow path in
nftables.
I assume this remark is related to 3.?
Yes, exactly.
quoted
if IPS_OFFLOAD is unset, then conntrack would update the state
according to this FIN or RST.
It will move to a different TCP state anyways only the ct state
will be at IPS_OFFLOAD_BIT and prevent it from beeing garbage collected.
The timeout will be bumped back up as long as IPS_OFFLOAD_BIT is set
even though TCP might already be CLOSED.
I see what you are trying to do here, I have some remarks:
quoted
If teardown fixes the ct state and timeout to established, and IPS_OFFLOAD is
unset, then the packet is passed up in a consistent state.
I made a patch, it is based on yours, it's attached:
- If flow timeout expires or rst/fin is seen, ct state and timeout is
fixed up (to established state) and IPS_OFFLOAD is unset.
- If rst/fin packet is seen, ct state and timeout is fixed up (to
established state) and IPS_OFFLOAD is unset. The packet continues
its travel up to the classic path, so conntrack triggers the
transition from established to one of the close states.
For the case 1., IPS_OFFLOAD is not set anymore, so conntrack gc
cannot race to reset the ct timeout anymore.
For the case 2., if gc conntrack ever removes the ct entry, then the
IPS_DYING bit is set, which implicitly triggers the teardown state
from the flowtable gc. The flowtable still holds a reference to the
ct object, so no UAF can happen.
For the case 3. the conntrack is set to ESTABLISHED with a long
timeout, yes. This is to deal with the two possible cases:
a) flowtable timeout expired, so conntrack recovers control on the
flow.
b) tcp rst/fin will take back the packet to slow path. The ct has been
fixed up to established state so it will trasition to one of the
close states.
Am I missing anything?
You should not fixup the tcp state back to established.
If flow_offload_teardown is not called because a packet got bumped up to the slow path
and you call flow_offload_teardown from nf_flow_offload_gc_step, the tcp state might already
be in CLOSE state and you just moved it back to established.
OK.
The entire function flow_offload_fixup_tcp can go away if we only allow established tcp states
in the flowtable.
I'm keeping it, but I remove the reset of the tcp state.
Same goes for the timeout. The timeout should really be set to the current tcp state
ct->proto.tcp->state which might not be established anymore.
OK.
For me the question remains, why can the ct gc not remove the ct when nf_ct_delete
is called before flow_offload_del is called?
nf_ct_delete() removes indeed the entry from the conntrack table, then
it calls nf_ct_put() which decrements the refcnt. Given that the
flowtable holds a reference to the conntrack object...
struct flow_offload *flow_offload_alloc(struct nf_conn *ct)
{
struct flow_offload *flow;
if (unlikely(nf_ct_is_dying(ct) ||
!refcount_inc_not_zero(&ct->ct_general.use)))
return NULL;
... use-after-free cannot happen. Note that flow_offload_free() calls
nf_ct_put(flow->ct), so at this point the ct object is released.
Is this your concern?
Also you probably want to move the IPS_OFFLOAD_BIT to the beginning of
flow_offload_teardown just to make sure that the ct gc is not bumping up the ct timeout
while it is changed in flow_offload_fixup_ct.
Hm, it is the trick to avoid checking for IPS_OFFLOAD from the packet
path that triggers the race, ie. nf_ct_is_expired()
The flowtable ct fixup races with conntrack gc collector.
Clearing IPS_OFFLOAD might result in offloading the entry again for
the closing packets.
Probably clear IPS_OFFLOAD from teardown, and skip offload if flow is
in a TCP state that represent closure?
if (unlikely(!tcph || tcph->fin || tcph->rst))
goto out;
this is already the intention in the existing code.
I'm attaching an incomplete sketch patch. My goal is to avoid the
extra IPS_ bit.
You might create a race with ct gc that will remove the ct
if it is in close or end of close and before flow offload teardown is running
so flow offload teardown might access memory that was freed.
flow object holds a reference to the ct object until it is released,
no use-after-free can happen.
Also if nf_ct_delete is called before flowtable delete?
Can you let me know why?
nf_ct_delete() removes the conntrack object from lists and it
decrements the reference counter by one.
flow_offload_free() also calls nf_ct_put(). flow_offload_alloc() bumps
the reference count on the conntrack object before creating the flow.
quoted
quoted
quoted
It is not a very likely scenario but never the less it might happen now
since the IPS_OFFLOAD_BIT is not set and the state might just time out.
If someone sets a very small TCP CLOSE timeout it gets more likely.
So Oz and myself were debatting about three possible cases/problems:
1. ct gc sets timeout even though the state is in CLOSE/FIN because the
IPS_OFFLOAD is still set but the flow is in teardown
2. ct gc removes the ct because the IPS_OFFLOAD is not set and
the CLOSE timeout is reached before the flow offload del
OK.
quoted
3. tcp ct is always set to ESTABLISHED with a very long timeout
in flow offload teardown/delete even though the state is already
CLOSED.
Also as a remark we can not assume that the FIN or RST packet is hitting
flow table teardown as the packet might get bumped to the slow path in
nftables.
I assume this remark is related to 3.?
Yes, exactly.
quoted
if IPS_OFFLOAD is unset, then conntrack would update the state
according to this FIN or RST.
It will move to a different TCP state anyways only the ct state
will be at IPS_OFFLOAD_BIT and prevent it from beeing garbage collected.
The timeout will be bumped back up as long as IPS_OFFLOAD_BIT is set
even though TCP might already be CLOSED.
I see what you are trying to do here, I have some remarks:
quoted
If teardown fixes the ct state and timeout to established, and IPS_OFFLOAD is
unset, then the packet is passed up in a consistent state.
I made a patch, it is based on yours, it's attached:
- If flow timeout expires or rst/fin is seen, ct state and timeout is
fixed up (to established state) and IPS_OFFLOAD is unset.
- If rst/fin packet is seen, ct state and timeout is fixed up (to
established state) and IPS_OFFLOAD is unset. The packet continues
its travel up to the classic path, so conntrack triggers the
transition from established to one of the close states.
For the case 1., IPS_OFFLOAD is not set anymore, so conntrack gc
cannot race to reset the ct timeout anymore.
For the case 2., if gc conntrack ever removes the ct entry, then the
IPS_DYING bit is set, which implicitly triggers the teardown state
from the flowtable gc. The flowtable still holds a reference to the
ct object, so no UAF can happen.
For the case 3. the conntrack is set to ESTABLISHED with a long
timeout, yes. This is to deal with the two possible cases:
a) flowtable timeout expired, so conntrack recovers control on the
flow.
b) tcp rst/fin will take back the packet to slow path. The ct has been
fixed up to established state so it will trasition to one of the
close states.
Am I missing anything?
You should not fixup the tcp state back to established.
If flow_offload_teardown is not called because a packet got bumped up to the slow path
and you call flow_offload_teardown from nf_flow_offload_gc_step, the tcp state might already
be in CLOSE state and you just moved it back to established.
OK.
quoted
The entire function flow_offload_fixup_tcp can go away if we only allow established tcp states
in the flowtable.
I'm keeping it, but I remove the reset of the tcp state.
quoted
Same goes for the timeout. The timeout should really be set to the current tcp state
ct->proto.tcp->state which might not be established anymore.
OK.
quoted
For me the question remains, why can the ct gc not remove the ct when nf_ct_delete
is called before flow_offload_del is called?
nf_ct_delete() removes indeed the entry from the conntrack table, then
it calls nf_ct_put() which decrements the refcnt. Given that the
flowtable holds a reference to the conntrack object...
struct flow_offload *flow_offload_alloc(struct nf_conn *ct)
{
struct flow_offload *flow;
if (unlikely(nf_ct_is_dying(ct) ||
!refcount_inc_not_zero(&ct->ct_general.use)))
return NULL;
... use-after-free cannot happen. Note that flow_offload_free() calls
nf_ct_put(flow->ct), so at this point the ct object is released.
Is this your concern?
Ah yes, thank you.
I did not catch the refcount_inc_not_zero call.
quoted
Also you probably want to move the IPS_OFFLOAD_BIT to the beginning of
flow_offload_teardown just to make sure that the ct gc is not bumping up the ct timeout
while it is changed in flow_offload_fixup_ct.
Done.
See patch attached.
quoted
The patch looks good to me, one remark.
This has to be
- if (unlikely(!tcph || tcph->fin || tcph->rst))
+ if (unlikely(!tcph || tcph->fin || tcph->rst ||
+ !nf_conntrack_tcp_established(&ct->proto.tcp)))
goto out;
You are currently go to out if the tcp state is established but you
want the opposite, not established.
I think this will cover all cases.
Best
Sven