[PATCH nf-next] netfilter: ctnetlink: make more information available in DESTROY events

[PATCH nf-next] netfilter: ctnetlink: make more information available in DESTROY events

[Florian Westphal]

destroy events currently don't contain the tcp state info and no
secmark and conntrack labels.

Quoting Victor:
 "I was hoping to get the last TCP state in a conntrack destroy event,
  however it seems to be unavailable."

Quoting Jarno:
 "I have a use case where we want to log terminating connections, but
 only if a specific label bit is set."

While at it, also include SECMARK in destroy events if one is available.

Cc: Jarno Rajahalme <jarno@ovn.org>
Cc: Victor Julien <lists@inliniac.net>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_conntrack_netlink.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 27540455dc62..b984c1b5b3ec 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -347,7 +347,7 @@ static inline int ctnetlink_label_size(const struct nf_conn *ct)
 }
 
 static int
-ctnetlink_dump_labels(struct sk_buff *skb, const struct nf_conn *ct)
+ctnetlink_dump_labels(struct sk_buff *skb, const struct nf_conn *ct, bool force)
 {
 	struct nf_conn_labels *labels = nf_ct_labels_find(ct);
 	unsigned int i;
@@ -357,7 +357,7 @@ ctnetlink_dump_labels(struct sk_buff *skb, const struct nf_conn *ct)
 
 	i = 0;
 	do {
-		if (labels->bits[i] != 0)
+		if (labels->bits[i] != 0 || force)
 			return nla_put(skb, CTA_LABELS, sizeof(labels->bits),
 				       labels->bits);
 		i++;
@@ -366,7 +366,7 @@ ctnetlink_dump_labels(struct sk_buff *skb, const struct nf_conn *ct)
 	return 0;
 }
 #else
-#define ctnetlink_dump_labels(a, b) (0)
+#define ctnetlink_dump_labels(a, b, c) (0)
 #define ctnetlink_label_size(a)	(0)
 #endif
 
@@ -511,7 +511,7 @@ ctnetlink_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
 	    ctnetlink_dump_helpinfo(skb, ct) < 0 ||
 	    ctnetlink_dump_mark(skb, ct) < 0 ||
 	    ctnetlink_dump_secctx(skb, ct) < 0 ||
-	    ctnetlink_dump_labels(skb, ct) < 0 ||
+	    ctnetlink_dump_labels(skb, ct, false) < 0 ||
 	    ctnetlink_dump_id(skb, ct) < 0 ||
 	    ctnetlink_dump_use(skb, ct) < 0 ||
 	    ctnetlink_dump_master(skb, ct) < 0 ||
@@ -697,7 +697,8 @@ ctnetlink_conntrack_event(unsigned int events, struct nf_ct_event *item)
 
 	if (events & (1 << IPCT_DESTROY)) {
 		if (ctnetlink_dump_acct(skb, ct, type) < 0 ||
-		    ctnetlink_dump_timestamp(skb, ct) < 0)
+		    ctnetlink_dump_timestamp(skb, ct) < 0 ||
+		    ctnetlink_dump_protoinfo(skb, ct) < 0)
 			goto nla_put_failure;
 	} else {
 		if (ctnetlink_dump_timeout(skb, ct) < 0)
@@ -711,15 +712,6 @@ ctnetlink_conntrack_event(unsigned int events, struct nf_ct_event *item)
 		    && ctnetlink_dump_helpinfo(skb, ct) < 0)
 			goto nla_put_failure;
 
-#ifdef CONFIG_NF_CONNTRACK_SECMARK
-		if ((events & (1 << IPCT_SECMARK) || ct->secmark)
-		    && ctnetlink_dump_secctx(skb, ct) < 0)
-			goto nla_put_failure;
-#endif
-		if (events & (1 << IPCT_LABEL) &&
-		     ctnetlink_dump_labels(skb, ct) < 0)
-			goto nla_put_failure;
-
 		if (events & (1 << IPCT_RELATED) &&
 		    ctnetlink_dump_master(skb, ct) < 0)
 			goto nla_put_failure;
@@ -734,6 +726,14 @@ ctnetlink_conntrack_event(unsigned int events, struct nf_ct_event *item)
 	    && ctnetlink_dump_mark(skb, ct) < 0)
 		goto nla_put_failure;
 #endif
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+	if ((events & (1 << IPCT_SECMARK) || ct->secmark) &&
+	    ctnetlink_dump_secctx(skb, ct) < 0)
+		goto nla_put_failure;
+#endif
+	if (ctnetlink_dump_labels(skb, ct, events & (1 << IPCT_LABEL)) < 0)
+		goto nla_put_failure;
+
 	rcu_read_unlock();
 
 	nlmsg_end(skb, nlh);
@@ -2234,7 +2234,7 @@ static int __ctnetlink_glue_build(struct sk_buff *skb, struct nf_conn *ct)
 	if (ct->mark && ctnetlink_dump_mark(skb, ct) < 0)
 		goto nla_put_failure;
 #endif
-	if (ctnetlink_dump_labels(skb, ct) < 0)
+	if (ctnetlink_dump_labels(skb, ct, false) < 0)
 		goto nla_put_failure;
 	rcu_read_unlock();
 	return 0;
-- 
2.7.3

Re: [PATCH nf-next] netfilter: ctnetlink: make more information available in DESTROY events

[Pablo Neira Ayuso]

On Wed, Jan 18, 2017 at 03:54:32PM +0100, Florian Westphal wrote:
> destroy events currently don't contain the tcp state info and no
> secmark and conntrack labels.
> 
> Quoting Victor:
>  "I was hoping to get the last TCP state in a conntrack destroy event,
>   however it seems to be unavailable."
> 
> Quoting Jarno:
>  "I have a use case where we want to log terminating connections, but
>  only if a specific label bit is set."
> 
> While at it, also include SECMARK in destroy events if one is available.

I'm fine with this.

But to remember the original problem is that netlink bandwidth is
limited, so the more we load the netlink message, the more chances we
have to hit ENOBUFS.

connlabel is optional, so you only get it if needed.

But the protoinfo thing, I would prefer we just dump the state given
this the usecase we have now.

Probably extend ->to_nlattr() to have a bool that indicates if this is
a dump?

Re: [PATCH nf-next] netfilter: ctnetlink: make more information available in DESTROY events

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Jan 18, 2017 at 03:54:32PM +0100, Florian Westphal wrote:
> > destroy events currently don't contain the tcp state info and no
> > secmark and conntrack labels.
> > 
> > Quoting Victor:
> >  "I was hoping to get the last TCP state in a conntrack destroy event,
> >   however it seems to be unavailable."
> > 
> > Quoting Jarno:
> >  "I have a use case where we want to log terminating connections, but
> >  only if a specific label bit is set."
> > 
> > While at it, also include SECMARK in destroy events if one is available.
> 
> I'm fine with this.
> 
> But to remember the original problem is that netlink bandwidth is
> limited, so the more we load the netlink message, the more chances we
> have to hit ENOBUFS.
> 
> connlabel is optional, so you only get it if needed.

Yes, and only if there was a label change or at least one label bit is
set.

> But the protoinfo thing, I would prefer we just dump the state given
> this the usecase we have now.
> 
> Probably extend ->to_nlattr() to have a bool that indicates if this is
> a dump?

Could do that by it only avoids 4 attributes (so we only save 32bytes
per message).

If you still think its worth it I'll resend a v2 without the dump change
and will send do a followup change that flags the requested event dump
to the protocol backend.

Re: [PATCH nf-next] netfilter: ctnetlink: make more information available in DESTROY events

[Pablo Neira Ayuso]

On Wed, Jan 18, 2017 at 04:07:45PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Jan 18, 2017 at 03:54:32PM +0100, Florian Westphal wrote:
> > > destroy events currently don't contain the tcp state info and no
> > > secmark and conntrack labels.
> > > 
> > > Quoting Victor:
> > >  "I was hoping to get the last TCP state in a conntrack destroy event,
> > >   however it seems to be unavailable."
> > > 
> > > Quoting Jarno:
> > >  "I have a use case where we want to log terminating connections, but
> > >  only if a specific label bit is set."
> > > 
> > > While at it, also include SECMARK in destroy events if one is available.
> > 
> > I'm fine with this.
> > 
> > But to remember the original problem is that netlink bandwidth is
> > limited, so the more we load the netlink message, the more chances we
> > have to hit ENOBUFS.
> > 
> > connlabel is optional, so you only get it if needed.
> 
> Yes, and only if there was a label change or at least one label bit is
> set.
> 
> > But the protoinfo thing, I would prefer we just dump the state given
> > this the usecase we have now.
> > 
> > Probably extend ->to_nlattr() to have a bool that indicates if this is
> > a dump?
> 
> Could do that by it only avoids 4 attributes (so we only save 32bytes
> per message).

IIRC, the destroy message is rather small. Moreover, think of
thousands of messages in that queue, that makes a difference. And I
cannot think of anything useful people can do with window scale and
flags at that stage, right?

Re: [PATCH nf-next] netfilter: ctnetlink: make more information available in DESTROY events

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> IIRC, the destroy message is rather small. Moreover, think of
> thousands of messages in that queue, that makes a difference. And I
> cannot think of anything useful people can do with window scale and
> flags at that stage, right?

I could not think of a reason why any of the l4 tracker attributes are
useful in destroy so I can't answer your question :)

Re: [PATCH nf-next] netfilter: ctnetlink: make more information available in DESTROY events

[Victor Julien]

On 18-01-17 17:03, Pablo Neira Ayuso wrote:
> On Wed, Jan 18, 2017 at 04:07:45PM +0100, Florian Westphal wrote:
>> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>>> On Wed, Jan 18, 2017 at 03:54:32PM +0100, Florian Westphal wrote:
>>>> destroy events currently don't contain the tcp state info and no
>>>> secmark and conntrack labels.
>>>>
>>>> Quoting Victor:
>>>>  "I was hoping to get the last TCP state in a conntrack destroy event,
>>>>   however it seems to be unavailable."
>>>>
>>>> Quoting Jarno:
>>>>  "I have a use case where we want to log terminating connections, but
>>>>  only if a specific label bit is set."
>>>>
>>>> While at it, also include SECMARK in destroy events if one is available.
>>>
>>> I'm fine with this.
>>>
>>> But to remember the original problem is that netlink bandwidth is
>>> limited, so the more we load the netlink message, the more chances we
>>> have to hit ENOBUFS.
>>>
>>> connlabel is optional, so you only get it if needed.
>>
>> Yes, and only if there was a label change or at least one label bit is
>> set.
>>
>>> But the protoinfo thing, I would prefer we just dump the state given
>>> this the usecase we have now.
>>>
>>> Probably extend ->to_nlattr() to have a bool that indicates if this is
>>> a dump?
>>
>> Could do that by it only avoids 4 attributes (so we only save 32bytes
>> per message).
> 
> IIRC, the destroy message is rather small. Moreover, think of
> thousands of messages in that queue, that makes a difference. And I
> cannot think of anything useful people can do with window scale and
> flags at that stage, right?

If with flags you mean tracking the tcp flags seen (SYN, RST, etc), then
I think that could be useful. Netflow logging often logs that. I was
planning to use that when/if it's available.

-- 
---------------------------------------------
Victor Julien
http://www.inliniac.net/
PGP: http://www.inliniac.net/victorjulien.asc
---------------------------------------------