On 11/23/22 16:21, Ilya Maximets wrote:
> On 11/8/22 19:15, Simon Horman wrote:
>> On Sat, Nov 05, 2022 at 11:11:53AM +0100, Ilya Maximets wrote:
>>> The value is right-justified after the string parsing with
>>> parse_int_string(), i.e. it is in BE byte order and aligned
>>> to the right side of the array.
>>>
>>> For example, the 0x10011 value in a 4-byte field will look
>>> like 0x00 0x01 0x00 0x11.
>>>
>>> However, value copy to the resulted ofpact is performed
>>> from the start of the memory. So, in case the destination
>>> size is smaller than the original field size, incorrect
>>> part of the value will be copied.
>>>
>>> In the 0x00 0x01 0x00 0x11 example above, if the copy is
>>> performed to a 3-byte field, the first 3 bytes will be
>>> copied, which are 0x00 0x01 0x00 instead of 0x01 0x00 0x11.
>>>
>>> This leads to a problem where NXM_NX_REG3[0..16]=0x10011
>>> turns into NXM_NX_REG3[0..16]=0x100 after the parsing.
>>>
>>> Fix that by offsetting the starting position to the size
>>> difference in bytes similarly to how it is done in
>>> learn_parse_load_immediate().
>>>
>>> Fixes: 21b2fa617126 ("ofp-parse: Allow match field names in actions and
>>> brackets in matches.")
>>> Reported-at:
>>> https://mail.openvswitch.org/pipermail/ovs-discuss/2022-November/052100.html
>>> Reported-by: Thomas Lee <[email protected]>
>>> Signed-off-by: Ilya Maximets <[email protected]>
>>> ---
>>> lib/learn.c | 4 +++-
>>> tests/learn.at | 4 ++--
>>> 2 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/lib/learn.c b/lib/learn.c
>>> index a40209ec0..cfd762527 100644
>>> --- a/lib/learn.c
>>> +++ b/lib/learn.c
>>> @@ -310,9 +310,11 @@ learn_parse_spec(const char *orig, char *name, char
>>> *value,
>>>
>>> /* Push value last, as this may reallocate 'spec'! */
>>> unsigned int imm_bytes = DIV_ROUND_UP(dst.n_bits, 8);
>>> + unsigned int offset = dst.field->n_bytes - imm_bytes;
>>> uint8_t *src_imm = ofpbuf_put_zeros(ofpacts,
>>>
>>> OFPACT_ALIGN(imm_bytes));
>>> - memcpy(src_imm, &imm, imm_bytes);
>>> +
>>> + memcpy(src_imm, (uint8_t *) &imm + offset, imm_bytes);
>>
>> FWIIW, This seems fine to me, but I wonder if
>> it would nicer (and indeed valid) to use &imm.u8 here.
>
> I guess, you meant 'imm.b', since we have mf_value and not mf_subvalue
> here. But I agree that it makes more sense. Thanks! I'll also change
> other places where &imm is used as a pointer to a byte array to make
> the code a bit more clear. Will send v2 shortly.
Hmm, mf_write_subfield_value() call just above also seems to be incorrect.
I'll re-check that part before sending v2...
>
> Sorry for the late reply, just got back from my PTO.
>
> Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
