Page MenuHomeFreeBSD

add TH_AE capabilities to ppp and pf
ClosedPublic

Authored by rscheff on Oct 14 2024, 1:59 PM.
Tags
None
Referenced Files
F106091087: D47106.diff
Wed, Dec 25, 6:15 AM
Unknown Object (File)
Fri, Dec 20, 1:33 AM
Unknown Object (File)
Wed, Dec 18, 10:44 AM
Unknown Object (File)
Fri, Nov 29, 9:54 AM
Unknown Object (File)
Nov 15 2024, 6:17 AM
Unknown Object (File)
Nov 14 2024, 10:33 PM
Unknown Object (File)
Nov 14 2024, 12:22 PM
Unknown Object (File)
Nov 13 2024, 10:42 AM

Details

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!

@kp can you please review the PF-related changes in this Diff?

@kp can you please review the PF-related changes in this Diff?

I'm happy with the pf bits.

This revision was not accepted when it landed; it landed in state Needs Review.Fri, Nov 29, 9:51 AM
This revision was automatically updated to reflect the committed changes.