| Message ID | 20250213202425.3566160-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | 0fb370bdcd943b66c26c166cbbefcc4f8023aeff |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] flow: Explicitly pad tcp_flags for TCP and tp_dst for IGMP. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/cirrus-robot | success | cirrus build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 2/13/25 21:24, Ilya Maximets wrote: > 'tcp_flags' are not placed at the beginning of the 64-bit block, so > they have to be padded. Today it is done in a hacky way by pushing > zeroes over the last 32-bits of the arp_tha. When that was written > the miniflow_pad_from_64() didn't exist, but it's better to use it > now instead to avoid confusion. > > 'ct_tp_src/dst' are not actually extracted for IGMP. See the > write_ct_md() function. The pushes are there for the padding purposes, > since 'tp_dst' doesn't end on a 64-bit boundary and so we need to pad > before pushing the IGMP group. Use an explicit padding function > instead to avoid a false impression that IGMP can have non-zero > "ports" in the conntrack tuple. > > This change should not change anything functionally. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- Recheck-request: github-robot
Ilya Maximets <i.maximets@ovn.org> writes: > 'tcp_flags' are not placed at the beginning of the 64-bit block, so > they have to be padded. Today it is done in a hacky way by pushing > zeroes over the last 32-bits of the arp_tha. When that was written > the miniflow_pad_from_64() didn't exist, but it's better to use it > now instead to avoid confusion. > > 'ct_tp_src/dst' are not actually extracted for IGMP. See the > write_ct_md() function. The pushes are there for the padding purposes, > since 'tp_dst' doesn't end on a 64-bit boundary and so we need to pad > before pushing the IGMP group. Use an explicit padding function > instead to avoid a false impression that IGMP can have non-zero > "ports" in the conntrack tuple. > > This change should not change anything functionally. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- Hi Ilya, the change LGTM. Acked-by: Paolo Valerio <pvalerio@redhat.com>
On 2/18/25 12:42, Paolo Valerio wrote: > Ilya Maximets <i.maximets@ovn.org> writes: > >> 'tcp_flags' are not placed at the beginning of the 64-bit block, so >> they have to be padded. Today it is done in a hacky way by pushing >> zeroes over the last 32-bits of the arp_tha. When that was written >> the miniflow_pad_from_64() didn't exist, but it's better to use it >> now instead to avoid confusion. >> >> 'ct_tp_src/dst' are not actually extracted for IGMP. See the >> write_ct_md() function. The pushes are there for the padding purposes, >> since 'tp_dst' doesn't end on a 64-bit boundary and so we need to pad >> before pushing the IGMP group. Use an explicit padding function >> instead to avoid a false impression that IGMP can have non-zero >> "ports" in the conntrack tuple. >> >> This change should not change anything functionally. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- > > Hi Ilya, the change LGTM. > > Acked-by: Paolo Valerio <pvalerio@redhat.com> > Thanks, Paolo! Applied to main. Best regards, Ilya Maximets.
diff --git a/lib/flow.c b/lib/flow.c index 0eb34892f..53ff7178e 100644 --- a/lib/flow.c +++ b/lib/flow.c @@ -1043,7 +1043,8 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) if (OVS_LIKELY(tcp_hdr_len >= TCP_HEADER_LEN) && OVS_LIKELY(size >= tcp_hdr_len)) { - miniflow_push_be32(mf, arp_tha.ea[2], 0); + /* tcp_flags are not at the beginning of the block. */ + miniflow_pad_from_64(mf, tcp_flags); miniflow_push_be32(mf, tcp_flags, TCP_FLAGS_BE32(tcp->tcp_ctl)); miniflow_push_be16(mf, tp_src, tcp->tcp_src); @@ -1110,8 +1111,8 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst) miniflow_push_be16(mf, tp_src, htons(igmp->igmp_type)); miniflow_push_be16(mf, tp_dst, htons(igmp->igmp_code)); - miniflow_push_be16(mf, ct_tp_src, ct_tp_src); - miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst); + /* ct_tp_src/dst are not extracted for IGMP. */ + miniflow_pad_to_64(mf, tp_dst); miniflow_push_be32(mf, igmp_group_ip4, get_16aligned_be32(&igmp->group)); miniflow_pad_to_64(mf, igmp_group_ip4);
'tcp_flags' are not placed at the beginning of the 64-bit block, so they have to be padded. Today it is done in a hacky way by pushing zeroes over the last 32-bits of the arp_tha. When that was written the miniflow_pad_from_64() didn't exist, but it's better to use it now instead to avoid confusion. 'ct_tp_src/dst' are not actually extracted for IGMP. See the write_ct_md() function. The pushes are there for the padding purposes, since 'tp_dst' doesn't end on a 64-bit boundary and so we need to pad before pushing the IGMP group. Use an explicit padding function instead to avoid a false impression that IGMP can have non-zero "ports" in the conntrack tuple. This change should not change anything functionally. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/flow.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)