From patchwork Wed Nov 23 21:23:37 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1708488 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::136; helo=smtp3.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp3.osuosl.org (smtp3.osuosl.org [IPv6:2605:bc80:3010::136]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4NHYyg5XFrz23lT for ; Thu, 24 Nov 2022 08:23:39 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id F1BB9610AB; Wed, 23 Nov 2022 21:23:37 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org F1BB9610AB X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id pClo-wC0LJ-i; Wed, 23 Nov 2022 21:23:37 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp3.osuosl.org (Postfix) with ESMTPS id 1E3A4606B0; Wed, 23 Nov 2022 21:23:36 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 1E3A4606B0 Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id D6E4CC0032; Wed, 23 Nov 2022 21:23:35 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp1.osuosl.org (smtp1.osuosl.org [IPv6:2605:bc80:3010::138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 97B4CC002D for ; Wed, 23 Nov 2022 21:23:34 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id 80D0C8128C for ; Wed, 23 Nov 2022 21:23:34 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 80D0C8128C X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id dBB0kAMrHjdN for ; Wed, 23 Nov 2022 21:23:32 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 887B980DBF Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by smtp1.osuosl.org (Postfix) with ESMTPS id 887B980DBF for ; Wed, 23 Nov 2022 21:23:31 +0000 (UTC) Received: (Authenticated sender: i.maximets@ovn.org) by mail.gandi.net (Postfix) with ESMTPSA id 279BC60006; Wed, 23 Nov 2022 21:23:27 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Wed, 23 Nov 2022 22:23:37 +0100 Message-Id: <20221123212337.392503-1-i.maximets@ovn.org> X-Mailer: git-send-email 2.38.1 MIME-Version: 1.0 Cc: Simon Horman , Thomas Lee , Ilya Maximets Subject: [ovs-dev] [PATCH v2] learn: Fix parsing immediate value for a field match. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" 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(). While at it, changing &imm to imm.b in function calls that expect byte arrays as an argument. The old way is technically correct, but more error prone. The mf_write_subfield_value() call was also incorrect. However, the 'match' variable is actually not used for anything since checking removal in commit: dd43a558597b ("Do not perform validation in learn_parse();") So, just removing the call and the 'match' variable entirely instead of fixing it. 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 Signed-off-by: Ilya Maximets Reviewed-by: Simon Horman --- Version 2: - Switch from using byte arithmetic on a union mf_value address to the byte array inside of it. This makes a code a bit more clear and easier to read. - Removed the incorrect call to mf_write_subfield_value() along with the unused 'match' variable. lib/learn.c | 18 +++++++----------- tests/learn.at | 4 ++-- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/lib/learn.c b/lib/learn.c index a40209ec0..a62add2fd 100644 --- a/lib/learn.c +++ b/lib/learn.c @@ -241,7 +241,7 @@ static char * OVS_WARN_UNUSED_RESULT learn_parse_spec(const char *orig, char *name, char *value, const struct ofputil_port_map *port_map, struct ofpact_learn_spec *spec, - struct ofpbuf *ofpacts, struct match *match) + struct ofpbuf *ofpacts) { /* Parse destination and check prerequisites. */ struct mf_subfield dst; @@ -275,14 +275,14 @@ learn_parse_spec(const char *orig, char *name, char *value, } else { char *tail; /* Partial field value. */ - if (parse_int_string(value, (uint8_t *)&imm, + if (parse_int_string(value, imm.b, dst.field->n_bytes, &tail) || *tail != 0) { imm_error = xasprintf("%s: cannot parse integer value", orig); } if (!imm_error && - !bitwise_is_all_zeros(&imm, dst.field->n_bytes, + !bitwise_is_all_zeros(imm.b, dst.field->n_bytes, dst.n_bits, dst.field->n_bytes * 8 - dst.n_bits)) { struct ds ds; @@ -304,15 +304,13 @@ learn_parse_spec(const char *orig, char *name, char *value, spec->src_type = NX_LEARN_SRC_IMMEDIATE; - /* Update 'match' to allow for satisfying destination - * prerequisites. */ - mf_write_subfield_value(&dst, &imm, match); - /* Push value last, as this may reallocate 'spec'! */ unsigned int imm_bytes = DIV_ROUND_UP(dst.n_bits, 8); uint8_t *src_imm = ofpbuf_put_zeros(ofpacts, OFPACT_ALIGN(imm_bytes)); - memcpy(src_imm, &imm, imm_bytes); + + memcpy(src_imm, &imm.b[dst.field->n_bytes - imm_bytes], + imm_bytes); free(error); return NULL; @@ -391,7 +389,6 @@ learn_parse__(char *orig, char *arg, const struct ofputil_port_map *port_map, struct ofpbuf *ofpacts) { struct ofpact_learn *learn; - struct match match; char *name, *value; learn = ofpact_put_LEARN(ofpacts); @@ -400,7 +397,6 @@ learn_parse__(char *orig, char *arg, const struct ofputil_port_map *port_map, learn->priority = OFP_DEFAULT_PRIORITY; learn->table_id = 1; - match_init_catchall(&match); while (ofputil_parse_key_value(&arg, &name, &value)) { if (!strcmp(name, "table")) { if (!ofputil_table_from_string(value, table_map, @@ -448,7 +444,7 @@ learn_parse__(char *orig, char *arg, const struct ofputil_port_map *port_map, spec = ofpbuf_put_zeros(ofpacts, sizeof *spec); error = learn_parse_spec(orig, name, value, port_map, - spec, ofpacts, &match); + spec, ofpacts); if (error) { return error; } diff --git a/tests/learn.at b/tests/learn.at index 5f1d6df9d..d127fed34 100644 --- a/tests/learn.at +++ b/tests/learn.at @@ -6,7 +6,7 @@ actions=learn() actions=learn(send_flow_rem) actions=learn(delete_learned) actions=learn(send_flow_rem,delete_learned) -actions=learn(NXM_OF_VLAN_TCI[0..11], NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], output:NXM_OF_IN_PORT[], load:10->NXM_NX_REG0[5..10]) +actions=learn(NXM_OF_VLAN_TCI[0..11], NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[], NXM_NX_REG3[3..19]=0x10011, output:NXM_OF_IN_PORT[], load:10->NXM_NX_REG0[5..10]) actions=learn(table=1,idle_timeout=10, hard_timeout=20, fin_idle_timeout=5, fin_hard_timeout=10, priority=10, cookie=0xfedcba9876543210, in_port=99,eth_dst=eth_src,load:in_port->reg1[16..31]) actions=learn(limit=4096) actions=learn(limit=4096,result_dst=reg0[0]) @@ -18,7 +18,7 @@ OFPT_FLOW_MOD (xid=0x1): ADD actions=learn(table=1) OFPT_FLOW_MOD (xid=0x2): ADD actions=learn(table=1,send_flow_rem) OFPT_FLOW_MOD (xid=0x3): ADD actions=learn(table=1,delete_learned) OFPT_FLOW_MOD (xid=0x4): ADD actions=learn(table=1,send_flow_rem,delete_learned) -OFPT_FLOW_MOD (xid=0x5): ADD actions=learn(table=1,NXM_OF_VLAN_TCI[0..11],NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],output:NXM_OF_IN_PORT[],load:0xa->NXM_NX_REG0[5..10]) +OFPT_FLOW_MOD (xid=0x5): ADD actions=learn(table=1,NXM_OF_VLAN_TCI[0..11],NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],NXM_NX_REG3[3..19]=0x10011,output:NXM_OF_IN_PORT[],load:0xa->NXM_NX_REG0[5..10]) OFPT_FLOW_MOD (xid=0x6): ADD actions=learn(table=1,idle_timeout=10,hard_timeout=20,fin_idle_timeout=5,fin_hard_timeout=10,priority=10,cookie=0xfedcba9876543210,in_port=99,NXM_OF_ETH_DST[]=NXM_OF_ETH_SRC[],load:NXM_OF_IN_PORT[]->NXM_NX_REG1[16..31]) OFPT_FLOW_MOD (xid=0x7): ADD actions=learn(table=1,limit=4096) OFPT_FLOW_MOD (xid=0x8): ADD actions=learn(table=1,limit=4096,result_dst=NXM_NX_REG0[0])