[ovs-dev] [patch v2] openvswitch: using a bit shift as a mask

Jarno Rajahalme jarno at ovn.org
Fri Mar 18 23:48:13 UTC 2016


Dan,

Thanks again for finding this bug! Running the OVS datapath testsuite I found that the use of a bit shift as a mask actually hid another bug: I had assumed that the IPS_EXPECTED_BIT is set for the first packet of a new connection only, while its definition is accompanied with a comment stating that “it [is] never changed”, i.e., the bit persists through the lifetime of the connection. The ctinfo value IP_CT_RELATED, however, is only set for expected packets that otherwise would be given the IP_CT_NEW value. Thus we should use a ‘ctinfo != IP_CT_RELATED’ to filter for new expected connections, instead of '!test_bit(IPS_EXPECTED_BIT, &ct->status)’. The resulting patch, with a comment clarification, would look like this:

diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
index dc5eb29..47f7c62 100644
--- a/net/openvswitch/conntrack.c
+++ b/net/openvswitch/conntrack.c
@@ -664,11 +664,12 @@ static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
 
 	/* Determine NAT type.
 	 * Check if the NAT type can be deduced from the tracked connection.
-	 * Make sure expected traffic is NATted only when committing.
+	 * Make sure new expected connections (IP_CT_RELATED) are NATted only
+	 * when committing.
 	 */
 	if (info->nat & OVS_CT_NAT && ctinfo != IP_CT_NEW &&
 	    ct->status & IPS_NAT_MASK &&
-	    (!(ct->status & IPS_EXPECTED_BIT) || info->commit)) {
+	    (ctinfo != IP_CT_RELATED || info->commit)) {
 		/* NAT an established or related connection like before. */
 		if (CTINFO2DIR(ctinfo) == IP_CT_DIR_REPLY)
 			/* This is the REPLY direction for a connection

So while your patch is correctly fixing a bug, the fix reveals another bug that breaks test cases. Maybe it would be better to send a new series with your fix as the first patch, and this one as the second patch? If so, here is my signed-off-by:

Signed-off-by: Jarno Rajahalme <jarno at ovn.org <mailto:jarno at ovn.org>>

  Jarno

> On Mar 18, 2016, at 10:28 AM, Dan Carpenter <dan.carpenter at oracle.com> wrote:
> 
> The original condition is never true.  We want to test if BIT(0) is set
> but the code is ANDing with zero.
> 
> Fixes: 05752523e565 ('openvswitch: Interface with NAT.')
> Signed-off-by: Dan Carpenter <dan.carpenter at oracle.com>
> ---
> v2: use test_bit() instead
> 
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index dc5eb29..9c9cac0 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -668,7 +668,7 @@ static int ovs_ct_nat(struct net *net, struct sw_flow_key *key,
> 	 */
> 	if (info->nat & OVS_CT_NAT && ctinfo != IP_CT_NEW &&
> 	    ct->status & IPS_NAT_MASK &&
> -	    (!(ct->status & IPS_EXPECTED_BIT) || info->commit)) {
> +	    (!test_bit(IPS_EXPECTED_BIT, &ct->status) || info->commit)) {
> 		/* NAT an established or related connection like before. */
> 		if (CTINFO2DIR(ctinfo) == IP_CT_DIR_REPLY)
> 			/* This is the REPLY direction for a connection




More information about the dev mailing list