Re: [ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 8/5/22 13:00, Roi Dayan wrote:
>
>
> On 2022-08-04 6:20 PM, Ilya Maximets wrote:
>> On 8/2/22 09:13, Roi Dayan wrote:
>>>
>>>
>>> On 2022-08-02 9:53 AM, Roi Dayan wrote:
On 2022-08-01 9:31 AM, Roi Dayan wrote:
>
>
> On 2022-07-29 5:53 PM, 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, destination port as well as CSUM configuration for unknown
>> reason was not taken from the actions list and were passed via HW
>> offload info instead of being consumed from the set() action.
>>
>> Reported-at:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-July%2F395522.html&data=05%7C01%7Croid%40nvidia.com%7C0b0fe4ddff284d603b3008da762cd0e0%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637952232106012230%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=uhL1ll5%2FnI1e%2F9OrcSGVfECBHL4r%2B%2FRg1VIcoyy3zas%3D&reserved=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 | 61 -
>> lib/netdev-offload.h | 3 --
>> 3 files changed, 55 insertions(+), 23 deletions(-)
>>
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index a498d5667..d9243a735 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, &match,
>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-
Re: [ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-08-04 6:20 PM, Ilya Maximets wrote:
On 8/2/22 09:13, Roi Dayan wrote:
On 2022-08-02 9:53 AM, Roi Dayan wrote:
On 2022-08-01 9:31 AM, Roi Dayan wrote:
On 2022-07-29 5:53 PM, 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, destination port as well as CSUM configuration for unknown
reason was not taken from the actions list and were passed via HW
offload info instead of being consumed from the set() action.
Reported-at:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-July%2F395522.html&data=05%7C01%7Croid%40nvidia.com%7C0b0fe4ddff284d603b3008da762cd0e0%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637952232106012230%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=uhL1ll5%2FnI1e%2F9OrcSGVfECBHL4r%2B%2FRg1VIcoyy3zas%3D&reserved=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 | 61 -
lib/netdev-offload.h | 3 --
3 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a498d5667..d9243a735 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, &match,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index a3eee8df3..57385597a 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1389,9 +1389,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) {
return parse_mpls_set_action(flower, action, set);
@@ -1405,6 +1407,7 @@ parse_put_flow_set_action(struct tc_flower *flower,
struct tc
Re: [ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 8/4/22 16:57, Paul Blakey wrote:
> On 29/07/2022 17:53, Ilya Maximets wrote:
>> @@ -1431,6 +1434,19 @@ parse_put_flow_set_action(struct tc_flower *flower,
>> struct tc_action *action,
>> action->encap.ttl = nl_attr_get_u8(tun_attr);
>> }
>> break;
>> + 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. */
>> + }
>> + break;
>> + case OVS_TUNNEL_KEY_ATTR_CSUM: {
>> + attr_csum_present = true;
>> + action->encap.no_csum = !nl_attr_get_u8(tun_attr);>
> Hi, OVS_TUNNEL_KEY_ATTR_CSUM is a flag (zero size, not u8), so maybe you meant
> setting action->encap.no_csum = 0; instead
Hmm, you're right. Got confused with TCA_TUNNEL_KEY_NO_CSUM, I guess.
Will fix in v3. Thanks!
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 8/2/22 09:13, Roi Dayan wrote:
>
>
> On 2022-08-02 9:53 AM, Roi Dayan wrote:
>>
>>
>> On 2022-08-01 9:31 AM, Roi Dayan wrote:
>>>
>>>
>>> On 2022-07-29 5:53 PM, 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, destination port as well as CSUM configuration for unknown
reason was not taken from the actions list and were passed via HW
offload info instead of being consumed from the set() action.
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 | 61 -
lib/netdev-offload.h | 3 --
3 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a498d5667..d9243a735 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, &match,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index a3eee8df3..57385597a 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1389,9 +1389,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) {
return parse_mpls_set_action(flower, action, set);
>>>
Re: [ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 29/07/2022 17:53, 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, destination port as well as CSUM configuration for unknown
reason was not taken from the actions list and were passed via HW
offload info instead of being consumed from the set() action.
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 | 61 -
lib/netdev-offload.h| 3 --
3 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a498d5667..d9243a735 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, &match,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index a3eee8df3..57385597a 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1389,9 +1389,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) {
return parse_mpls_set_action(flower, action, set);
@@ -1405,6 +1407,7 @@ parse_put_flow_set_action(struct tc_flower *flower,
struct tc_action *action,
tunnel = nl_attr_get(set);
tunnel_len = nl_attr_get_size(set);
+attr_csum_present = false;
action->type = TC_ACT_ENCAP;
action->encap.id_present = false;
flower->action_count++;
@@ -1431,6 +1434,19 @@ parse_put_flow_set_action(struct tc_flower *flower,
struct tc_action *action,
action->encap.ttl = nl_attr_get_u8(tun_attr);
}
break;
+case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT: {
+/* XXX: This is wrong! We're ignoring the DF flag configuration
+
Re: [ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-08-02 9:53 AM, Roi Dayan wrote:
On 2022-08-01 9:31 AM, Roi Dayan wrote:
On 2022-07-29 5:53 PM, 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, destination port as well as CSUM configuration for unknown
reason was not taken from the actions list and were passed via HW
offload info instead of being consumed from the set() action.
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 | 61 -
lib/netdev-offload.h | 3 --
3 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a498d5667..d9243a735 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, &match,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index a3eee8df3..57385597a 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1389,9 +1389,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) {
return parse_mpls_set_action(flower, action, set);
@@ -1405,6 +1407,7 @@ parse_put_flow_set_action(struct tc_flower
*flower, struct tc_action *action,
tunnel = nl_attr_get(set);
tunnel_len = nl_attr_get_size(set);
+ attr_csum_present = false;
action->type = TC_ACT_ENCAP;
action->encap.id_present = false;
flower->action_count++;
@@ -1431,6 +1434,19 @@ parse_put_flow_set_action(struct tc_flower
*flower, struct tc_action *action,
action->encap.ttl = nl_attr_get_u8(tun_attr);
}
break;
+ case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT: {
+ /*
Re: [ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-08-01 9:31 AM, Roi Dayan wrote:
On 2022-07-29 5:53 PM, 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, destination port as well as CSUM configuration for unknown
reason was not taken from the actions list and were passed via HW
offload info instead of being consumed from the set() action.
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 | 61 -
lib/netdev-offload.h | 3 --
3 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a498d5667..d9243a735 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, &match,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index a3eee8df3..57385597a 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1389,9 +1389,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) {
return parse_mpls_set_action(flower, action, set);
@@ -1405,6 +1407,7 @@ parse_put_flow_set_action(struct tc_flower
*flower, struct tc_action *action,
tunnel = nl_attr_get(set);
tunnel_len = nl_attr_get_size(set);
+ attr_csum_present = false;
action->type = TC_ACT_ENCAP;
action->encap.id_present = false;
flower->action_count++;
@@ -1431,6 +1434,19 @@ parse_put_flow_set_action(struct tc_flower
*flower, struct tc_action *action,
action->encap.ttl = nl_attr_get_u8(tun_attr);
}
break;
+ case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT: {
+ /* XXX: This is wrong! We're ignoring the DF
Re: [ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
On 2022-07-29 5:53 PM, 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, destination port as well as CSUM configuration for unknown
reason was not taken from the actions list and were passed via HW
offload info instead of being consumed from the set() action.
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 | 61 -
lib/netdev-offload.h| 3 --
3 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a498d5667..d9243a735 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, &match,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index a3eee8df3..57385597a 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1389,9 +1389,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) {
return parse_mpls_set_action(flower, action, set);
@@ -1405,6 +1407,7 @@ parse_put_flow_set_action(struct tc_flower *flower,
struct tc_action *action,
tunnel = nl_attr_get(set);
tunnel_len = nl_attr_get_size(set);
+attr_csum_present = false;
action->type = TC_ACT_ENCAP;
action->encap.id_present = false;
flower->action_count++;
@@ -1431,6 +1434,19 @@ parse_put_flow_set_action(struct tc_flower *flower,
struct tc_action *action,
action->encap.ttl = nl_attr_get_u8(tun_attr);
}
break;
+case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT: {
+/* XXX: This is wrong! We're ignoring the DF flag configuration
+
Re: [ovs-dev] [PATCH v2 4/5] 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 #124 FILE: lib/netdev-offload-tc.c:1438: /* XXX: This is wrong! We're ignoring the DF flag configuration WARNING: Comment with 'xxx' marker #221 FILE: lib/netdev-offload-tc.c:2036: /* 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
[ovs-dev] [PATCH v2 4/5] netdev-offload-tc: Fix ignoring unknown tunnel keys.
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, destination port as well as CSUM configuration for unknown
reason was not taken from the actions list and were passed via HW
offload info instead of being consumed from the set() action.
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 | 61 -
lib/netdev-offload.h| 3 --
3 files changed, 55 insertions(+), 23 deletions(-)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index a498d5667..d9243a735 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, &match,
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index a3eee8df3..57385597a 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -1389,9 +1389,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) {
return parse_mpls_set_action(flower, action, set);
@@ -1405,6 +1407,7 @@ parse_put_flow_set_action(struct tc_flower *flower,
struct tc_action *action,
tunnel = nl_attr_get(set);
tunnel_len = nl_attr_get_size(set);
+attr_csum_present = false;
action->type = TC_ACT_ENCAP;
action->encap.id_present = false;
flower->action_count++;
@@ -1431,6 +1434,19 @@ parse_put_flow_set_action(struct tc_flower *flower,
struct tc_action *action,
action->encap.ttl = nl_attr_get_u8(tun_attr);
}
break;
+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
