Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 7/14/22 19:03, Roi Dayan wrote:
>
>
> On 2022-07-14 4:18 PM, Ilya Maximets wrote:
>> On 7/14/22 15:13, Roi Dayan wrote:
>>>
>>>
>>> On 2022-07-14 4:01 PM, Roi Dayan wrote:
On 2022-07-14 3:52 PM, Roi Dayan wrote:
>
>
> On 2022-07-14 3:37 PM, Roi Dayan wrote:
>>
>>
>> On 2022-07-13 7:28 PM, Ilya Maximets wrote:
>>> On 7/13/22 12:11, Roi Dayan wrote:
On 2022-07-13 12:01 PM, Roi Dayan wrote:
>
>
> On 2022-07-06 4:58 PM, Roi Dayan wrote:
>>
>>
>> On 2022-07-05 1:45 AM, Ilya Maximets wrote:
>>> Current offloading code supports only limited number of tunnel keys
>>> and silently ignores everything it doesn't understand. This is
>>> causing, for example, offloaded ERSPAN tunnels to not work, because
>>> flow is offloaded, but ERSPAN options are not provided to TC.
>>>
>>> There is a number of tunnel keys, which are supported by the
>>> userspace,
>>> but silently ignored during offloading:
>>>
>>> OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
>>> OVS_TUNNEL_KEY_ATTR_OAM
>>> OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
>>> OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
>>>
>>> OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
>>> and for some reason is set from the tunnel port instead of the
>>> provided action, and not currently supported for the tunnel key in
>>> the match.
>>>
>>> Addig a default case to fail offloading of unknown attributes. For
>>> now explicitly allowing incorrect behavior for the DONT_FRAGMENT
>>> flag,
>>> otherwise we'll break all tunnel offloading by default. VXLAN and
>>> ERSPAN options has to fail offloading, because the tunnel will not
>>> work otherwise. OAM is not a default configurations, so failing it
>>> as well. The missing DONT_FRAGMENT flag though should, probably,
>>> cause frequent flow revalidation, but that is not new with this
>>> patch.
>>>
>>> Same for the 'match' key, only clearing masks that was actually
>>> consumed, except for the DONT_FRAGMENT and CSUM flags, which are
>>> explicitly allowed and highlighted as broken.
>>>
>>> Also, ttl and tos were incorrectly checked by the value instead of a
>>> mask during the flow key dump. Destination port as well as CSUM
>>> configuration for unknown reason was not taken from the actions list
>>> and were passed via HW offload info.
>>>
>>> Reported-at:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-July%2F395522.htmldata=05%7C01%7Croid%40nvidia.com%7Cd5529f26731846561c5108da659b57a3%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637934015121728988%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=s9utolIUQDeF7SP0gwe6PucT%2FUzp99uHNpV3W9b0Z6M%3Dreserved=0
>>> Reported-by: Eelco Chaudron
>>> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put
>>> using tc interface")
>>> Signed-off-by: Ilya Maximets
>>> ---
>>> lib/dpif-netlink.c | 14 +-
>>> lib/netdev-offload-tc.c | 57
>>> ++---
>>> lib/netdev-offload.h | 3 ---
>>> 3 files changed, 49 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>>> index 06e1e8ca0..1e116b4ad 100644
>>> --- a/lib/dpif-netlink.c
>>> +++ b/lib/dpif-netlink.c
>>> @@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif,
>>> struct dpif_flow_put *put)
>>> size_t left;
>>> struct netdev *dev;
>>> struct offload_info info;
>>> - ovs_be16 dst_port = 0;
>>> - uint8_t csum_on = false;
>>> int err;
>>> info.tc_modify_flow_deleted = false;
>>> @@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif,
>>> struct dpif_flow_put *put)
>>> return EOPNOTSUPP;
>>> }
>>> - /* Get tunnel dst port */
>>> + /* Check the output port for a tunnel. */
>>> NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len)
>>> {
>>> if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
>>> - const struct netdev_tunnel_config *tnl_cfg;
>>> struct netdev *outdev;
>>> odp_port_t out_port;
>>> @@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif,
>>> struct dpif_flow_put *put)
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-07-14 4:18 PM, Ilya Maximets wrote:
On 7/14/22 15:13, Roi Dayan wrote:
On 2022-07-14 4:01 PM, Roi Dayan wrote:
On 2022-07-14 3:52 PM, Roi Dayan wrote:
On 2022-07-14 3:37 PM, Roi Dayan wrote:
On 2022-07-13 7:28 PM, Ilya Maximets wrote:
On 7/13/22 12:11, Roi Dayan wrote:
On 2022-07-13 12:01 PM, Roi Dayan wrote:
On 2022-07-06 4:58 PM, Roi Dayan wrote:
On 2022-07-05 1:45 AM, Ilya Maximets wrote:
Current offloading code supports only limited number of tunnel keys
and silently ignores everything it doesn't understand. This is
causing, for example, offloaded ERSPAN tunnels to not work, because
flow is offloaded, but ERSPAN options are not provided to TC.
There is a number of tunnel keys, which are supported by the userspace,
but silently ignored during offloading:
OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
OVS_TUNNEL_KEY_ATTR_OAM
OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
and for some reason is set from the tunnel port instead of the
provided action, and not currently supported for the tunnel key in
the match.
Addig a default case to fail offloading of unknown attributes. For
now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag,
otherwise we'll break all tunnel offloading by default. VXLAN and
ERSPAN options has to fail offloading, because the tunnel will not
work otherwise. OAM is not a default configurations, so failing it
as well. The missing DONT_FRAGMENT flag though should, probably,
cause frequent flow revalidation, but that is not new with this patch.
Same for the 'match' key, only clearing masks that was actually
consumed, except for the DONT_FRAGMENT and CSUM flags, which are
explicitly allowed and highlighted as broken.
Also, ttl and tos were incorrectly checked by the value instead of a
mask during the flow key dump. Destination port as well as CSUM
configuration for unknown reason was not taken from the actions list
and were passed via HW offload info.
Reported-at:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-July%2F395522.htmldata=05%7C01%7Croid%40nvidia.com%7Cd5529f26731846561c5108da659b57a3%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637934015121728988%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=s9utolIUQDeF7SP0gwe6PucT%2FUzp99uHNpV3W9b0Z6M%3Dreserved=0
Reported-by: Eelco Chaudron
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc
interface")
Signed-off-by: Ilya Maximets
---
lib/dpif-netlink.c | 14 +-
lib/netdev-offload-tc.c | 57 ++---
lib/netdev-offload.h | 3 ---
3 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 06e1e8ca0..1e116b4ad 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
size_t left;
struct netdev *dev;
struct offload_info info;
- ovs_be16 dst_port = 0;
- uint8_t csum_on = false;
int err;
info.tc_modify_flow_deleted = false;
@@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
return EOPNOTSUPP;
}
- /* Get tunnel dst port */
+ /* Check the output port for a tunnel. */
NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
- const struct netdev_tunnel_config *tnl_cfg;
struct netdev *outdev;
odp_port_t out_port;
@@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
err = EOPNOTSUPP;
goto out;
}
- tnl_cfg = netdev_get_tunnel_config(outdev);
- if (tnl_cfg && tnl_cfg->dst_port != 0) {
- dst_port = tnl_cfg->dst_port;
- }
- if (tnl_cfg) {
- csum_on = tnl_cfg->csum;
- }
netdev_close(outdev);
}
}
- info.tp_dst_port = dst_port;
- info.tunnel_csum_on = csum_on;
info.recirc_id_shared_with_tc = (dpif->user_features
& OVS_DP_F_TC_RECIRC_SHARING);
err = netdev_flow_put(dev, ,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 262faf3c6..e62687c82 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -805,11 +805,11 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>key.tunnel.ipv6.ipv6_src,
>mask.tunnel.ipv6.ipv6_src);
}
- if (flower->key.tunnel.tos) {
+ if (flower->mask.tunnel.tos) {
match_set_tun_tos_masked(match, flower->key.tunnel.tos,
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 7/14/22 15:13, Roi Dayan wrote:
>
>
> On 2022-07-14 4:01 PM, Roi Dayan wrote:
>>
>>
>> On 2022-07-14 3:52 PM, Roi Dayan wrote:
>>>
>>>
>>> On 2022-07-14 3:37 PM, Roi Dayan wrote:
On 2022-07-13 7:28 PM, Ilya Maximets wrote:
> On 7/13/22 12:11, Roi Dayan wrote:
>>
>>
>> On 2022-07-13 12:01 PM, Roi Dayan wrote:
>>>
>>>
>>> On 2022-07-06 4:58 PM, Roi Dayan wrote:
On 2022-07-05 1:45 AM, Ilya Maximets wrote:
> Current offloading code supports only limited number of tunnel keys
> and silently ignores everything it doesn't understand. This is
> causing, for example, offloaded ERSPAN tunnels to not work, because
> flow is offloaded, but ERSPAN options are not provided to TC.
>
> There is a number of tunnel keys, which are supported by the
> userspace,
> but silently ignored during offloading:
>
> OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
> OVS_TUNNEL_KEY_ATTR_OAM
> OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
> OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
>
> OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
> and for some reason is set from the tunnel port instead of the
> provided action, and not currently supported for the tunnel key in
> the match.
>
> Addig a default case to fail offloading of unknown attributes. For
> now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag,
> otherwise we'll break all tunnel offloading by default. VXLAN and
> ERSPAN options has to fail offloading, because the tunnel will not
> work otherwise. OAM is not a default configurations, so failing it
> as well. The missing DONT_FRAGMENT flag though should, probably,
> cause frequent flow revalidation, but that is not new with this patch.
>
> Same for the 'match' key, only clearing masks that was actually
> consumed, except for the DONT_FRAGMENT and CSUM flags, which are
> explicitly allowed and highlighted as broken.
>
> Also, ttl and tos were incorrectly checked by the value instead of a
> mask during the flow key dump. Destination port as well as CSUM
> configuration for unknown reason was not taken from the actions list
> and were passed via HW offload info.
>
> Reported-at:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-July%2F395522.htmldata=05%7C01%7Croid%40nvidia.com%7Cba6998f8523f4fcf52eb08da64ecba35%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637933265159187181%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=QDm8f%2FFzZE0X7q26i09UI8zaGK0UkD9tcEChI2XYNZ4%3Dreserved=0
> Reported-by: Eelco Chaudron
> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put
> using tc interface")
> Signed-off-by: Ilya Maximets
> ---
> lib/dpif-netlink.c | 14 +-
> lib/netdev-offload-tc.c | 57
> ++---
> lib/netdev-offload.h | 3 ---
> 3 files changed, 49 insertions(+), 25 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 06e1e8ca0..1e116b4ad 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif,
> struct dpif_flow_put *put)
> size_t left;
> struct netdev *dev;
> struct offload_info info;
> - ovs_be16 dst_port = 0;
> - uint8_t csum_on = false;
> int err;
> info.tc_modify_flow_deleted = false;
> @@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif,
> struct dpif_flow_put *put)
> return EOPNOTSUPP;
> }
> - /* Get tunnel dst port */
> + /* Check the output port for a tunnel. */
> NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
> if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
> - const struct netdev_tunnel_config *tnl_cfg;
> struct netdev *outdev;
> odp_port_t out_port;
> @@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif,
> struct dpif_flow_put *put)
> err = EOPNOTSUPP;
> goto out;
> }
> - tnl_cfg = netdev_get_tunnel_config(outdev);
> - if (tnl_cfg && tnl_cfg->dst_port != 0) {
> - dst_port = tnl_cfg->dst_port;
> - }
> -
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-07-14 4:01 PM, Roi Dayan wrote:
On 2022-07-14 3:52 PM, Roi Dayan wrote:
On 2022-07-14 3:37 PM, Roi Dayan wrote:
On 2022-07-13 7:28 PM, Ilya Maximets wrote:
On 7/13/22 12:11, Roi Dayan wrote:
On 2022-07-13 12:01 PM, Roi Dayan wrote:
On 2022-07-06 4:58 PM, Roi Dayan wrote:
On 2022-07-05 1:45 AM, Ilya Maximets wrote:
Current offloading code supports only limited number of tunnel keys
and silently ignores everything it doesn't understand. This is
causing, for example, offloaded ERSPAN tunnels to not work, because
flow is offloaded, but ERSPAN options are not provided to TC.
There is a number of tunnel keys, which are supported by the
userspace,
but silently ignored during offloading:
OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
OVS_TUNNEL_KEY_ATTR_OAM
OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
and for some reason is set from the tunnel port instead of the
provided action, and not currently supported for the tunnel key in
the match.
Addig a default case to fail offloading of unknown attributes. For
now explicitly allowing incorrect behavior for the DONT_FRAGMENT
flag,
otherwise we'll break all tunnel offloading by default. VXLAN and
ERSPAN options has to fail offloading, because the tunnel will not
work otherwise. OAM is not a default configurations, so failing it
as well. The missing DONT_FRAGMENT flag though should, probably,
cause frequent flow revalidation, but that is not new with this
patch.
Same for the 'match' key, only clearing masks that was actually
consumed, except for the DONT_FRAGMENT and CSUM flags, which are
explicitly allowed and highlighted as broken.
Also, ttl and tos were incorrectly checked by the value instead
of a
mask during the flow key dump. Destination port as well as CSUM
configuration for unknown reason was not taken from the actions
list
and were passed via HW offload info.
Reported-at:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-July%2F395522.htmldata=05%7C01%7Croid%40nvidia.com%7Cba6998f8523f4fcf52eb08da64ecba35%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637933265159187181%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=QDm8f%2FFzZE0X7q26i09UI8zaGK0UkD9tcEChI2XYNZ4%3Dreserved=0
Reported-by: Eelco Chaudron
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow
put using tc interface")
Signed-off-by: Ilya Maximets
---
lib/dpif-netlink.c | 14 +-
lib/netdev-offload-tc.c | 57
++---
lib/netdev-offload.h | 3 ---
3 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 06e1e8ca0..1e116b4ad 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
size_t left;
struct netdev *dev;
struct offload_info info;
- ovs_be16 dst_port = 0;
- uint8_t csum_on = false;
int err;
info.tc_modify_flow_deleted = false;
@@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
return EOPNOTSUPP;
}
- /* Get tunnel dst port */
+ /* Check the output port for a tunnel. */
NL_ATTR_FOR_EACH(nla, left, put->actions,
put->actions_len) {
if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
- const struct netdev_tunnel_config *tnl_cfg;
struct netdev *outdev;
odp_port_t out_port;
@@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink
*dpif, struct dpif_flow_put *put)
err = EOPNOTSUPP;
goto out;
}
- tnl_cfg = netdev_get_tunnel_config(outdev);
- if (tnl_cfg && tnl_cfg->dst_port != 0) {
- dst_port = tnl_cfg->dst_port;
- }
- if (tnl_cfg) {
- csum_on = tnl_cfg->csum;
- }
netdev_close(outdev);
}
}
- info.tp_dst_port = dst_port;
- info.tunnel_csum_on = csum_on;
info.recirc_id_shared_with_tc = (dpif->user_features
&
OVS_DP_F_TC_RECIRC_SHARING);
err = netdev_flow_put(dev, ,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 262faf3c6..e62687c82 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -805,11 +805,11 @@ parse_tc_flower_to_match(struct tc_flower
*flower,
>key.tunnel.ipv6.ipv6_src,
>mask.tunnel.ipv6.ipv6_src);
}
- if (flower->key.tunnel.tos) {
+ if (flower->mask.tunnel.tos) {
match_set_tun_tos_masked(match,
flower->key.tunnel.tos,
flower->mask.tunnel.tos);
}
- if (flower->key.tunnel.ttl) {
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-07-14 3:52 PM, Roi Dayan wrote:
On 2022-07-14 3:37 PM, Roi Dayan wrote:
On 2022-07-13 7:28 PM, Ilya Maximets wrote:
On 7/13/22 12:11, Roi Dayan wrote:
On 2022-07-13 12:01 PM, Roi Dayan wrote:
On 2022-07-06 4:58 PM, Roi Dayan wrote:
On 2022-07-05 1:45 AM, Ilya Maximets wrote:
Current offloading code supports only limited number of tunnel keys
and silently ignores everything it doesn't understand. This is
causing, for example, offloaded ERSPAN tunnels to not work, because
flow is offloaded, but ERSPAN options are not provided to TC.
There is a number of tunnel keys, which are supported by the
userspace,
but silently ignored during offloading:
OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
OVS_TUNNEL_KEY_ATTR_OAM
OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
and for some reason is set from the tunnel port instead of the
provided action, and not currently supported for the tunnel key in
the match.
Addig a default case to fail offloading of unknown attributes. For
now explicitly allowing incorrect behavior for the DONT_FRAGMENT
flag,
otherwise we'll break all tunnel offloading by default. VXLAN and
ERSPAN options has to fail offloading, because the tunnel will not
work otherwise. OAM is not a default configurations, so failing it
as well. The missing DONT_FRAGMENT flag though should, probably,
cause frequent flow revalidation, but that is not new with this
patch.
Same for the 'match' key, only clearing masks that was actually
consumed, except for the DONT_FRAGMENT and CSUM flags, which are
explicitly allowed and highlighted as broken.
Also, ttl and tos were incorrectly checked by the value instead of a
mask during the flow key dump. Destination port as well as CSUM
configuration for unknown reason was not taken from the actions list
and were passed via HW offload info.
Reported-at:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-July%2F395522.htmldata=05%7C01%7Croid%40nvidia.com%7Cba6998f8523f4fcf52eb08da64ecba35%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637933265159187181%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=QDm8f%2FFzZE0X7q26i09UI8zaGK0UkD9tcEChI2XYNZ4%3Dreserved=0
Reported-by: Eelco Chaudron
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow
put using tc interface")
Signed-off-by: Ilya Maximets
---
lib/dpif-netlink.c | 14 +-
lib/netdev-offload-tc.c | 57
++---
lib/netdev-offload.h | 3 ---
3 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 06e1e8ca0..1e116b4ad 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
size_t left;
struct netdev *dev;
struct offload_info info;
- ovs_be16 dst_port = 0;
- uint8_t csum_on = false;
int err;
info.tc_modify_flow_deleted = false;
@@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
return EOPNOTSUPP;
}
- /* Get tunnel dst port */
+ /* Check the output port for a tunnel. */
NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
- const struct netdev_tunnel_config *tnl_cfg;
struct netdev *outdev;
odp_port_t out_port;
@@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
err = EOPNOTSUPP;
goto out;
}
- tnl_cfg = netdev_get_tunnel_config(outdev);
- if (tnl_cfg && tnl_cfg->dst_port != 0) {
- dst_port = tnl_cfg->dst_port;
- }
- if (tnl_cfg) {
- csum_on = tnl_cfg->csum;
- }
netdev_close(outdev);
}
}
- info.tp_dst_port = dst_port;
- info.tunnel_csum_on = csum_on;
info.recirc_id_shared_with_tc = (dpif->user_features
&
OVS_DP_F_TC_RECIRC_SHARING);
err = netdev_flow_put(dev, ,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 262faf3c6..e62687c82 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -805,11 +805,11 @@ parse_tc_flower_to_match(struct tc_flower
*flower,
>key.tunnel.ipv6.ipv6_src,
>mask.tunnel.ipv6.ipv6_src);
}
- if (flower->key.tunnel.tos) {
+ if (flower->mask.tunnel.tos) {
match_set_tun_tos_masked(match,
flower->key.tunnel.tos,
flower->mask.tunnel.tos);
}
- if (flower->key.tunnel.ttl) {
+ if (flower->mask.tunnel.ttl) {
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-07-14 3:37 PM, Roi Dayan wrote:
On 2022-07-13 7:28 PM, Ilya Maximets wrote:
On 7/13/22 12:11, Roi Dayan wrote:
On 2022-07-13 12:01 PM, Roi Dayan wrote:
On 2022-07-06 4:58 PM, Roi Dayan wrote:
On 2022-07-05 1:45 AM, Ilya Maximets wrote:
Current offloading code supports only limited number of tunnel keys
and silently ignores everything it doesn't understand. This is
causing, for example, offloaded ERSPAN tunnels to not work, because
flow is offloaded, but ERSPAN options are not provided to TC.
There is a number of tunnel keys, which are supported by the
userspace,
but silently ignored during offloading:
OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
OVS_TUNNEL_KEY_ATTR_OAM
OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
and for some reason is set from the tunnel port instead of the
provided action, and not currently supported for the tunnel key in
the match.
Addig a default case to fail offloading of unknown attributes. For
now explicitly allowing incorrect behavior for the DONT_FRAGMENT
flag,
otherwise we'll break all tunnel offloading by default. VXLAN and
ERSPAN options has to fail offloading, because the tunnel will not
work otherwise. OAM is not a default configurations, so failing it
as well. The missing DONT_FRAGMENT flag though should, probably,
cause frequent flow revalidation, but that is not new with this
patch.
Same for the 'match' key, only clearing masks that was actually
consumed, except for the DONT_FRAGMENT and CSUM flags, which are
explicitly allowed and highlighted as broken.
Also, ttl and tos were incorrectly checked by the value instead of a
mask during the flow key dump. Destination port as well as CSUM
configuration for unknown reason was not taken from the actions list
and were passed via HW offload info.
Reported-at:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-July%2F395522.htmldata=05%7C01%7Croid%40nvidia.com%7Cba6998f8523f4fcf52eb08da64ecba35%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637933265159187181%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=QDm8f%2FFzZE0X7q26i09UI8zaGK0UkD9tcEChI2XYNZ4%3Dreserved=0
Reported-by: Eelco Chaudron
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow
put using tc interface")
Signed-off-by: Ilya Maximets
---
lib/dpif-netlink.c | 14 +-
lib/netdev-offload-tc.c | 57
++---
lib/netdev-offload.h | 3 ---
3 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 06e1e8ca0..1e116b4ad 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
size_t left;
struct netdev *dev;
struct offload_info info;
- ovs_be16 dst_port = 0;
- uint8_t csum_on = false;
int err;
info.tc_modify_flow_deleted = false;
@@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
return EOPNOTSUPP;
}
- /* Get tunnel dst port */
+ /* Check the output port for a tunnel. */
NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
- const struct netdev_tunnel_config *tnl_cfg;
struct netdev *outdev;
odp_port_t out_port;
@@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
err = EOPNOTSUPP;
goto out;
}
- tnl_cfg = netdev_get_tunnel_config(outdev);
- if (tnl_cfg && tnl_cfg->dst_port != 0) {
- dst_port = tnl_cfg->dst_port;
- }
- if (tnl_cfg) {
- csum_on = tnl_cfg->csum;
- }
netdev_close(outdev);
}
}
- info.tp_dst_port = dst_port;
- info.tunnel_csum_on = csum_on;
info.recirc_id_shared_with_tc = (dpif->user_features
&
OVS_DP_F_TC_RECIRC_SHARING);
err = netdev_flow_put(dev, ,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 262faf3c6..e62687c82 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -805,11 +805,11 @@ parse_tc_flower_to_match(struct tc_flower
*flower,
>key.tunnel.ipv6.ipv6_src,
>mask.tunnel.ipv6.ipv6_src);
}
- if (flower->key.tunnel.tos) {
+ if (flower->mask.tunnel.tos) {
match_set_tun_tos_masked(match,
flower->key.tunnel.tos,
flower->mask.tunnel.tos);
}
- if (flower->key.tunnel.ttl) {
+ if (flower->mask.tunnel.ttl) {
match_set_tun_ttl_masked(match,
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-07-13 7:28 PM, Ilya Maximets wrote:
On 7/13/22 12:11, Roi Dayan wrote:
On 2022-07-13 12:01 PM, Roi Dayan wrote:
On 2022-07-06 4:58 PM, Roi Dayan wrote:
On 2022-07-05 1:45 AM, Ilya Maximets wrote:
Current offloading code supports only limited number of tunnel keys
and silently ignores everything it doesn't understand. This is
causing, for example, offloaded ERSPAN tunnels to not work, because
flow is offloaded, but ERSPAN options are not provided to TC.
There is a number of tunnel keys, which are supported by the userspace,
but silently ignored during offloading:
OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
OVS_TUNNEL_KEY_ATTR_OAM
OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
and for some reason is set from the tunnel port instead of the
provided action, and not currently supported for the tunnel key in
the match.
Addig a default case to fail offloading of unknown attributes. For
now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag,
otherwise we'll break all tunnel offloading by default. VXLAN and
ERSPAN options has to fail offloading, because the tunnel will not
work otherwise. OAM is not a default configurations, so failing it
as well. The missing DONT_FRAGMENT flag though should, probably,
cause frequent flow revalidation, but that is not new with this patch.
Same for the 'match' key, only clearing masks that was actually
consumed, except for the DONT_FRAGMENT and CSUM flags, which are
explicitly allowed and highlighted as broken.
Also, ttl and tos were incorrectly checked by the value instead of a
mask during the flow key dump. Destination port as well as CSUM
configuration for unknown reason was not taken from the actions list
and were passed via HW offload info.
Reported-at:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-July%2F395522.htmldata=05%7C01%7Croid%40nvidia.com%7Cba6998f8523f4fcf52eb08da64ecba35%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637933265159187181%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7Csdata=QDm8f%2FFzZE0X7q26i09UI8zaGK0UkD9tcEChI2XYNZ4%3Dreserved=0
Reported-by: Eelco Chaudron
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc
interface")
Signed-off-by: Ilya Maximets
---
lib/dpif-netlink.c | 14 +-
lib/netdev-offload-tc.c | 57 ++---
lib/netdev-offload.h | 3 ---
3 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 06e1e8ca0..1e116b4ad 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
size_t left;
struct netdev *dev;
struct offload_info info;
- ovs_be16 dst_port = 0;
- uint8_t csum_on = false;
int err;
info.tc_modify_flow_deleted = false;
@@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
return EOPNOTSUPP;
}
- /* Get tunnel dst port */
+ /* Check the output port for a tunnel. */
NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
- const struct netdev_tunnel_config *tnl_cfg;
struct netdev *outdev;
odp_port_t out_port;
@@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
err = EOPNOTSUPP;
goto out;
}
- tnl_cfg = netdev_get_tunnel_config(outdev);
- if (tnl_cfg && tnl_cfg->dst_port != 0) {
- dst_port = tnl_cfg->dst_port;
- }
- if (tnl_cfg) {
- csum_on = tnl_cfg->csum;
- }
netdev_close(outdev);
}
}
- info.tp_dst_port = dst_port;
- info.tunnel_csum_on = csum_on;
info.recirc_id_shared_with_tc = (dpif->user_features
& OVS_DP_F_TC_RECIRC_SHARING);
err = netdev_flow_put(dev, ,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 262faf3c6..e62687c82 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -805,11 +805,11 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>key.tunnel.ipv6.ipv6_src,
>mask.tunnel.ipv6.ipv6_src);
}
- if (flower->key.tunnel.tos) {
+ if (flower->mask.tunnel.tos) {
match_set_tun_tos_masked(match, flower->key.tunnel.tos,
flower->mask.tunnel.tos);
}
- if (flower->key.tunnel.ttl) {
+ if (flower->mask.tunnel.ttl) {
match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 7/13/22 12:11, Roi Dayan wrote:
>
>
> On 2022-07-13 12:01 PM, Roi Dayan wrote:
>>
>>
>> On 2022-07-06 4:58 PM, Roi Dayan wrote:
>>>
>>>
>>> On 2022-07-05 1:45 AM, Ilya Maximets wrote:
Current offloading code supports only limited number of tunnel keys
and silently ignores everything it doesn't understand. This is
causing, for example, offloaded ERSPAN tunnels to not work, because
flow is offloaded, but ERSPAN options are not provided to TC.
There is a number of tunnel keys, which are supported by the userspace,
but silently ignored during offloading:
OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
OVS_TUNNEL_KEY_ATTR_OAM
OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
and for some reason is set from the tunnel port instead of the
provided action, and not currently supported for the tunnel key in
the match.
Addig a default case to fail offloading of unknown attributes. For
now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag,
otherwise we'll break all tunnel offloading by default. VXLAN and
ERSPAN options has to fail offloading, because the tunnel will not
work otherwise. OAM is not a default configurations, so failing it
as well. The missing DONT_FRAGMENT flag though should, probably,
cause frequent flow revalidation, but that is not new with this patch.
Same for the 'match' key, only clearing masks that was actually
consumed, except for the DONT_FRAGMENT and CSUM flags, which are
explicitly allowed and highlighted as broken.
Also, ttl and tos were incorrectly checked by the value instead of a
mask during the flow key dump. Destination port as well as CSUM
configuration for unknown reason was not taken from the actions list
and were passed via HW offload info.
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395522.html
Reported-by: Eelco Chaudron
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using
tc interface")
Signed-off-by: Ilya Maximets
---
lib/dpif-netlink.c | 14 +-
lib/netdev-offload-tc.c | 57 ++---
lib/netdev-offload.h | 3 ---
3 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 06e1e8ca0..1e116b4ad 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
size_t left;
struct netdev *dev;
struct offload_info info;
- ovs_be16 dst_port = 0;
- uint8_t csum_on = false;
int err;
info.tc_modify_flow_deleted = false;
@@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
return EOPNOTSUPP;
}
- /* Get tunnel dst port */
+ /* Check the output port for a tunnel. */
NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
- const struct netdev_tunnel_config *tnl_cfg;
struct netdev *outdev;
odp_port_t out_port;
@@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
err = EOPNOTSUPP;
goto out;
}
- tnl_cfg = netdev_get_tunnel_config(outdev);
- if (tnl_cfg && tnl_cfg->dst_port != 0) {
- dst_port = tnl_cfg->dst_port;
- }
- if (tnl_cfg) {
- csum_on = tnl_cfg->csum;
- }
netdev_close(outdev);
}
}
- info.tp_dst_port = dst_port;
- info.tunnel_csum_on = csum_on;
info.recirc_id_shared_with_tc = (dpif->user_features
& OVS_DP_F_TC_RECIRC_SHARING);
err = netdev_flow_put(dev, ,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 262faf3c6..e62687c82 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -805,11 +805,11 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>key.tunnel.ipv6.ipv6_src,
>mask.tunnel.ipv6.ipv6_src);
}
- if (flower->key.tunnel.tos) {
+ if (flower->mask.tunnel.tos) {
match_set_tun_tos_masked(match, flower->key.tunnel.tos,
flower->mask.tunnel.tos);
}
- if (flower->key.tunnel.ttl) {
+ if (flower->mask.tunnel.ttl) {
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-07-13 12:01 PM, Roi Dayan wrote:
On 2022-07-06 4:58 PM, Roi Dayan wrote:
On 2022-07-05 1:45 AM, Ilya Maximets wrote:
Current offloading code supports only limited number of tunnel keys
and silently ignores everything it doesn't understand. This is
causing, for example, offloaded ERSPAN tunnels to not work, because
flow is offloaded, but ERSPAN options are not provided to TC.
There is a number of tunnel keys, which are supported by the userspace,
but silently ignored during offloading:
OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
OVS_TUNNEL_KEY_ATTR_OAM
OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
and for some reason is set from the tunnel port instead of the
provided action, and not currently supported for the tunnel key in
the match.
Addig a default case to fail offloading of unknown attributes. For
now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag,
otherwise we'll break all tunnel offloading by default. VXLAN and
ERSPAN options has to fail offloading, because the tunnel will not
work otherwise. OAM is not a default configurations, so failing it
as well. The missing DONT_FRAGMENT flag though should, probably,
cause frequent flow revalidation, but that is not new with this patch.
Same for the 'match' key, only clearing masks that was actually
consumed, except for the DONT_FRAGMENT and CSUM flags, which are
explicitly allowed and highlighted as broken.
Also, ttl and tos were incorrectly checked by the value instead of a
mask during the flow key dump. Destination port as well as CSUM
configuration for unknown reason was not taken from the actions list
and were passed via HW offload info.
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395522.html
Reported-by: Eelco Chaudron
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put
using tc interface")
Signed-off-by: Ilya Maximets
---
lib/dpif-netlink.c | 14 +-
lib/netdev-offload-tc.c | 57 ++---
lib/netdev-offload.h | 3 ---
3 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 06e1e8ca0..1e116b4ad 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
size_t left;
struct netdev *dev;
struct offload_info info;
- ovs_be16 dst_port = 0;
- uint8_t csum_on = false;
int err;
info.tc_modify_flow_deleted = false;
@@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
return EOPNOTSUPP;
}
- /* Get tunnel dst port */
+ /* Check the output port for a tunnel. */
NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
- const struct netdev_tunnel_config *tnl_cfg;
struct netdev *outdev;
odp_port_t out_port;
@@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
err = EOPNOTSUPP;
goto out;
}
- tnl_cfg = netdev_get_tunnel_config(outdev);
- if (tnl_cfg && tnl_cfg->dst_port != 0) {
- dst_port = tnl_cfg->dst_port;
- }
- if (tnl_cfg) {
- csum_on = tnl_cfg->csum;
- }
netdev_close(outdev);
}
}
- info.tp_dst_port = dst_port;
- info.tunnel_csum_on = csum_on;
info.recirc_id_shared_with_tc = (dpif->user_features
& OVS_DP_F_TC_RECIRC_SHARING);
err = netdev_flow_put(dev, ,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 262faf3c6..e62687c82 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -805,11 +805,11 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>key.tunnel.ipv6.ipv6_src,
>mask.tunnel.ipv6.ipv6_src);
}
- if (flower->key.tunnel.tos) {
+ if (flower->mask.tunnel.tos) {
match_set_tun_tos_masked(match, flower->key.tunnel.tos,
flower->mask.tunnel.tos);
}
- if (flower->key.tunnel.ttl) {
+ if (flower->mask.tunnel.ttl) {
match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
flower->mask.tunnel.ttl);
}
@@ -1284,9 +1284,11 @@ static int
parse_put_flow_set_action(struct tc_flower *flower, struct
tc_action *action,
const struct nlattr *set, size_t set_len)
{
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
const struct nlattr *tunnel;
const struct nlattr *tun_attr;
size_t tun_left, tunnel_len;
+ bool attr_csum_present;
if (nl_attr_type(set) == OVS_KEY_ATTR_MPLS)
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-07-06 4:58 PM, Roi Dayan wrote:
On 2022-07-05 1:45 AM, Ilya Maximets wrote:
Current offloading code supports only limited number of tunnel keys
and silently ignores everything it doesn't understand. This is
causing, for example, offloaded ERSPAN tunnels to not work, because
flow is offloaded, but ERSPAN options are not provided to TC.
There is a number of tunnel keys, which are supported by the userspace,
but silently ignored during offloading:
OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
OVS_TUNNEL_KEY_ATTR_OAM
OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
and for some reason is set from the tunnel port instead of the
provided action, and not currently supported for the tunnel key in
the match.
Addig a default case to fail offloading of unknown attributes. For
now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag,
otherwise we'll break all tunnel offloading by default. VXLAN and
ERSPAN options has to fail offloading, because the tunnel will not
work otherwise. OAM is not a default configurations, so failing it
as well. The missing DONT_FRAGMENT flag though should, probably,
cause frequent flow revalidation, but that is not new with this patch.
Same for the 'match' key, only clearing masks that was actually
consumed, except for the DONT_FRAGMENT and CSUM flags, which are
explicitly allowed and highlighted as broken.
Also, ttl and tos were incorrectly checked by the value instead of a
mask during the flow key dump. Destination port as well as CSUM
configuration for unknown reason was not taken from the actions list
and were passed via HW offload info.
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395522.html
Reported-by: Eelco Chaudron
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put
using tc interface")
Signed-off-by: Ilya Maximets
---
lib/dpif-netlink.c | 14 +-
lib/netdev-offload-tc.c | 57 ++---
lib/netdev-offload.h | 3 ---
3 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 06e1e8ca0..1e116b4ad 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
size_t left;
struct netdev *dev;
struct offload_info info;
- ovs_be16 dst_port = 0;
- uint8_t csum_on = false;
int err;
info.tc_modify_flow_deleted = false;
@@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
return EOPNOTSUPP;
}
- /* Get tunnel dst port */
+ /* Check the output port for a tunnel. */
NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
- const struct netdev_tunnel_config *tnl_cfg;
struct netdev *outdev;
odp_port_t out_port;
@@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif,
struct dpif_flow_put *put)
err = EOPNOTSUPP;
goto out;
}
- tnl_cfg = netdev_get_tunnel_config(outdev);
- if (tnl_cfg && tnl_cfg->dst_port != 0) {
- dst_port = tnl_cfg->dst_port;
- }
- if (tnl_cfg) {
- csum_on = tnl_cfg->csum;
- }
netdev_close(outdev);
}
}
- info.tp_dst_port = dst_port;
- info.tunnel_csum_on = csum_on;
info.recirc_id_shared_with_tc = (dpif->user_features
& OVS_DP_F_TC_RECIRC_SHARING);
err = netdev_flow_put(dev, ,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 262faf3c6..e62687c82 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -805,11 +805,11 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>key.tunnel.ipv6.ipv6_src,
>mask.tunnel.ipv6.ipv6_src);
}
- if (flower->key.tunnel.tos) {
+ if (flower->mask.tunnel.tos) {
match_set_tun_tos_masked(match, flower->key.tunnel.tos,
flower->mask.tunnel.tos);
}
- if (flower->key.tunnel.ttl) {
+ if (flower->mask.tunnel.ttl) {
match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
flower->mask.tunnel.ttl);
}
@@ -1284,9 +1284,11 @@ static int
parse_put_flow_set_action(struct tc_flower *flower, struct tc_action
*action,
const struct nlattr *set, size_t set_len)
{
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
const struct nlattr *tunnel;
const struct nlattr *tun_attr;
size_t tun_left, tunnel_len;
+ bool attr_csum_present;
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-07-05 1:45 AM, Ilya Maximets wrote:
Current offloading code supports only limited number of tunnel keys
and silently ignores everything it doesn't understand. This is
causing, for example, offloaded ERSPAN tunnels to not work, because
flow is offloaded, but ERSPAN options are not provided to TC.
There is a number of tunnel keys, which are supported by the userspace,
but silently ignored during offloading:
OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
OVS_TUNNEL_KEY_ATTR_OAM
OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
and for some reason is set from the tunnel port instead of the
provided action, and not currently supported for the tunnel key in
the match.
Addig a default case to fail offloading of unknown attributes. For
now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag,
otherwise we'll break all tunnel offloading by default. VXLAN and
ERSPAN options has to fail offloading, because the tunnel will not
work otherwise. OAM is not a default configurations, so failing it
as well. The missing DONT_FRAGMENT flag though should, probably,
cause frequent flow revalidation, but that is not new with this patch.
Same for the 'match' key, only clearing masks that was actually
consumed, except for the DONT_FRAGMENT and CSUM flags, which are
explicitly allowed and highlighted as broken.
Also, ttl and tos were incorrectly checked by the value instead of a
mask during the flow key dump. Destination port as well as CSUM
configuration for unknown reason was not taken from the actions list
and were passed via HW offload info.
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395522.html
Reported-by: Eelco Chaudron
Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc
interface")
Signed-off-by: Ilya Maximets
---
lib/dpif-netlink.c | 14 +-
lib/netdev-offload-tc.c | 57 ++---
lib/netdev-offload.h| 3 ---
3 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 06e1e8ca0..1e116b4ad 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
size_t left;
struct netdev *dev;
struct offload_info info;
-ovs_be16 dst_port = 0;
-uint8_t csum_on = false;
int err;
info.tc_modify_flow_deleted = false;
@@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct
dpif_flow_put *put)
return EOPNOTSUPP;
}
-/* Get tunnel dst port */
+/* Check the output port for a tunnel. */
NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) {
if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
-const struct netdev_tunnel_config *tnl_cfg;
struct netdev *outdev;
odp_port_t out_port;
@@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
err = EOPNOTSUPP;
goto out;
}
-tnl_cfg = netdev_get_tunnel_config(outdev);
-if (tnl_cfg && tnl_cfg->dst_port != 0) {
-dst_port = tnl_cfg->dst_port;
-}
-if (tnl_cfg) {
-csum_on = tnl_cfg->csum;
-}
netdev_close(outdev);
}
}
-info.tp_dst_port = dst_port;
-info.tunnel_csum_on = csum_on;
info.recirc_id_shared_with_tc = (dpif->user_features
& OVS_DP_F_TC_RECIRC_SHARING);
err = netdev_flow_put(dev, ,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 262faf3c6..e62687c82 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -805,11 +805,11 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>key.tunnel.ipv6.ipv6_src,
>mask.tunnel.ipv6.ipv6_src);
}
-if (flower->key.tunnel.tos) {
+if (flower->mask.tunnel.tos) {
match_set_tun_tos_masked(match, flower->key.tunnel.tos,
flower->mask.tunnel.tos);
}
-if (flower->key.tunnel.ttl) {
+if (flower->mask.tunnel.ttl) {
match_set_tun_ttl_masked(match, flower->key.tunnel.ttl,
flower->mask.tunnel.ttl);
}
@@ -1284,9 +1284,11 @@ static int
parse_put_flow_set_action(struct tc_flower *flower, struct tc_action *action,
const struct nlattr *set, size_t set_len)
{
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
const struct nlattr *tunnel;
const struct nlattr *tun_attr;
size_t tun_left, tunnel_len;
+bool attr_csum_present;
if (nl_attr_type(set) ==
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 5 Jul 2022, at 0:45, Ilya Maximets wrote:
> Current offloading code supports only limited number of tunnel keys
> and silently ignores everything it doesn't understand. This is
> causing, for example, offloaded ERSPAN tunnels to not work, because
> flow is offloaded, but ERSPAN options are not provided to TC.
>
> There is a number of tunnel keys, which are supported by the userspace,
> but silently ignored during offloading:
>
> OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT
> OVS_TUNNEL_KEY_ATTR_OAM
> OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS
> OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS
>
> OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions
> and for some reason is set from the tunnel port instead of the
> provided action, and not currently supported for the tunnel key in
> the match.
>
> Addig a default case to fail offloading of unknown attributes. For
> now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag,
> otherwise we'll break all tunnel offloading by default. VXLAN and
> ERSPAN options has to fail offloading, because the tunnel will not
> work otherwise. OAM is not a default configurations, so failing it
> as well. The missing DONT_FRAGMENT flag though should, probably,
> cause frequent flow revalidation, but that is not new with this patch.
>
> Same for the 'match' key, only clearing masks that was actually
> consumed, except for the DONT_FRAGMENT and CSUM flags, which are
> explicitly allowed and highlighted as broken.
>
> Also, ttl and tos were incorrectly checked by the value instead of a
> mask during the flow key dump. Destination port as well as CSUM
> configuration for unknown reason was not taken from the actions list
> and were passed via HW offload info.
>
> Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395522.html
> Reported-by: Eelco Chaudron
> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc
> interface")
> Signed-off-by: Ilya Maximets
Changes look good to me, and all system-traffic.at tests that were passing
without the change are still passing, including the failing ERSPAN ones.
Acked-by: Eelco Chaudron
Two small nits below.
> ---
> lib/dpif-netlink.c | 14 +-
> lib/netdev-offload-tc.c | 57 ++---
> lib/netdev-offload.h| 3 ---
> 3 files changed, 49 insertions(+), 25 deletions(-)
>
> +case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT: {
> +/* XXX: This is wrong! We're ignoring the DF flag configuration
> + * requested by the user. However, TC for now has no way to pass
> + * that flag and it is set by default, meaning tunnel offloading
> + * will not work if 'options:df_default=false' is not set.
> + * Keeping incorrect behavior for now. */
> +
> +/* XXX: This is wrong! We're ignoring DF and CSUM flags
> configuration
> + * requested by the user. However, TC for now has no way to pass
> + * these flags in a flower key and their masks are set by default,
> + * meaning tunnel offloading will not work at all if not cleared.
> + * Keeping incorrect behavior for now. */
> +tnl_mask->flags &= ~(FLOW_TNL_F_DONT_FRAGMENT | FLOW_TNL_F_CSUM);
> +
Small nit, 2x two spaces before “ However, TC for...”.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 1/2] netdev-offload-tc: Fix ignoring unknown tunnel keys.
Bleep bloop. Greetings Ilya Maximets, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Comment with 'xxx' marker #139 FILE: lib/netdev-offload-tc.c:1333: /* XXX: This is wrong! We're ignoring the DF flag configuration WARNING: Comment with 'xxx' marker #214 FILE: lib/netdev-offload-tc.c:1664: /* XXX: This is wrong! We're ignoring DF and CSUM flags configuration Lines checked: 256, Warnings: 2, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
