Page MenuHomeFreeBSD

extend the use of the th_flags accessor function
Needs ReviewPublic

Authored by rscheff on Fri, Oct 11, 4:13 PM.
Tags
None
Referenced Files
F102083966: D47063.id144782.diff
Thu, Nov 7, 10:25 AM
Unknown Object (File)
Wed, Nov 6, 4:44 AM
Unknown Object (File)
Wed, Nov 6, 3:44 AM
Unknown Object (File)
Wed, Nov 6, 3:30 AM
Unknown Object (File)
Wed, Nov 6, 3:29 AM
Unknown Object (File)
Wed, Nov 6, 3:28 AM
Unknown Object (File)
Wed, Nov 6, 1:39 AM
Unknown Object (File)
Mon, Nov 4, 12:14 AM

Details

Reviewers
tuexen
cc
cy
glebius
shurd
Group Reviewers
transport
Restricted Owners Package(Owns No Changed Paths)
iflib
Summary

Formally, there are 12 bits for TCP header flags.
Use the accessor functions in more (kernel) places.

No functional change.

Diff Detail

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

Event Timeline

cy requested changes to this revision.Fri, Oct 11, 4:45 PM
cy added a subscriber: cy.

This fails to build in /sbin/natd.

ld: error: undefined reference due to --no-allow-shlib-undefined: tcp_get_flags

referenced by /export/obj/opt/src/git-src/amd64.amd64/tmp/usr/lib/libalias.so

cc: error: linker command failed with exit code 1 (use -v to see invocation)

  • Error code 1

Stop.
make[4]: stopped making "all" in /opt/src/git-src/sbin/natd
.ERROR_TARGET='natd.full'

This revision now requires changes to proceed.Fri, Oct 11, 4:45 PM
  • use userspace accessor function

I'm happy with the pf bits.
It mostly doesn't matter because we tend to only look for th_flags flags and not th_x2, but it's probably better to use the accessor anyway.

We should probably do this too:

diff --git a/sys/net/pfvar.h b/sys/net/pfvar.h
index 30be1128d4d3..88f9c236b14d 100644
--- a/sys/net/pfvar.h
+++ b/sys/net/pfvar.h
@@ -2338,7 +2338,7 @@ extern struct pf_ksrc_node        *pf_find_src_node(struct pf_addr *,
 extern void                     pf_unlink_src_node(struct pf_ksrc_node *);
 extern u_int                    pf_free_src_nodes(struct pf_ksrc_node_list *);
 extern void                     pf_print_state(struct pf_kstate *);
-extern void                     pf_print_flags(u_int8_t);
+extern void                     pf_print_flags(uint16_t);
 extern int                      pf_addr_wrap_neq(struct pf_addr_wrap *,
                                    struct pf_addr_wrap *);
 extern u_int16_t                pf_cksum_fixup(u_int16_t, u_int16_t, u_int16_t,
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index bd8b709e396e..47134a169a68 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -2891,7 +2891,7 @@ pf_print_state_parts(struct pf_kstate *s,
 }

 void
-pf_print_flags(u_int8_t f)
+pf_print_flags(uint16_t f)
 {
        if (f)
                printf(" ");
@@ -2911,6 +2911,8 @@ pf_print_flags(u_int8_t f)
                printf("E");
        if (f & TH_CWR)
                printf("W");
+       if (f & TH_AE)
+               printf("[AE]");
 }

 #define        PF_SET_SKIP_STEPS(i)                                    \
sys/netinet/tcp_output.c
1310–1311

This looks weird though. Do we really want to not update the TCP header with this flag?
I may have missed it because it's a fairly large function, but it doesn't look like we end up applying 'flags' to 'th_flags' later.

  • add TH_AE decoding (identical to tcpdump as "e") as suggested by kp
  • moving updating the th_flags further down past the last adjustment
In D47063#1073100, @kp wrote:

I'm happy with the pf bits.
It mostly doesn't matter because we tend to only look for th_flags flags and not th_x2, but it's probably better to use the accessor anyway.

We should probably do this too
...

Thanks. I added this, but used the same character ("e") used by tcpdump ( https://github.com/the-tcpdump-group/tcpdump/pull/999/commits/55dc3206aa07c3cdfe19b8d23e7b134d37b9a6b7#diff-7334e7c33e9abb09f57bd40e3c24fbb29a7088bf0ab7cc2f499b1425fc6b9741 ) for consistency (even though here we use "A" instead of "." for the ACK flag).

sys/netinet/tcp_output.c
1310–1311

Thanks for spotting this - I missed that the update to th_flags was above this line - apparently I missed this final th_flags update in the original change. Moved the final back-assignment after this conditional.

rscheff marked an inline comment as done.
  • extend the use of accessor function in userspace and pfil

Thanks. I added this, but used the same character ("e") used by tcpdump

Yeah, that's better.

I'm still happy with the pf bits.

cc requested changes to this revision.Mon, Oct 14, 1:01 PM

I current concern is that new code for the TH_AE shall be in a separate patch, so that this patch can be a pure big non-functional change.

sys/netinet/libalias/alias.c
186

If possible, suggest to use the explicit uint16_t for the type of th_flags throughout this change.

sys/netpfil/ipfilter/netinet/ip_compat.h
1131–1134 ↗(On Diff #144782)

Suggest to separate this new code into a new patch.

sys/netpfil/pf/pf.c
2922

Same here, new code shall be in new patch.

usr.sbin/ppp/ip.c
374–377

For readability, can we also make it cleaner by removing the outer ()? For example:

estab = __tcp_get_flags(th) & TH_ACK;
564

Same here, new code shall be in a new patch.

This revision now requires changes to proceed.Mon, Oct 14, 1:01 PM
  • clean up type and remove superfluous parenthesis
  • revert adding support for TH_AE in ppp and pf
  • missed function definition
cc added inline comments.
sys/netpfil/ipfw/ip_fw2.c
1024

Remove the extra space in if ( (.

Builds now. Thanks.

This revision is now accepted and ready to land.Thu, Oct 31, 4:46 PM
  • use accessor in various drivers
This revision now requires review to proceed.Fri, Nov 1, 10:14 AM
Owners added a reviewer: Restricted Owners Package.Fri, Nov 1, 10:14 AM

This seems fine to me (cxgbe bits). In some places I misread tcp_set_flags as being an or (I) operation rather than assignment (=) since in English prose "setting a flag" usually means enabling a flag (or flags) leaving other bits unchanged. In a similar vein, having a tcp_clear_flags helper that took a mask of bits to clear might be nicer to replace places that previously used &= ~. Some sort of wrapper for |= might also be nice. I'm not sure what to call that wrapper though without further tripping over the ambiguity of tcp_set_flags. (atomic_set_* set bits rather than assigning values and that has caused some confusion in the past as well FWIW.)