| Message ID | 20220729145319.3228334-2-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tc: Fixes for tunnel offloading. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 2022-07-29 5:53 PM, Ilya Maximets wrote: > 'wc.masks.tunnel.metadata.present.len' must be a mask for the same > field in the flow key, but in the tc_flower structure it's the real > length of metadata masks. > > That is correctly handled for the individual opt->length, setting all > the masks to 0x1f like it's done in the tun_metadata_to_geneve_mask__(), > but not handled for the main 'len' field. > > Fix that by setting the mask to 0xff like it's done during the flow > translation in xlate_actions() and during the flow dump in the > tun_metadata_from_geneve_nlattr(). > > Extra checks and comments added around the code to better explain > what is going on. > > Fixes: a468645c6d33 ("lib/tc: add geneve with option match offload") > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > lib/netdev-offload-tc.c | 7 +++++-- > lib/tc.c | 13 ++++++++++--- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 29d0e63eb..750cc5682 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -606,8 +606,9 @@ flower_tun_opt_to_match(struct match *match, struct tc_flower *flower) > len -= sizeof(struct geneve_opt) + opt->length * 4; > } > > - match->wc.masks.tunnel.metadata.present.len = > - flower->mask.tunnel.metadata.present.len; > + /* In the 'flower' mask len is an actual length, not the mask. > + * But in 'match' it is an actual mask and should be an exact match. */ > + match->wc.masks.tunnel.metadata.present.len = 0xff; > match->wc.masks.tunnel.flags |= FLOW_TNL_F_UDPIF; > } > > @@ -1574,6 +1575,8 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl, > len -= sizeof(struct geneve_opt) + opt->length * 4; > } > > + /* Copying from the key and not from the mask, since in the 'flower' > + * the length for a mask is not a mask, but the actual length. */ > flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len; > } > > diff --git a/lib/tc.c b/lib/tc.c > index 25e66b5c5..0f3f27c11 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -721,15 +721,17 @@ flower_tun_geneve_opt_check_len(struct tun_metadata *key, > const struct geneve_opt *opt, *opt_mask; > int len, cnt = 0; > > + if (key->present.len != mask->present.len) { > + goto bad_length; > + } > + > len = key->present.len; > while (len) { > opt = &key->opts.gnv[cnt]; > opt_mask = &mask->opts.gnv[cnt]; > > if (opt->length != opt_mask->length) { > - VLOG_ERR_RL(&error_rl, > - "failed to parse tun options; key/mask length differ"); > - return EINVAL; > + goto bad_length; > } > > cnt += sizeof(struct geneve_opt) / 4 + opt->length; > @@ -737,6 +739,11 @@ flower_tun_geneve_opt_check_len(struct tun_metadata *key, > } > > return 0; > + > +bad_length: > + VLOG_ERR_RL(&error_rl, > + "failed to parse tun options; key/mask length differ"); > + return EINVAL; > } > > static int reviewed and tested the series with some tunnel traffic. same test I did on prev patchset. thanks Reviewed-by: Roi Dayan <roid@nvidia.com>
On Fri, Jul 29, 2022 at 04:53:15PM +0200, Ilya Maximets wrote: > @@ -606,8 +606,9 @@ flower_tun_opt_to_match(struct match *match, struct tc_flower *flower) > len -= sizeof(struct geneve_opt) + opt->length * 4; > } > > - match->wc.masks.tunnel.metadata.present.len = > - flower->mask.tunnel.metadata.present.len; > + /* In the 'flower' mask len is an actual length, not the mask. > + * But in 'match' it is an actual mask and should be an exact match. */ > + match->wc.masks.tunnel.metadata.present.len = 0xff; > match->wc.masks.tunnel.flags |= FLOW_TNL_F_UDPIF; > } I (a bit briefly) reviewed the pathset now. Overall it LGTM but I won't add a reviewed-by then. Anyhow, it would be nice if this difference was even clearer. Otherwise, it is very possible that this will be hit again in the future. It is almost impossible to catch this on code reviews later on if not deep in the context. Simple field renaming is not an option, because both are using struct tun_metadata. Cheers, Marcelo
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 29d0e63eb..750cc5682 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -606,8 +606,9 @@ flower_tun_opt_to_match(struct match *match, struct tc_flower *flower) len -= sizeof(struct geneve_opt) + opt->length * 4; } - match->wc.masks.tunnel.metadata.present.len = - flower->mask.tunnel.metadata.present.len; + /* In the 'flower' mask len is an actual length, not the mask. + * But in 'match' it is an actual mask and should be an exact match. */ + match->wc.masks.tunnel.metadata.present.len = 0xff; match->wc.masks.tunnel.flags |= FLOW_TNL_F_UDPIF; } @@ -1574,6 +1575,8 @@ flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl, len -= sizeof(struct geneve_opt) + opt->length * 4; } + /* Copying from the key and not from the mask, since in the 'flower' + * the length for a mask is not a mask, but the actual length. */ flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len; } diff --git a/lib/tc.c b/lib/tc.c index 25e66b5c5..0f3f27c11 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -721,15 +721,17 @@ flower_tun_geneve_opt_check_len(struct tun_metadata *key, const struct geneve_opt *opt, *opt_mask; int len, cnt = 0; + if (key->present.len != mask->present.len) { + goto bad_length; + } + len = key->present.len; while (len) { opt = &key->opts.gnv[cnt]; opt_mask = &mask->opts.gnv[cnt]; if (opt->length != opt_mask->length) { - VLOG_ERR_RL(&error_rl, - "failed to parse tun options; key/mask length differ"); - return EINVAL; + goto bad_length; } cnt += sizeof(struct geneve_opt) / 4 + opt->length; @@ -737,6 +739,11 @@ flower_tun_geneve_opt_check_len(struct tun_metadata *key, } return 0; + +bad_length: + VLOG_ERR_RL(&error_rl, + "failed to parse tun options; key/mask length differ"); + return EINVAL; } static int
'wc.masks.tunnel.metadata.present.len' must be a mask for the same field in the flow key, but in the tc_flower structure it's the real length of metadata masks. That is correctly handled for the individual opt->length, setting all the masks to 0x1f like it's done in the tun_metadata_to_geneve_mask__(), but not handled for the main 'len' field. Fix that by setting the mask to 0xff like it's done during the flow translation in xlate_actions() and during the flow dump in the tun_metadata_from_geneve_nlattr(). Extra checks and comments added around the code to better explain what is going on. Fixes: a468645c6d33 ("lib/tc: add geneve with option match offload") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/netdev-offload-tc.c | 7 +++++-- lib/tc.c | 13 ++++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-)