netdev - Re: [PATCH nf-next RFC,v2 1/6] netfilter: nf_conntrack: add IPS

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: <20171208210009.GA4126@salvia>
Date:   Fri, 8 Dec 2017 22:00:09 +0100
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Florian Westphal <fw@...len.de>
Cc:     netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
        f.fainelli@...il.com, simon.horman@...ronome.com,
        ronye@...lanox.com, jiri@...lanox.com, nbd@....name,
        john@...ozen.org, kubakici@...pl
Subject: Re: [PATCH nf-next RFC,v2 1/6] netfilter: nf_conntrack: add
 IPS_OFFLOAD status bit

On Fri, Dec 08, 2017 at 07:47:02AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@...filter.org> wrote:
> > diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h b/include/uapi/linux/netfilter/nf_conntrack_common.h
> > index dc947e59d03a..6b463b88182d 100644
> > --- a/include/uapi/linux/netfilter/nf_conntrack_common.h
> > +++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
> > @@ -100,6 +100,10 @@ enum ip_conntrack_status {
> >  	IPS_HELPER_BIT = 13,
> >  	IPS_HELPER = (1 << IPS_HELPER_BIT),
> >  
> > +	/* Conntrack has been offloaded to flow table. */
> > +	IPS_OFFLOAD_BIT = 14,
> > +	IPS_OFFLOAD = (1 << IPS_OFFLOAD_BIT),
> > +
> >  	/* Be careful here, modifying these bits can make things messy,
> >  	 * so don't let users modify them directly.
> >  	 */
> 
> I think this new bit has to be added to the UNCHANGEABLE mask below.

Right.

> > diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> > index 01130392b7c0..02e195accd47 100644
> > --- a/net/netfilter/nf_conntrack_core.c
> > +++ b/net/netfilter/nf_conntrack_core.c
> > @@ -901,6 +901,9 @@ static unsigned int early_drop_list(struct net *net,
> >  	hlist_nulls_for_each_entry_rcu(h, n, head, hnnode) {
> >  		tmp = nf_ct_tuplehash_to_ctrack(h);
> >  
> > +		if (test_bit(IPS_OFFLOAD_BIT, &tmp->status))
> > +			continue;
> > +
> 
> nit: I would move this below the ASSURED bit check, AFAIU most
> (all?) offloaded conntracks are not in ASSURED state since they never
> see two-way communication but in case we've mixed flows or no offloading
> in place then the ASSURED check takes care of skipping earlydrop
> already.

Offload happens once we enter established state (or later if your rule
postpone it to a later stage), that is before we observe the full
3-way handshake in tcp, hence the assured bit. So I think we still
need this here.

> > +/* Set an arbitrary timeout large enough not to ever expire, this save
> > + * us a check for the IPS_OFFLOAD_BIT from the packet path via
> > + * nf_ct_is_expired().
> > + */
> > +static void nf_ct_offload_timeout(struct nf_conn *ct)
> > +{
> > +       ct->timeout = nfct_time_stamp + DAY;
> > +}
> 
> Not sure if its worth adding a test to avoid unconditional write,
> e.g. something like
> 
> > +static void nf_ct_offload_timeout(struct nf_conn *ct)
> > +{
> 	if (nf_ct_expires(ct) < DAY/2))
> > +       ct->timeout = nfct_time_stamp + DAY;
> 
> but perhaps not worth it, gc_worker is infrequent.

That's fine, I'll do this.