netdev - Re: [PATCH netfilter] netfilter: conntrack: udp: generate event on switch to stream timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YWlKGFpHa5o5jFgJ@salvia>
Date:   Fri, 15 Oct 2021 11:30:00 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Maciej Żenczykowski <zenczykowski@...il.com>
Cc:     Maciej Żenczykowski <maze@...gle.com>,
        Florian Westphal <fw@...len.de>,
        Linux Network Development Mailing List 
        <netdev@...r.kernel.org>,
        Netfilter Development Mailing List 
        <netfilter-devel@...r.kernel.org>
Subject: Re: [PATCH netfilter] netfilter: conntrack: udp: generate event on
 switch to stream timeout

On Fri, Oct 15, 2021 at 02:09:34AM -0700, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@...gle.com>
> 
> Without this it's hard to offload udp due to lack of a conntrack event
> at the appropriate time (ie. once udp stream is established and stream
> timeout is in effect).
> 
> Without this udp conntrack events 'update/assured/timeout=30'
> either need to be ignored, or polling loop needs to be <30 second
> instead of <120 second.
> 
> With this change:
>       [NEW] udp      17 30 src=10.246.11.13 dst=216.239.35.0 sport=37282 dport=123 [UNREPLIED] src=216.239.35.0 dst=10.246.11.13 sport=123 dport=37282
>    [UPDATE] udp      17 30 src=10.246.11.13 dst=216.239.35.0 sport=37282 dport=123 src=216.239.35.0 dst=10.246.11.13 sport=123 dport=37282
>    [UPDATE] udp      17 30 src=10.246.11.13 dst=216.239.35.0 sport=37282 dport=123 src=216.239.35.0 dst=10.246.11.13 sport=123 dport=37282 [ASSURED]
>    [UPDATE] udp      17 120 src=10.246.11.13 dst=216.239.35.0 sport=37282 dport=123 src=216.239.35.0 dst=10.246.11.13 sport=123 dport=37282 [ASSURED]
>   [DESTROY] udp      17 src=10.246.11.13 dst=216.239.35.0 sport=37282 dport=123 src=216.239.35.0 dst=10.246.11.13 sport=123 dport=37282 [ASSURED]
> (the 3rd update/assured/120 event is new)

Hm, I still don't understand why do you need this extra 3rd
update/assured event event. Could you explain your usecase?

Thanks.

> Cc: Florian Westphal <fw@...len.de>
> Cc: Pablo Neira Ayuso <pablo@...filter.org>
> Fixes: d535c8a69c19 'netfilter: conntrack: udp: only extend timeout to stream mode after 2s'
> Signed-off-by: Maciej Żenczykowski <maze@...gle.com>
> ---
>  include/net/netfilter/nf_conntrack.h             |  1 +
>  .../uapi/linux/netfilter/nf_conntrack_common.h   |  1 +
>  net/netfilter/nf_conntrack_proto_udp.c           | 16 ++++++++++++++--
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
> index cc663c68ddc4..12029d616cfa 100644
> --- a/include/net/netfilter/nf_conntrack.h
> +++ b/include/net/netfilter/nf_conntrack.h
> @@ -26,6 +26,7 @@
>  
>  struct nf_ct_udp {
>  	unsigned long	stream_ts;
> +	bool		notified;
>  };
>  
>  /* per conntrack: protocol private data */
> diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h b/include/uapi/linux/netfilter/nf_conntrack_common.h
> index 4b3395082d15..a8e91b5821fa 100644
> --- a/include/uapi/linux/netfilter/nf_conntrack_common.h
> +++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
> @@ -144,6 +144,7 @@ enum ip_conntrack_events {
>  	IPCT_SECMARK,		/* new security mark has been set */
>  	IPCT_LABEL,		/* new connlabel has been set */
>  	IPCT_SYNPROXY,		/* synproxy has been set */
> +	IPCT_UDPSTREAM,		/* udp stream has been set */
>  #ifdef __KERNEL__
>  	__IPCT_MAX
>  #endif
> diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
> index 68911fcaa0f1..f0d9869aa30f 100644
> --- a/net/netfilter/nf_conntrack_proto_udp.c
> +++ b/net/netfilter/nf_conntrack_proto_udp.c
> @@ -97,18 +97,23 @@ int nf_conntrack_udp_packet(struct nf_conn *ct,
>  	if (!timeouts)
>  		timeouts = udp_get_timeouts(nf_ct_net(ct));
>  
> -	if (!nf_ct_is_confirmed(ct))
> +	if (!nf_ct_is_confirmed(ct)) {
>  		ct->proto.udp.stream_ts = 2 * HZ + jiffies;
> +		ct->proto.udp.notified = false;
> +	}
>  
>  	/* If we've seen traffic both ways, this is some kind of UDP
>  	 * stream. Set Assured.
>  	 */
>  	if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) {
>  		unsigned long extra = timeouts[UDP_CT_UNREPLIED];
> +		bool stream = false;
>  
>  		/* Still active after two seconds? Extend timeout. */
> -		if (time_after(jiffies, ct->proto.udp.stream_ts))
> +		if (time_after(jiffies, ct->proto.udp.stream_ts)) {
>  			extra = timeouts[UDP_CT_REPLIED];
> +			stream = true;
> +		}
>  
>  		nf_ct_refresh_acct(ct, ctinfo, skb, extra);
>  
> @@ -116,9 +121,16 @@ int nf_conntrack_udp_packet(struct nf_conn *ct,
>  		if (unlikely((ct->status & IPS_NAT_CLASH)))
>  			return NF_ACCEPT;
>  
> +		if (stream) {
> +			stream = !ct->proto.udp.notified;
> +			ct->proto.udp.notified = true;
> +		}
> +
>  		/* Also, more likely to be important, and not a probe */
>  		if (!test_and_set_bit(IPS_ASSURED_BIT, &ct->status))
>  			nf_conntrack_event_cache(IPCT_ASSURED, ct);
> +		else if (stream)
> +			nf_conntrack_event_cache(IPCT_UDPSTREAM, ct);
>  	} else {
>  		nf_ct_refresh_acct(ct, ctinfo, skb, timeouts[UDP_CT_UNREPLIED]);
>  	}
> -- 
> 2.33.0.1079.g6e70778dc9-goog
>