[ovs-dev] [PATCHv2 1/2] ofproto-dpif-xlate: Fix bitwise ops on ct_mark.

[Joe Stringer]

On 4 April 2016 at 11:20, Joe Stringer <joe at ovn.org> wrote:
> On 30 March 2016 at 19:55, Ben Pfaff <blp at ovn.org> wrote:
>> On Thu, Mar 31, 2016 at 12:21:13AM +1300, Joe Stringer wrote:
>>> Using the action ct(commit,set_field:0x1/0x1->ct_mark), ie, specifying a
>>> mask, would previously overwrite the entire ct_mark field rather than
>>> modifying only the specified bits. Fix the issue.
>>>
>>> Fixes: 8e53fe8cf7a1 ("Add connection tracking mark support.")
>>> Signed-off-by: Joe Stringer <joe at ovn.org>
>>> ---
>>> v2: Remove wildcards from put_ct_mark().
>>
>> I didn't test this but it looks good and compiles cleanly.
>>
>> A possible (though unnecessary) enhancement to the test would be to
>> verify that setting a bit to 0 also works.
>>
>> Acked-by: Ben Pfaff <blp at ovn.org>
>
> Thanks for the review.
>
> Actually, the "set-0" test is necessary and reveals that this series
> isn't really targeting the core issue.
>
> I was a bit confused about how 'wc' were being used and what's the
> 'correct' way to handle these. After discussion with Jarno I
> understand that the 'wc' represents both the incoming match on a given
> field, but also the bits that have been modified in a 'set_field'
> action. Furthermore, as soon as a 'set_field' action is translated,
> the field is fully masked. This is actually what leads to the
> incorrect behaviour for setting the ct_mark/ct_labels. Even when
> modifying just a single bit of a field, the entire field is marked to
> change in the 'wc'.
>
> I'm looking at preparing a patch which will only mask the bits which
> are modified by the set_field action.

I should add that this bug only affects bitmask generation when
performing ct(...,exec(set_field...)) on packets that haven't yet been
through the connection tracker, eg:

in_port=1,actions=ct(commit,exec(set_field(0x1/0x1->ct_mark))),2
in_port=2,actions=ct(commit,exec(set_field(0x2/0x2->ct_mark))),1

Connections flowing through this pipeline will constantly flip between
having ct_mark=0x1 and ct_mark=0x2, rather than having ct_mark=0x3.

I intend to drop this series in favour of a different fix.