Page MenuHomeFreeBSD

add TH_AE capabilities to ppp and pf
Needs ReviewPublic

Authored by rscheff on Mon, Oct 14, 1:59 PM.
Tags
None
Referenced Files
F101976048: D47106.diff
Wed, Nov 6, 1:34 AM
F101971038: D47106.id144880.diff
Tue, Nov 5, 11:42 PM
Unknown Object (File)
Fri, Oct 25, 6:29 PM
Unknown Object (File)
Sun, Oct 20, 7:58 AM
Unknown Object (File)
Sun, Oct 20, 4:44 AM
Unknown Object (File)
Fri, Oct 18, 3:22 PM
Unknown Object (File)
Wed, Oct 16, 6:48 AM
Unknown Object (File)
Wed, Oct 16, 6:16 AM

Details

Reviewers
kp
cc
tuexen
cy
Group Reviewers
transport
Summary

Split out changes from D47063 which are strictly
speaking changing the functionality.

Add support for the AE Flag in the TCP header to pf and ppp

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 59993
Build 56878: arc lint + arc unit

Event Timeline

pf bits look good.

usr.sbin/ppp/ip.c
563

It looks like we only use this in a loop that ends when mask == 0x40, so we're never going to see ECE, CWR or AE there, right?

  • missed function definition
sys/netpfil/ipfilter/netinet/ip_compat.h
698

I found TCPF_ALL is used in printtcpflags() from sbin/ipf/libipf/printtcpflags.c.

Would you please also add these in flagset[] and flags[] from sbin/ipf/libipf/flags.c?

  • decode ACE in libipf too
  • expand ipf for AccECN compatibility
  • prepare more ipf tools for AccECN
  • log TCP flags properly in ppp
In D47106#1073744, @kp wrote:

pf bits look good.

usr.sbin/ppp/ip.c
563

Then let's remove the magic number, and decode all TCP header flags properly for logging instead...

My current concern is that the definition and the usage of the super set macro TH_FLAGS or TCPF_ALL are inconsistent. For example, TH_ECE is in TH_FLAGS, but TH_ECN is in TCPF_ALL.

sbin/ipf/ipsend/ipsend.c
368–393

Looks the same typo is among these lines, shall use __tcp_get_flags() inside, like this:

	__tcp_set_flags(tcp, __tcp_get_flags(tcp) | TH_SYN);
sbin/ipf/ipsend/iptests.c
927

I am thinking if we can just use the super set macro TH_FLAGS from sys/netinet/tcp.h or TCPF_ALL from sys/netpfil/ipfilter/netinet/ip_compat.h. But looks there is duplicated definition as TH_ECE == TH_ECN. I am confused about which one to use. But since you are using TH_ECE, so I take it for granted to use TH_FLAGS from sys/netinet/tcp.h.

sbin/ipf/libipf/printpacket.c
103

Shall be W for TH_CWR.

rscheff marked an inline comment as done.
  • fix typo, remove redundant all-flags definitions

Thanks for the thorough review!

I did build world, but the typo wasn't flagged - is that source code not compiled by GENERIC?

sbin/ipf/ipsend/iptests.c
927

Yes, there is no point of having multiple independent definitions to sum up all the TCP header flags. Removing the redundant definitions and using TH_FLAGS is the better path. Thanks!