[ovs-dev,RFC,2/5] netfilter: Factor out nf_ct_get_info().
diff mbox

Message ID 1445379629-112880-2-git-send-email-jrajahalme@nicira.com
State Not Applicable
Headers show

Commit Message

Jarno Rajahalme Oct. 20, 2015, 10:20 p.m. UTC
Define a new inline function to map conntrack status to enum
ip_conntrack_info.  This removes the need to otherwise duplicate this
code in a later patch.

Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
---
 include/net/netfilter/nf_conntrack.h | 15 +++++++++++++++
 net/netfilter/nf_conntrack_core.c    | 22 +++-------------------
 2 files changed, 18 insertions(+), 19 deletions(-)

Comments

Thomas Graf Oct. 21, 2015, 8:50 a.m. UTC | #1
On 10/20/15 at 03:20pm, Jarno Rajahalme wrote:
> Define a new inline function to map conntrack status to enum
> ip_conntrack_info.  This removes the need to otherwise duplicate this
> code in a later patch.
> 
> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>

LGTM

> ---
>  include/net/netfilter/nf_conntrack.h | 15 +++++++++++++++
>  net/netfilter/nf_conntrack_core.c    | 22 +++-------------------
>  2 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
> index fde4068..b3de10e 100644
> --- a/include/net/netfilter/nf_conntrack.h
> +++ b/include/net/netfilter/nf_conntrack.h
> @@ -125,6 +125,21 @@ nf_ct_tuplehash_to_ctrack(const struct nf_conntrack_tuple_hash *hash)
>  			    tuplehash[hash->tuple.dst.dir]);
>  }
>  
> +static inline enum ip_conntrack_info
> +nf_ct_get_info(const struct nf_conntrack_tuple_hash *h)
> +{
> +	const struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
> +
> +	if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
> +		return IP_CT_ESTABLISHED_REPLY;
> +	/* Once we've had two way comms, always ESTABLISHED. */
> +	if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status))
> +		return IP_CT_ESTABLISHED;
> +	if (test_bit(IPS_EXPECTED_BIT, &ct->status))
> +		return IP_CT_RELATED;
> +	return IP_CT_NEW;
> +}
> +
>  static inline u_int16_t nf_ct_l3num(const struct nf_conn *ct)
>  {
>  	return ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.l3num;
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 3cb3cb8..70ddbd8 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -1056,25 +1056,9 @@ resolve_normal_ct(struct net *net, struct nf_conn *tmpl,
>  	ct = nf_ct_tuplehash_to_ctrack(h);
>  
>  	/* It exists; we have (non-exclusive) reference. */
> -	if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY) {
> -		*ctinfo = IP_CT_ESTABLISHED_REPLY;
> -		/* Please set reply bit if this packet OK */
> -		*set_reply = 1;
> -	} else {
> -		/* Once we've had two way comms, always ESTABLISHED. */
> -		if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) {
> -			pr_debug("nf_conntrack_in: normal packet for %p\n", ct);
> -			*ctinfo = IP_CT_ESTABLISHED;
> -		} else if (test_bit(IPS_EXPECTED_BIT, &ct->status)) {
> -			pr_debug("nf_conntrack_in: related packet for %p\n",
> -				 ct);
> -			*ctinfo = IP_CT_RELATED;
> -		} else {
> -			pr_debug("nf_conntrack_in: new packet for %p\n", ct);
> -			*ctinfo = IP_CT_NEW;
> -		}
> -		*set_reply = 0;
> -	}
> +	*ctinfo = nf_ct_get_info(h);
> +	*set_reply = NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY;
> +
>  	skb->nfct = &ct->ct_general;
>  	skb->nfctinfo = *ctinfo;
>  	return ct;
Pablo Neira Ayuso Oct. 21, 2015, 10:45 a.m. UTC | #2
On Tue, Oct 20, 2015 at 03:20:26PM -0700, Jarno Rajahalme wrote:
> Define a new inline function to map conntrack status to enum
> ip_conntrack_info.  This removes the need to otherwise duplicate this
> code in a later patch.

Where is that later patch that justifies this update?

> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
> ---
>  include/net/netfilter/nf_conntrack.h | 15 +++++++++++++++
>  net/netfilter/nf_conntrack_core.c    | 22 +++-------------------
>  2 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
> index fde4068..b3de10e 100644
> --- a/include/net/netfilter/nf_conntrack.h
> +++ b/include/net/netfilter/nf_conntrack.h
> @@ -125,6 +125,21 @@ nf_ct_tuplehash_to_ctrack(const struct nf_conntrack_tuple_hash *hash)
>  			    tuplehash[hash->tuple.dst.dir]);
>  }
>  
> +static inline enum ip_conntrack_info
> +nf_ct_get_info(const struct nf_conntrack_tuple_hash *h)
> +{
> +	const struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
> +
> +	if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
> +		return IP_CT_ESTABLISHED_REPLY;
> +	/* Once we've had two way comms, always ESTABLISHED. */
> +	if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status))
> +		return IP_CT_ESTABLISHED;
> +	if (test_bit(IPS_EXPECTED_BIT, &ct->status))
> +		return IP_CT_RELATED;
> +	return IP_CT_NEW;
> +}

I would really like to see how you want to use this.
Jarno Rajahalme Oct. 21, 2015, 8:43 p.m. UTC | #3
> On Oct 21, 2015, at 3:45 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> On Tue, Oct 20, 2015 at 03:20:26PM -0700, Jarno Rajahalme wrote:
>> Define a new inline function to map conntrack status to enum
>> ip_conntrack_info.  This removes the need to otherwise duplicate this
>> code in a later patch.
> 
> Where is that later patch that justifies this update?
> 

It is in patch 5/5, ovs_ct_find_existing(). The intent it to locate an existing conntrack entry without modifying it, if such an entry exists. This is needed in a case where we first go through the conntrack (calling nf_conntrack_in), then recirculate (to match on the state bits), but do not find a matching flow entry in the flow table. In this case we issue an upcall and let the OVS userspace compute a new flow entry. When the packet gets back to kernel we have lost the skb (as we do not buffer it in the kernel after issuing the upcall). ovs_ct_find_existing() can then be used to populate the nfct and nfctinfo fields of the new skb, as if this skb had gone through the nf_conntrack_in() call. This allows us to pass the packet through NAT after such an upcall without passing the packet through nf_conntrack_in() twice.

ovs_ct_find_existing() is intended to be used only when we have evidence in the OVS flow key that the packet actually went through nf_conntrack_in() prior to the upcall. In the current version of the patches I do not have a test case for this, and I intend to separate out this change into a separate patch before the NAT patch for the next (non-RFC) version.

I could implement ovs_ct_find_existing() using exported conntrack APIs apart from this mapping from conntrack status to enum ip_conntrack_info. ovs_ct_find_existing() is in no way OVS specific, though, so it could be added to the conntrack proper as well, if so desired.

  Jarno

>> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
>> ---
>> include/net/netfilter/nf_conntrack.h | 15 +++++++++++++++
>> net/netfilter/nf_conntrack_core.c    | 22 +++-------------------
>> 2 files changed, 18 insertions(+), 19 deletions(-)
>> 
>> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
>> index fde4068..b3de10e 100644
>> --- a/include/net/netfilter/nf_conntrack.h
>> +++ b/include/net/netfilter/nf_conntrack.h
>> @@ -125,6 +125,21 @@ nf_ct_tuplehash_to_ctrack(const struct nf_conntrack_tuple_hash *hash)
>> 			    tuplehash[hash->tuple.dst.dir]);
>> }
>> 
>> +static inline enum ip_conntrack_info
>> +nf_ct_get_info(const struct nf_conntrack_tuple_hash *h)
>> +{
>> +	const struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
>> +
>> +	if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
>> +		return IP_CT_ESTABLISHED_REPLY;
>> +	/* Once we've had two way comms, always ESTABLISHED. */
>> +	if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status))
>> +		return IP_CT_ESTABLISHED;
>> +	if (test_bit(IPS_EXPECTED_BIT, &ct->status))
>> +		return IP_CT_RELATED;
>> +	return IP_CT_NEW;
>> +}
> 
> I would really like to see how you want to use this.
Jarno Rajahalme Oct. 21, 2015, 9:38 p.m. UTC | #4
> On Oct 21, 2015, at 3:45 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> On Tue, Oct 20, 2015 at 03:20:26PM -0700, Jarno Rajahalme wrote:
>> Define a new inline function to map conntrack status to enum
>> ip_conntrack_info.  This removes the need to otherwise duplicate this
>> code in a later patch.
> 
> Where is that later patch that justifies this update?
> 

It is in patch 5/5, ovs_ct_find_existing(). The intent it to locate an existing conntrack entry without modifying it, if such an entry exists. This is needed in a case where we first go through the conntrack (calling nf_conntrack_in), then recirculate (to match on the state bits), but do not find a matching flow entry in the flow table. In this case we issue an upcall and let the OVS userspace compute a new flow entry. When the packet gets back to kernel we have lost the skb (as we do not buffer it in the kernel after issuing the upcall). ovs_ct_find_existing() can then be used to populate the nfct and nfctinfo fields of the new skb, as if this skb had gone through the nf_conntrack_in() call. This allows us to pass the packet through NAT after such an upcall without passing the packet through nf_conntrack_in() twice.

ovs_ct_find_existing() is intended to be used only when we have evidence in the OVS flow key that the packet actually went through nf_conntrack_in() prior to the upcall. In the current version of the patches I do not have a test case for this, and I intend to separate out this change into a separate patch before the NAT patch for the next (non-RFC) version.

I could implement ovs_ct_find_existing() using exported conntrack APIs apart from this mapping from conntrack status to enum ip_conntrack_info. ovs_ct_find_existing() is in no way OVS specific, though, so it could be added to the conntrack proper as well, if so desired.

  Jarno

>> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
>> ---
>> include/net/netfilter/nf_conntrack.h | 15 +++++++++++++++
>> net/netfilter/nf_conntrack_core.c    | 22 +++-------------------
>> 2 files changed, 18 insertions(+), 19 deletions(-)
>> 
>> diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
>> index fde4068..b3de10e 100644
>> --- a/include/net/netfilter/nf_conntrack.h
>> +++ b/include/net/netfilter/nf_conntrack.h
>> @@ -125,6 +125,21 @@ nf_ct_tuplehash_to_ctrack(const struct nf_conntrack_tuple_hash *hash)
>> 			    tuplehash[hash->tuple.dst.dir]);
>> }
>> 
>> +static inline enum ip_conntrack_info
>> +nf_ct_get_info(const struct nf_conntrack_tuple_hash *h)
>> +{
>> +	const struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
>> +
>> +	if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
>> +		return IP_CT_ESTABLISHED_REPLY;
>> +	/* Once we've had two way comms, always ESTABLISHED. */
>> +	if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status))
>> +		return IP_CT_ESTABLISHED;
>> +	if (test_bit(IPS_EXPECTED_BIT, &ct->status))
>> +		return IP_CT_RELATED;
>> +	return IP_CT_NEW;
>> +}
> 
> I would really like to see how you want to use this.

Patch
diff mbox

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index fde4068..b3de10e 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -125,6 +125,21 @@  nf_ct_tuplehash_to_ctrack(const struct nf_conntrack_tuple_hash *hash)
 			    tuplehash[hash->tuple.dst.dir]);
 }
 
+static inline enum ip_conntrack_info
+nf_ct_get_info(const struct nf_conntrack_tuple_hash *h)
+{
+	const struct nf_conn *ct = nf_ct_tuplehash_to_ctrack(h);
+
+	if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY)
+		return IP_CT_ESTABLISHED_REPLY;
+	/* Once we've had two way comms, always ESTABLISHED. */
+	if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status))
+		return IP_CT_ESTABLISHED;
+	if (test_bit(IPS_EXPECTED_BIT, &ct->status))
+		return IP_CT_RELATED;
+	return IP_CT_NEW;
+}
+
 static inline u_int16_t nf_ct_l3num(const struct nf_conn *ct)
 {
 	return ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.l3num;
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 3cb3cb8..70ddbd8 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1056,25 +1056,9 @@  resolve_normal_ct(struct net *net, struct nf_conn *tmpl,
 	ct = nf_ct_tuplehash_to_ctrack(h);
 
 	/* It exists; we have (non-exclusive) reference. */
-	if (NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY) {
-		*ctinfo = IP_CT_ESTABLISHED_REPLY;
-		/* Please set reply bit if this packet OK */
-		*set_reply = 1;
-	} else {
-		/* Once we've had two way comms, always ESTABLISHED. */
-		if (test_bit(IPS_SEEN_REPLY_BIT, &ct->status)) {
-			pr_debug("nf_conntrack_in: normal packet for %p\n", ct);
-			*ctinfo = IP_CT_ESTABLISHED;
-		} else if (test_bit(IPS_EXPECTED_BIT, &ct->status)) {
-			pr_debug("nf_conntrack_in: related packet for %p\n",
-				 ct);
-			*ctinfo = IP_CT_RELATED;
-		} else {
-			pr_debug("nf_conntrack_in: new packet for %p\n", ct);
-			*ctinfo = IP_CT_NEW;
-		}
-		*set_reply = 0;
-	}
+	*ctinfo = nf_ct_get_info(h);
+	*set_reply = NF_CT_DIRECTION(h) == IP_CT_DIR_REPLY;
+
 	skb->nfct = &ct->ct_general;
 	skb->nfctinfo = *ctinfo;
 	return ct;