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 <ja...@ovn.org <mailto:ja...@ovn.org>>
Jarno
> On Mar 18, 2016, at 10:28 AM, Dan Carpenter <dan.carpen...@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.carpen...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
