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 <newsfortho...@engineer.com> >> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org> >> --- >> 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. Sorry for the late reply, just got back from my PTO. Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev