[PATCH nf-next] netfilter: ctnetlink: always include remaining timeout

[PATCH nf-next] netfilter: ctnetlink: always include remaining timeout

[Florian Westphal]

DESTROY events do not include the remaining timeout.

Unconditionally including the timeout allows to see if the entry timed
timed out or was removed explicitly.

The latter case can happen when a conntrack gets deleted prematurely,
e.g. due to a tcp reset, module removal, netdev notifier (nat/masquerade
device went down), ctnetlink and so on.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Might make sense to further extend nf_ct_delete and also pass a
 reason code in the future.

 net/netfilter/nf_conntrack_netlink.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 3d0fd33be018..3f957769cd72 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -778,15 +778,14 @@ ctnetlink_conntrack_event(unsigned int events, struct nf_ct_event *item)
 
 	if (ctnetlink_dump_status(skb, ct) < 0)
 		goto nla_put_failure;
+	if (ctnetlink_dump_timeout(skb, ct) < 0)
+		goto nla_put_failure;
 
 	if (events & (1 << IPCT_DESTROY)) {
 		if (ctnetlink_dump_acct(skb, ct, type) < 0 ||
 		    ctnetlink_dump_timestamp(skb, ct) < 0)
 			goto nla_put_failure;
 	} else {
-		if (ctnetlink_dump_timeout(skb, ct) < 0)
-			goto nla_put_failure;
-
 		if (events & (1 << IPCT_PROTOINFO)
 		    && ctnetlink_dump_protoinfo(skb, ct) < 0)
 			goto nla_put_failure;
-- 
2.26.2

Re: [PATCH nf-next] netfilter: ctnetlink: always include remaining timeout

[Pablo Neira Ayuso]

On Thu, Dec 10, 2020 at 12:20:22PM +0100, Florian Westphal wrote:
> DESTROY events do not include the remaining timeout.
> 
> Unconditionally including the timeout allows to see if the entry timed
> timed out or was removed explicitly.
> 
> The latter case can happen when a conntrack gets deleted prematurely,
> e.g. due to a tcp reset, module removal, netdev notifier (nat/masquerade
> device went down), ctnetlink and so on.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  Might make sense to further extend nf_ct_delete and also pass a
>  reason code in the future.

IIRC, TCP state is not included in the event, right?

This has been requested many times in the past, to debug connectivity
issues too.

Probably extending .to_nlattr to take a bool parameter to specify if
this is the destroy event path, then _only_ include the TCP state
information there (other TCP information is not relevant and netlink
bandwidth is limited from the event path).

Thanks.

Re: [PATCH nf-next] netfilter: ctnetlink: always include remaining timeout

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Dec 10, 2020 at 12:20:22PM +0100, Florian Westphal wrote:
> > DESTROY events do not include the remaining timeout.
> > 
> > Unconditionally including the timeout allows to see if the entry timed
> > timed out or was removed explicitly.
> > 
> > The latter case can happen when a conntrack gets deleted prematurely,
> > e.g. due to a tcp reset, module removal, netdev notifier (nat/masquerade
> > device went down), ctnetlink and so on.
> >
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  Might make sense to further extend nf_ct_delete and also pass a
> >  reason code in the future.
> 
> IIRC, TCP state is not included in the event, right?

No, protoinfo is only dumped for non-destroy case.

> This has been requested many times in the past, to debug connectivity
> issues too.
> 
> Probably extending .to_nlattr to take a bool parameter to specify if
> this is the destroy event path, then _only_ include the TCP state
> information there (other TCP information is not relevant and netlink
> bandwidth is limited from the event path).

Sounds reasonable, will send a v2.