"Iyengar, Prashanth" <prashanth_iyen...@alliedtelesis.com> writes:
> From fc0a9e1329573692c04438370f85e87125a268b0 Mon Sep 17 00:00:00 2001
> From: Prashanth Iyengar <prashanth_iyen...@alliedtelesis.com>
> Date: Mon, 14 Jan 2019 12:53:11 -0800
> Subject: [PATCH] Fix OpenFlow v1.3.4 Conf test failures: 430.500, 430.510
>
> This commit adds additional verification to nx_pull_header__() in
> lib/nx-match.c to distinguish between bad match and bad action header
> conditions and return the appropriate error type/code.
>
> Signed-off-by: Prashanth Iyengar <prashanth_iyen...@alliedtelesis.com>
> Reviewed-by: Tony van der Peet <tony.vanderp...@alliedtelesis.co.nz>
> Reviewed-by: Rahul Gupta <rahul_gu...@alliedtelesis.com>
> ---
There seems to be some formatting issues not just with the code, but
also with this particular patch. If it continues to be an issue,
consider either attaching the patch to the message, or submitting a pull
request on github and notifying the list.
> lib/meta-flow.c | 2 +-
> lib/nx-match.c | 62 +++++++++++++++++++++++++++++++++++++-------
> lib/nx-match.h | 2 +-
> tests/ofp-actions.at | 14 ++++++++++
> 4 files changed, 68 insertions(+), 12 deletions(-)
>
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c index b6d9e92b6..8f9662943
> 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -3503,7 +3503,7 @@ mf_vl_mff_nx_pull_entry(struct ofpbuf *b, const struct
> vl_mff_map *vl_mff_map,
> const struct mf_field **field, union mf_value *value,
> union mf_value *mask, uint64_t *tlv_bitmap) {
> - enum ofperr error = nx_pull_entry(b, vl_mff_map, field, value, mask);
> + enum ofperr error = nx_pull_entry(b, vl_mff_map, field, value,
> + mask, true);
The spacing here isn't correct.
> if (error) {
> return error;
> }
> diff --git a/lib/nx-match.c b/lib/nx-match.c index fd3eac0c0..33b95a2d4 100644
> --- a/lib/nx-match.c
> +++ b/lib/nx-match.c
> @@ -97,6 +97,7 @@ enum ofp12_oxm_class {
>
> /* Functions for extracting raw field values from OXM/NXM headers. */
> static uint32_t nxm_vendor(uint64_t header) { return header; }
> +enum ofperr nxm_validate_action_field_header(uint64_t header);
> static int nxm_class(uint64_t header) { return header >> 48; } static int
> nxm_field(uint64_t header) { return (header >> 41) & 0x7f; } static bool
> nxm_hasmask(uint64_t header) { return (header >> 40) & 1; } @@ -313,7 +314,7
> @@ is_cookie_pseudoheader(uint64_t header) static enum ofperr
> nx_pull_header__(struct ofpbuf *b, bool allow_cookie,
Looks like a number of lines got collapsed?
> const struct vl_mff_map *vl_mff_map, uint64_t *header,
> - const struct mf_field **field)
> + const struct mf_field **field, bool is_action)
> {
> if (b->size < 4) {
> goto bad_len;
> @@ -340,7 +341,16 @@ nx_pull_header__(struct ofpbuf *b, bool allow_cookie,
> if (!*field && !(allow_cookie && is_cookie_pseudoheader(*header))) {
> VLOG_DBG_RL(&rl, "OXM header "NXM_HEADER_FMT" is unknown",
> NXM_HEADER_ARGS(*header));
> - return OFPERR_OFPBMC_BAD_FIELD;
> + if (is_action) {
> + enum ofperr h_error =
> nxm_validate_action_field_header(*header);
> + if (h_error) {
> + *field = NULL;
> + return h_error;
> + }
> + return OFPERR_OFPBAC_BAD_SET_TYPE;
> + } else {
> + return OFPERR_OFPBMC_BAD_FIELD;
> + }
> } else if (mf_vl_mff_invalid(*field, vl_mff_map)) {
> return OFPERR_NXFMFC_INVALID_TLV_FIELD;
> }
> @@ -381,7 +391,7 @@ static enum ofperr
> nx_pull_entry__(struct ofpbuf *b, bool allow_cookie,
> const struct vl_mff_map *vl_mff_map, uint64_t *header,
> const struct mf_field **field_,
> - union mf_value *value, union mf_value *mask)
> + union mf_value *value, union mf_value *mask, bool
> + is_action)
Likewise, spacing
> {
> const struct mf_field *field;
> enum ofperr header_error;
> @@ -390,7 +400,7 @@ nx_pull_entry__(struct ofpbuf *b, bool allow_cookie,
> int width;
>
> header_error = nx_pull_header__(b, allow_cookie, vl_mff_map, header,
> - &field);
> + &field, is_action);
> if (header_error && header_error != OFPERR_OFPBMC_BAD_FIELD) {
> return header_error;
> }
> @@ -442,15 +452,20 @@ nx_pull_entry__(struct ofpbuf *b, bool allow_cookie,
> *
> * If a NULL 'mask' is supplied, masked OXM or NXM entries are treated as
> * errors (with OFPERR_OFPBMC_BAD_MASK).
> + *
> + * The "bool is_action" is supplied to differentiate between match and
> + action
> + * headers. This is done in order to return appropriate error type and
> + code for
> + * bad match or bad action conditions. If set to True, indicates that
> + the
> + * OXM or NXM entries belong to an action header.
> */
> enum ofperr
> nx_pull_entry(struct ofpbuf *b, const struct vl_mff_map *vl_mff_map,
> const struct mf_field **field, union mf_value *value,
> - union mf_value *mask)
> + union mf_value *mask, bool is_action)
> {
> uint64_t header;
>
> - return nx_pull_entry__(b, false, vl_mff_map, &header, field, value,
> mask);
> + return nx_pull_entry__(b, false, vl_mff_map, &header, field, value,
> + mask, is_action);
> }
>
> /* Attempts to pull an NXM or OXM header from the beginning of 'b'. If @@
> -470,7 +485,7 @@ nx_pull_header(struct ofpbuf *b, const struct vl_mff_map
> *vl_mff_map,
> enum ofperr error;
> uint64_t header;
>
> - error = nx_pull_header__(b, false, vl_mff_map, &header, field);
> + error = nx_pull_header__(b, false, vl_mff_map, &header, field,
> + false);
> if (masked) {
> *masked = !error && nxm_hasmask(header);
> } else if (!error && nxm_hasmask(header)) { @@ -489,7 +504,7 @@
> nx_pull_match_entry(struct ofpbuf *b, bool allow_cookie,
Something weird here.
> uint64_t header;
>
> error = nx_pull_entry__(b, allow_cookie, vl_mff_map, &header, field,
> value,
> - mask);
> + mask, false);
> if (error) {
> return error;
> }
> @@ -739,7 +754,7 @@ oxm_pull_field_array(const void *fields_data, size_t
> fields_len,
> uint64_t header;
>
> error = nx_pull_entry__(&b, false, NULL, &header, &field, &value,
> - NULL);
> + NULL, false);
> if (error) {
> VLOG_DBG_RL(&rl, "error pulling field array field");
> } else if (!field) {
> @@ -1488,7 +1503,7 @@ nx_match_to_string(const uint8_t *p, unsigned int
> match_len)
> uint64_t header;
> int value_len;
>
> - error = nx_pull_entry__(&b, true, NULL, &header, NULL, &value,
> &mask);
> + error = nx_pull_entry__(&b, true, NULL, &header, NULL, &value,
> + &mask, false);
> if (error) {
> break;
> }
> @@ -2244,6 +2259,33 @@ nxm_field_by_header(uint64_t header)
> return NULL;
> }
>
> +enum ofperr
> +nxm_validate_action_field_header(uint64_t header) {
Please reformat as:
+enum ofperr nxm_validate_action_field_header(uint64_t header)
+{
> + const struct nxm_field_index *nfi;
> + uint64_t header_no_len;
> +
> + nxm_init();
> + if (nxm_hasmask(header)) {
> + header = nxm_make_exact_header(header);
> + }
> +
> + header_no_len = nxm_no_len(header);
> +
> + HMAP_FOR_EACH_IN_BUCKET (nfi, header_node, hash_uint64(header_no_len),
> + &nxm_header_map) {
> + if (header_no_len == nxm_no_len(nfi->nf.header)) {
> + if (nxm_length(header) > 0) {
> + if (nxm_length(header) != nxm_length(nfi->nf.header)) {
> + return OFPERR_OFPBAC_BAD_SET_LEN;
> + }
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> static const struct nxm_field *
> nxm_field_by_name(const char *name, size_t len) { diff --git
> a/lib/nx-match.h b/lib/nx-match.h index e304072d3..9be40a981 100644
Something weird here with the patch...
> --- a/lib/nx-match.h
> +++ b/lib/nx-match.h
> @@ -82,7 +82,7 @@ int oxm_put_field_array(struct ofpbuf *, const struct
> field_array *,
> * ID followed by a value and possibly a mask). */ enum ofperr
> nx_pull_entry(struct ofpbuf *, const struct vl_mff_map *,
> const struct mf_field **, union mf_value *value,
> - union mf_value *mask);
> + union mf_value *mask, bool is_action);
> enum ofperr nx_pull_header(struct ofpbuf *, const struct vl_mff_map *,
> const struct mf_field **, bool *masked); void
> nxm_put_entry_raw(struct ofpbuf *, enum mf_field_id field, diff --git
> a/tests/ofp-actions.at b/tests/ofp-actions.at index e320a92a8..38c2b5cbd
> 100644
> --- a/tests/ofp-actions.at
> +++ b/tests/ofp-actions.at
> @@ -746,6 +746,20 @@ dnl Check the ONF extension form of "copy_field".
> # actions=move:NXM_OF_IN_PORT[]->NXM_OF_VLAN_TCI[]
> ffff 0020 4f4e4600 0c80 0000 0010 0000 0000 0000 00000002 00000802 00000000
>
> +dnl Check OpenFlow v1.3.4 Conformance Test: 430.500.
> +# bad OpenFlow13 actions: OFPBAC_BAD_SET_TYPE & ofp_actions|WARN|bad
> +action at offset 0 (OFPBAC_BAD_SET_TYPE):
> +& 00000000 00 19 00 08 80 00 fe 00-00 00 00 10 00 00 00 01 & 00000010
> +00 00 00 00 00 00 00 00-
> +0019 0008 8000fe00 000000100000 000100000000 00000000
> +
> +dnl Check OpenFlow v1.3.4 Conformance Test: 430.510.
> +# bad OpenFlow13 actions: OFPBAC_BAD_SET_LEN & ofp_actions|WARN|bad
> +action at offset 0 (OFPBAC_BAD_SET_LEN):
> +& 00000000 00 19 00 10 80 00 08 07-00 01 02 03 04 05 00 00 & 00000010
> +00 00 00 10 00 00 00 01-
> +0019 0010 80000807 000102030405 000000000010 00000001
> +
> ])
> sed '/^[[#&]]/d' < test-data > input.txt sed -n 's/^# //p; /^$/p' <
> test-data > expout
> --
> 2.17.1
>
> This e-mail message is for the sole use of the intended recipient(s) and may
> contain confidential and privileged information. Any unauthorized review,
> use, disclosure or distribution is prohibited. If you are not the intended
> recipient, please contact the sender by reply E-mail and destroy all copies
> of the original message. If you are the intended recipient, please be advised
> that the content of this message is subject to access, review and disclosure
> by the sender's E-mail System Administrator.
>
> Privacy Policy
These trailers aren't appropriate on a public mailing list.
> We are committed to respecting your privacy and keeping you informed about
> our practices. Read our Privacy Policy: Allied Telesis Privacy
> Policy<http://www.alliedtelesis.com/legal/privacy>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
