Assorted fixes for the flow API
cover Assorted fixes for the flow API
This series makes a few adjustments to the flow API (rte_flow) based on issues encountered by preliminary PMD implementations and addresses bugs reported by Coverity in the testpmd flow command.
Adrien Mazarguil (5):
app/testpmd: fix array bounds checks
ethdev: modify flow API's error setting function
ethdev: clarify MARK and FLAG actions in flow API
ethdev: clarify RSS action in flow API
ethdev: define default item masks in flow API
app/test-pmd/config.c | 8 +- app/test-pmd/rxonly.c | 3 +- doc/guides/prog_guide/rte_flow.rst | 57 +++++++------ lib/librte_ether/rte_flow.c | 24 +++--- lib/librte_ether/rte_flow.h | 137 +++++++++++++++++++++++++++----- lib/librte_ether/rte_flow_driver.h | 6 +- 6 files changed, 170 insertions(+), 65 deletions(-)
Signature · 1 line
2.1.4
PATCH 1/5 app/testpmd: fix array bounds checks
This commit addresses several obvious issues reported by Coverity (139596, 139597, 139598 and 139599) with array bounds checks in functions related to the flow API.
--- app/test-pmd/config.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
app/test-pmd/config.c +4 -4
@@ -884,7 +884,7 @@ port_flow_new(const struct rte_flow_attr *attr, do { struct rte_flow_item *dst = NULL; - if ((unsigned int)item->type > RTE_DIM(flow_item) || + if ((unsigned int)item->type >= RTE_DIM(flow_item) || !flow_item[item->type].name) goto notsup; if (pf)
@@ -918,7 +918,7 @@ port_flow_new(const struct rte_flow_attr *attr, do { struct rte_flow_action *dst = NULL; - if ((unsigned int)action->type > RTE_DIM(flow_action) || + if ((unsigned int)action->type >= RTE_DIM(flow_action) || !flow_action[action->type].name) goto notsup; if (pf)
@@ -977,7 +977,7 @@ port_flow_complain(struct rte_flow_error *error)char buf[32 ];int err = rte_errno; -if ((unsigned int )error->type >RTE_DIM (errstrlist) || +if ((unsigned int )error->type >=RTE_DIM (errstrlist) || !errstrlist[error->type]) errstr ="unknown type" ;else
@@ -1146,7 +1146,7 @@ port_flow_query(portid_t port_id, uint32_t rule,printf ("Flow rule #%u not found\n" , rule);return -ENOENT ; } -if ((unsigned int )action >RTE_DIM (flow_action) || +if ((unsigned int )action >=RTE_DIM (flow_action) || !flow_action[action].name) name ="unknown" ;else
Signature · 1 line
2.1.4
PATCH 2/5 ethdev: modify flow API's error setting function
In reply to Adrien MazarguilBased on initial PMD implementations of the flow API, returning the error structure which may be NULL is useless and always discarded.
Returning the error code instead appears to be much more convenient.
--- lib/librte_ether/rte_flow.c | 24 ++++++++++++------------ lib/librte_ether/rte_flow_driver.h | 6 +++--- 2 files changed, 15 insertions(+), 15 deletions(-)
lib/librte_ether/rte_flow.c +12 -12
@@ -78,9 +78,9 @@ rte_flow_validate(uint8_t port_id,return -rte_errno;if (likely (!!ops->validate))return ops->validate (dev, attr, pattern, actions, error); -rte_flow_error_set (error,ENOSYS ,RTE_FLOW_ERROR_TYPE_UNSPECIFIED , -NULL ,rte_strerror (ENOSYS )); -return -rte_errno; +return -rte_flow_error_set (error,ENOSYS , +RTE_FLOW_ERROR_TYPE_UNSPECIFIED , +NULL ,rte_strerror (ENOSYS )); }/* Create a flow rule on a given port. */
@@ -116,9 +116,9 @@ rte_flow_destroy(uint8_t port_id,return -rte_errno;if (likely (!!ops->destroy))return ops->destroy (dev, flow, error); -rte_flow_error_set (error,ENOSYS ,RTE_FLOW_ERROR_TYPE_UNSPECIFIED , -NULL ,rte_strerror (ENOSYS )); -return -rte_errno; +return -rte_flow_error_set (error,ENOSYS , +RTE_FLOW_ERROR_TYPE_UNSPECIFIED , +NULL ,rte_strerror (ENOSYS )); }/* Destroy all flow rules associated with a port. */
@@ -133,9 +133,9 @@ rte_flow_flush(uint8_t port_id,return -rte_errno;if (likely (!!ops->flush))return ops->flush (dev, error); -rte_flow_error_set (error,ENOSYS ,RTE_FLOW_ERROR_TYPE_UNSPECIFIED , -NULL ,rte_strerror (ENOSYS )); -return -rte_errno; +return -rte_flow_error_set (error,ENOSYS , +RTE_FLOW_ERROR_TYPE_UNSPECIFIED , +NULL ,rte_strerror (ENOSYS )); }/* Query an existing flow rule. */
@@ -153,7 +153,7 @@ rte_flow_query(uint8_t port_id,return -rte_errno;if (likely (!!ops->query))return ops->query (dev, flow, action, data, error); -rte_flow_error_set (error,ENOSYS ,RTE_FLOW_ERROR_TYPE_UNSPECIFIED , -NULL ,rte_strerror (ENOSYS )); -return -rte_errno; +return -rte_flow_error_set (error,ENOSYS , +RTE_FLOW_ERROR_TYPE_UNSPECIFIED , +NULL ,rte_strerror (ENOSYS )); }
lib/librte_ether/rte_flow_driver.h +3 -3
@@ -139,9 +139,9 @@ struct rte_flow_ops {* Human-readable error message. * * @return -* Pointer to flow error structure. +* Error code (\code). */ -static inline struct rte_flow_error * +static inline int rte_flow_error_set(struct rte_flow_error *error, int code, enum rte_flow_error_type type,
@@ -156,7 +156,7 @@ rte_flow_error_set(struct rte_flow_error *error, }; } rte_errno = code; -return error; +return code; }/**
Signature · 1 line
2.1.4
PATCH 3/5 ethdev: clarify MARK and FLAG actions in flow API
In reply to Adrien MazarguilBoth actions share the PKT_RX_FDIR mbuf flag, as a result there is no way to tell them apart. Moreover, the maximum allowed value for the MARK action may not necessarily cover the entire 32-bit space.
--- doc/guides/prog_guide/rte_flow.rst | 24 ++++++++++++------------ lib/librte_ether/rte_flow.h | 19 ++++++++++--------- 2 files changed, 22 insertions(+), 21 deletions(-)
doc/guides/prog_guide/rte_flow.rst +12 -12
@@ -1049,31 +1049,31 @@ flow rules: Action: ``MARK`` ^^^^^^^^^^^^^^^^ -Attaches a 32 bit value to packets. +Attaches an integer value to packets and sets ``PKT_RX_FDIR`` and +``PKT_RX_FDIR_ID`` mbuf flags. -This value is arbitrary and application-defined. For compatibility with FDIR -it is returned in the ``hash.fdir.hi`` mbuf field. ``PKT_RX_FDIR_ID`` is -also set in ``ol_flags``. +This value is arbitrary and application-defined. Maximum allowed value +depends on the underlying implementation. It is returned in the +``hash.fdir.hi`` mbuf field. .. _table_rte_flow_action_mark: .. table:: MARK - +--------+-------------------------------------+ - | Field | Value | - +========+=====================================+ - | ``id`` | 32 bit value to return with packets | - +--------+-------------------------------------+ + +--------+--------------------------------------+ + | Field | Value | + +========+======================================+ + | ``id`` | integer value to return with packets | + +--------+--------------------------------------+ Action: ``FLAG`` ^^^^^^^^^^^^^^^^ -Flag packets. Similar to `Action: MARK`_ but only affects ``ol_flags``. +Flags packets. Similar to `Action: MARK`_ without a specific value; only +sets the ``PKT_RX_FDIR`` mbuf flag. - No configurable properties. -Note: a distinctive flag must be defined for it. - .. _table_rte_flow_action_flag: .. table:: FLAG
lib/librte_ether/rte_flow.h +10 -9
@@ -556,7 +556,8 @@ enum rte_flow_action_type {/** * [META] * -* Attaches a 32 bit value to packets. +* Attaches an integer value to packets and sets PKT_RX_FDIR and +* PKT_RX_FDIR_ID mbuf flags. * * See struct rte_flow_action_mark. */
@@ -565,9 +566,8 @@ enum rte_flow_action_type {/** * [META] * -* Flag packets. Similar to MARK but only affects ol_flags. -* -* Note: a distinctive flag must be defined for it. +* Flags packets. Similar to MARK without a specific value; only +* sets the PKT_RX_FDIR mbuf flag. * * No associated configuration structure. */
@@ -640,14 +640,15 @@ enum rte_flow_action_type {/** * RTE_FLOW_ACTION_TYPE_MARK * -* Attaches a 32 bit value to packets. +* Attaches an integer value to packets and sets PKT_RX_FDIR and +* PKT_RX_FDIR_ID mbuf flags. * -* This value is arbitrary and application-defined. For compatibility with -* FDIR it is returned in the hash.fdir.hi mbuf field. PKT_RX_FDIR_ID is -* also set in ol_flags. +* This value is arbitrary and application-defined. Maximum allowed value +* depends on the underlying implementation. It is returned in the +* hash.fdir.hi mbuf field. */ struct rte_flow_action_mark { -uint32_t id;/**< 32 bit value to return with packets. */ +uint32_t id;/**< Integer value to return with packets. */ };/**
Signature · 1 line
2.1.4
PATCH 4/5 ethdev: clarify RSS action in flow API
In reply to Adrien MazarguilContrary to the current description, mbuf RSS hash result storage does not overlap with the returned MARK value (hash.fdir.lo vs. hash.fdir.hi), and both may be combined.
Reflect this change by allowing testpmd to display both values simultaneously.
--- app/test-pmd/rxonly.c | 3 ++- doc/guides/prog_guide/rte_flow.rst | 8 +++----- lib/librte_ether/rte_flow.h | 8 +++----- 3 files changed, 8 insertions(+), 11 deletions(-)
app/test-pmd/rxonly.c +2 -1
@@ -148,7 +148,8 @@ pkt_burst_receive(struct fwd_stream *fs)if (ol_flags &PKT_RX_RSS_HASH ) {printf (" - RSS hash=0x%x" , (unsigned ) mb->hash.rss);printf (" - RSS queue=0x%x" ,(unsigned ) fs->rx_queue); - }else if (ol_flags &PKT_RX_FDIR ) { + } +if (ol_flags &PKT_RX_FDIR ) {printf (" - FDIR matched " );if (ol_flags &PKT_RX_FDIR_ID ) printf("ID=0x%x",
doc/guides/prog_guide/rte_flow.rst +3 -5
@@ -1187,11 +1187,9 @@ Action: ``RSS`` Similar to QUEUE, except RSS is additionally performed on packets to spread them among several queues according to the provided parameters. -Note: RSS hash result is normally stored in the ``hash.rss`` mbuf field, -however it conflicts with `Action: MARK`_ as they share the same space. When -both actions are specified, the RSS hash is discarded and -``PKT_RX_RSS_HASH`` is not set in ``ol_flags``. MARK has priority. The mbuf -structure should eventually evolve to store both. +Note: RSS hash result is stored in the ``hash.rss`` mbuf field which +overlaps ``hash.fdir.lo``. Since `Action: MARK`_ sets the ``hash.fdir.hi`` +field only, both can be requested simultaneously. - Terminating by default.
lib/librte_ether/rte_flow.h +3 -5
@@ -696,11 +696,9 @@ struct rte_flow_action_dup {* Similar to QUEUE, except RSS is additionally performed on packets to * spread them among several queues according to the provided parameters. * -* Note: RSS hash result is normally stored in the hash.rss mbuf field, -* however it conflicts with the MARK action as they share the same -* space. When both actions are specified, the RSS hash is discarded and -* PKT_RX_RSS_HASH is not set in ol_flags. MARK has priority. The mbuf -* structure should eventually evolve to store both. +* Note: RSS hash result is stored in the hash.rss mbuf field which overlaps +* hash.fdir.lo. Since the MARK action sets the hash.fdir.hi field only, +* both can be requested simultaneously. * * Terminating by default. */
Signature · 1 line
2.1.4
PATCH 5/5 ethdev: define default item masks in flow API
In reply to Adrien MazarguilLeaving default pattern item mask values up for interpretation by PMDs is an undefined behavior that applications might find difficult to use in the wild. It also needlessly complicates PMD implementation.
This commit addresses this by defining consistent default masks for each item type.
--- doc/guides/prog_guide/rte_flow.rst | 25 ++++++-- lib/librte_ether/rte_flow.h | 110 +++++++++++++++++++++++++++++--- 2 files changed, 121 insertions(+), 14 deletions(-)
doc/guides/prog_guide/rte_flow.rst +19 -6
@@ -217,13 +217,11 @@ Usage restrictions and expected behavior: values lower than those in ``spec`` are not supported. - Setting ``spec`` and optionally ``last`` without ``mask`` causes the PMD - to only take the fields it can recognize into account. There is no error - checking for unsupported fields. + to use the default mask defined for that item (defined as + ``rte_flow_item_{name}_mask`` constants). -- Not setting any of them (assuming item type allows it) uses default - parameters that depend on the item type. Most of the time, particularly - for protocol header items, it is equivalent to providing an empty (zeroed) - ``mask``. +- Not setting any of them (assuming item type allows it) is equivalent to + providing an empty (zeroed) ``mask`` for broad (nonspecific) matching. - ``mask`` is a simple bit-mask applied before interpreting the contents of ``spec`` and ``last``, which may yield unexpected results if not used
@@ -550,6 +548,7 @@ duplicated between device instances by default. - Can be specified multiple times to match traffic addressed to several VF IDs. - Can be combined with a PF item to match both PF and VF traffic. +- Default ``mask`` matches any VF ID. .. _table_rte_flow_item_vf:
@@ -583,6 +582,8 @@ not be contiguous. As a device property, the list of allowed values as well as the value associated with a port_id should be retrieved by other means. +- Default ``mask`` matches any port index. + .. _table_rte_flow_item_port: .. table:: PORT
@@ -616,6 +617,8 @@ stand for several protocol layers. This is usually specified as the first pattern item when looking for a protocol anywhere in a packet. +- Default ``mask`` stands for any number of layers. + .. _table_rte_flow_item_any: .. table:: ANY
@@ -673,6 +676,7 @@ Matching a zero-length pattern is allowed, doing so resets the relative offset for subsequent items. - This type does not support ranges (``last`` field). +- Default ``mask`` matches all fields exactly. .. _table_rte_flow_item_raw:
@@ -785,6 +789,7 @@ Matches an Ethernet header. - ``dst``: destination MAC. - ``src``: source MAC. - ``type``: EtherType. +- Default ``mask`` matches destination and source addresses only. Item: ``VLAN`` ^^^^^^^^^^^^^^
@@ -793,6 +798,7 @@ Matches an 802.1Q/ad VLAN tag. - ``tpid``: tag protocol identifier. - ``tci``: tag control information. +- Default ``mask`` matches TCI only. Item: ``IPV4`` ^^^^^^^^^^^^^^
@@ -802,6 +808,7 @@ Matches an IPv4 header. Note: IPv4 options are handled by dedicated pattern items. - ``hdr``: IPv4 header definition (``rte_ip.h``). +- Default ``mask`` matches source and destination addresses only. Item: ``IPV6`` ^^^^^^^^^^^^^^
@@ -811,6 +818,7 @@ Matches an IPv6 header. Note: IPv6 options are handled by dedicated pattern items. - ``hdr``: IPv6 header definition (``rte_ip.h``). +- Default ``mask`` matches source and destination addresses only. Item: ``ICMP`` ^^^^^^^^^^^^^^
@@ -818,6 +826,7 @@ Item: ``ICMP`` Matches an ICMP header. - ``hdr``: ICMP header definition (``rte_icmp.h``). +- Default ``mask`` matches ICMP type and code only. Item: ``UDP`` ^^^^^^^^^^^^^
@@ -825,6 +834,7 @@ Item: ``UDP`` Matches a UDP header. - ``hdr``: UDP header definition (``rte_udp.h``). +- Default ``mask`` matches source and destination ports only. Item: ``TCP`` ^^^^^^^^^^^^^
@@ -832,6 +842,7 @@ Item: ``TCP`` Matches a TCP header. - ``hdr``: TCP header definition (``rte_tcp.h``). +- Default ``mask`` matches source and destination ports only. Item: ``SCTP`` ^^^^^^^^^^^^^^
@@ -839,6 +850,7 @@ Item: ``SCTP`` Matches a SCTP header. - ``hdr``: SCTP header definition (``rte_sctp.h``). +- Default ``mask`` matches source and destination ports only. Item: ``VXLAN`` ^^^^^^^^^^^^^^^
@@ -849,6 +861,7 @@ Matches a VXLAN header (RFC 7348). - ``rsvd0``: reserved, normally 0x000000. - ``vni``: VXLAN network identifier. - ``rsvd1``: reserved, normally 0x00. +- Default ``mask`` matches VNI only. Actions ~~~~~~~
lib/librte_ether/rte_flow.h +102 -8
206 lines not on this page · raw message
Signature · 1 line
2.1.4
Thomas Monjalon
Applied2017-01-10 14:08, Adrien Mazarguil:
Quoted 10 lines
This series makes a few adjustments to the flow API (
rte_flow) based on issues encountered by preliminary PMD implementations and addresses bugs reported by Coverity in the testpmd flow command.Adrien Mazarguil (5):
app/testpmd: fix array bounds checks
ethdev: modify flow API's error setting function
ethdev: clarify MARK and FLAG actions in flow API
ethdev: clarify RSS action in flow API
ethdev: define default item masks in flow API
Applied, thanks