| Message ID | 20210121223318.3353623-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] pcap-file: Fix calculation of TCP payload length in tcp_reader_run(). | expand |
On 1/21/21 11:33 PM, Ben Pfaff wrote: > The calculation in tcp_reader_run() failed to account for L2 padding. > This fixes the problem, by moving the existing function > tcp_payload_length() from a conntrack private header file into > dp-packet.h and renaming it to suit the dp_packet style. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- LGTM, Acked-by: Ilya Maximets <i.maximets@ovn.org>
On Tue, Feb 02, 2021 at 05:11:09PM +0100, Ilya Maximets wrote: > On 1/21/21 11:33 PM, Ben Pfaff wrote: > > The calculation in tcp_reader_run() failed to account for L2 padding. > > This fixes the problem, by moving the existing function > > tcp_payload_length() from a conntrack private header file into > > dp-packet.h and renaming it to suit the dp_packet style. > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > LGTM, > Acked-by: Ilya Maximets <i.maximets@ovn.org> Thanks, applied to master.
On Tue, Feb 2, 2021 at 10:00 AM Ben Pfaff <blp@ovn.org> wrote: > > On Tue, Feb 02, 2021 at 05:11:09PM +0100, Ilya Maximets wrote: > > On 1/21/21 11:33 PM, Ben Pfaff wrote: > > > The calculation in tcp_reader_run() failed to account for L2 padding. > > > This fixes the problem, by moving the existing function > > > tcp_payload_length() from a conntrack private header file into > > > dp-packet.h and renaming it to suit the dp_packet style. > > > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > --- > > > > LGTM, > > Acked-by: Ilya Maximets <i.maximets@ovn.org> > > Thanks, applied to master. Hi Ben and Ilya, I'm confused with what l2_pad_size is. I thought it's between L2 and L3 header, there is a 2-byte padding to make it 16-byte alignment. But it doesn't look like that. Then every time dp_packet API use dp_packet_tail, we have to subtract the l2_pad_size. ex: dp_packet_l4_size() So is thie l2_pad_size the extra byte at the end of buffer? (before the memory pointed by dp_packet_tail(p)? Thanks William
On Fri, Feb 12, 2021 at 11:15:46AM -0800, William Tu wrote: > I'm confused with what l2_pad_size is. > I thought it's between L2 and L3 header, there is a 2-byte padding to > make it 16-byte alignment. But it doesn't look like that. Ethernet has a 64-byte minimum packet length. l2_pad_size is the number of bytes of padding added to a packet to bring it up to that minimum length.
I see, thanks. On Tue, Feb 16, 2021 at 12:52 PM Ben Pfaff <blp@ovn.org> wrote: > > On Fri, Feb 12, 2021 at 11:15:46AM -0800, William Tu wrote: > > I'm confused with what l2_pad_size is. > > I thought it's between L2 and L3 header, there is a 2-byte padding to > > make it 16-byte alignment. But it doesn't look like that. > > Ethernet has a 64-byte minimum packet length. l2_pad_size is the number > of bytes of padding added to a packet to bring it up to that minimum > length.
diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h index 3895bc6880d7..e8332bdba1ec 100644 --- a/lib/conntrack-private.h +++ b/lib/conntrack-private.h @@ -204,16 +204,4 @@ struct ct_l4_proto { struct ct_dpif_protoinfo *); }; -static inline uint32_t -tcp_payload_length(struct dp_packet *pkt) -{ - const char *tcp_payload = dp_packet_get_tcp_payload(pkt); - if (tcp_payload) { - return ((char *) dp_packet_tail(pkt) - dp_packet_l2_pad_size(pkt) - - tcp_payload); - } else { - return 0; - } -} - #endif /* conntrack-private.h */ diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c index 18a2aa7c7d02..8a7c98cc459e 100644 --- a/lib/conntrack-tcp.c +++ b/lib/conntrack-tcp.c @@ -175,7 +175,7 @@ tcp_conn_update(struct conntrack *ct, struct conn *conn_, uint16_t win = ntohs(tcp->tcp_winsz); uint32_t ack, end, seq, orig_seq; - uint32_t p_len = tcp_payload_length(pkt); + uint32_t p_len = dp_packet_get_tcp_payload_length(pkt); if (tcp_invalid_flags(tcp_flags)) { COVERAGE_INC(conntrack_invalid_tcp_flags); @@ -450,7 +450,7 @@ tcp_new_conn(struct conntrack *ct, struct dp_packet *pkt, long long now, dst = &newconn->peer[1]; src->seqlo = ntohl(get_16aligned_be32(&tcp->tcp_seq)); - src->seqhi = src->seqlo + tcp_payload_length(pkt) + 1; + src->seqhi = src->seqlo + dp_packet_get_tcp_payload_length(pkt) + 1; if (tcp_flags & TCP_SYN) { src->seqhi++; diff --git a/lib/conntrack.c b/lib/conntrack.c index bba38f9f576d..423281a257a6 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -2899,7 +2899,7 @@ get_ftp_ctl_msg(struct dp_packet *pkt, char *ftp_msg) { struct tcp_header *th = dp_packet_l4(pkt); char *tcp_hdr = (char *) th; - uint32_t tcp_payload_len = tcp_payload_length(pkt); + uint32_t tcp_payload_len = dp_packet_get_tcp_payload_length(pkt); size_t tcp_payload_of_interest = MIN(tcp_payload_len, LARGEST_FTP_MSG_OF_INTEREST); size_t tcp_hdr_len = TCP_OFFSET(th->tcp_ctl) * 4; diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 0430cca8ebbd..36c53360cde8 100644 --- a/lib/dp-packet.h +++ b/lib/dp-packet.h @@ -466,6 +466,18 @@ dp_packet_get_tcp_payload(const struct dp_packet *b) return NULL; } +static inline uint32_t +dp_packet_get_tcp_payload_length(const struct dp_packet *pkt) +{ + const char *tcp_payload = dp_packet_get_tcp_payload(pkt); + if (tcp_payload) { + return ((char *) dp_packet_tail(pkt) - dp_packet_l2_pad_size(pkt) + - tcp_payload); + } else { + return 0; + } +} + static inline const void * dp_packet_get_udp_payload(const struct dp_packet *b) { diff --git a/lib/pcap-file.c b/lib/pcap-file.c index f0cac8e0fa4d..b30a11c24b87 100644 --- a/lib/pcap-file.c +++ b/lib/pcap-file.c @@ -411,7 +411,7 @@ tcp_reader_run(struct tcp_reader *r, const struct flow *flow, } tcp = dp_packet_l4(packet); flags = TCP_FLAGS(tcp->tcp_ctl); - l7_length = (char *) dp_packet_tail(packet) - l7; + l7_length = dp_packet_get_tcp_payload_length(packet); seq = ntohl(get_16aligned_be32(&tcp->tcp_seq)); /* Construct key. */
The calculation in tcp_reader_run() failed to account for L2 padding. This fixes the problem, by moving the existing function tcp_payload_length() from a conntrack private header file into dp-packet.h and renaming it to suit the dp_packet style. Signed-off-by: Ben Pfaff <blp@ovn.org> --- lib/conntrack-private.h | 12 ------------ lib/conntrack-tcp.c | 4 ++-- lib/conntrack.c | 2 +- lib/dp-packet.h | 12 ++++++++++++ lib/pcap-file.c | 2 +- 5 files changed, 16 insertions(+), 16 deletions(-)