| Message ID | 20200611100231.2829329-1-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] odp-execute: Extract l2 padding while executing check_pkt_len action. | expand |
On 6/11/20 12:02 PM, Ilya Maximets wrote: > If dp-packet contains l2 padding its size will be larger than the > actual size of a payload and action will work incorrectly. > > Padding could be set during miniflow_extract() if detected. > > CC: Numan Siddique <nusiddiq@redhat.com> > Fixes: 5b34f8fc3b38 ("Add a new OVS action check_pkt_larger") > Reported-by: Miroslav Kubiczek <miroslav.kubiczek@adaptivemobile.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-May/050157.html > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > > I didn't test this, only checked existing unit tests. > > Miroslav, could you, please, check if this works in your case? > > It might be also good to write some unit test for this. > > lib/odp-execute.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > index 6018e378a..548083cef 100644 > --- a/lib/odp-execute.c > +++ b/lib/odp-execute.c > @@ -761,10 +761,10 @@ odp_execute_check_pkt_len(void *dp, struct dp_packet *packet, bool steal, > > const struct nlattr *a; > struct dp_packet_batch pb; > + uint32_t size = dp_packet_size(packet) - dp_packet_l2_pad_size(packet); We might also need to subtract dp_packet_get_cutlen() or use dp_packet_get_send_len() instead of dp_packet_size() for the same purpose, otherwise cutlen + check_pkt_len might not work as expected. > > a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN]; > - bool is_greater = dp_packet_size(packet) > nl_attr_get_u16(a); > - if (is_greater) { > + if (size > nl_attr_get_u16(a)) { > a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER]; > } else { > a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL]; >
On 6/11/20 12:12 PM, Ilya Maximets wrote: > On 6/11/20 12:02 PM, Ilya Maximets wrote: >> If dp-packet contains l2 padding its size will be larger than the >> actual size of a payload and action will work incorrectly. >> >> Padding could be set during miniflow_extract() if detected. >> >> CC: Numan Siddique <nusiddiq@redhat.com> >> Fixes: 5b34f8fc3b38 ("Add a new OVS action check_pkt_larger") >> Reported-by: Miroslav Kubiczek <miroslav.kubiczek@adaptivemobile.com> >> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-May/050157.html >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> >> I didn't test this, only checked existing unit tests. >> >> Miroslav, could you, please, check if this works in your case? >> >> It might be also good to write some unit test for this. >> >> lib/odp-execute.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/lib/odp-execute.c b/lib/odp-execute.c >> index 6018e378a..548083cef 100644 >> --- a/lib/odp-execute.c >> +++ b/lib/odp-execute.c >> @@ -761,10 +761,10 @@ odp_execute_check_pkt_len(void *dp, struct dp_packet *packet, bool steal, >> >> const struct nlattr *a; >> struct dp_packet_batch pb; >> + uint32_t size = dp_packet_size(packet) - dp_packet_l2_pad_size(packet); > > We might also need to subtract dp_packet_get_cutlen() or use > dp_packet_get_send_len() instead of dp_packet_size() for the > same purpose, otherwise cutlen + check_pkt_len might not work > as expected. > I only had a quick glance at this and I'm not that familiar to the code base but the patch looks good to me (with dp_packet_get_send_len() instead of dp_packet_size()). Regards, Dumitru >> >> a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN]; >> - bool is_greater = dp_packet_size(packet) > nl_attr_get_u16(a); >> - if (is_greater) { >> + if (size > nl_attr_get_u16(a)) { >> a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER]; >> } else { >> a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL]; >> >
diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 6018e378a..548083cef 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -761,10 +761,10 @@ odp_execute_check_pkt_len(void *dp, struct dp_packet *packet, bool steal, const struct nlattr *a; struct dp_packet_batch pb; + uint32_t size = dp_packet_size(packet) - dp_packet_l2_pad_size(packet); a = attrs[OVS_CHECK_PKT_LEN_ATTR_PKT_LEN]; - bool is_greater = dp_packet_size(packet) > nl_attr_get_u16(a); - if (is_greater) { + if (size > nl_attr_get_u16(a)) { a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_GREATER]; } else { a = attrs[OVS_CHECK_PKT_LEN_ATTR_ACTIONS_IF_LESS_EQUAL];
If dp-packet contains l2 padding its size will be larger than the actual size of a payload and action will work incorrectly. Padding could be set during miniflow_extract() if detected. CC: Numan Siddique <nusiddiq@redhat.com> Fixes: 5b34f8fc3b38 ("Add a new OVS action check_pkt_larger") Reported-by: Miroslav Kubiczek <miroslav.kubiczek@adaptivemobile.com> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-May/050157.html Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- I didn't test this, only checked existing unit tests. Miroslav, could you, please, check if this works in your case? It might be also good to write some unit test for this. lib/odp-execute.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)