extend the use of the th_flags accessor function

[rscheff]

Summary

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

No functional change.

[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'

[rscheff]

• use userspace accessor function

[kp]

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)                                    \

[kp]

sys/netinet/tcp_output.c

1309–1310

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.

[rscheff]

• add TH_AE decoding (identical to tcpdump as "e") as suggested by kp
• moving updating the th_flags further down past the last adjustment

[rscheff]

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

1309–1310

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]

• extend the use of accessor function in userspace and pfil

[kp]

In D47063#1073412, @rscheff wrote:

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]

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

2914

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.

[rscheff]

• clean up type and remove superfluous parenthesis
• revert adding support for TH_AE in ppp and pf

[rscheff]

• missed function definition

[cc]

sys/netpfil/ipfw/ip_fw2.c

1024

Remove the extra space in if ( (.

[cy]

Builds now. Thanks.

[rscheff]

• use accessor in various drivers

[jhb]

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.)