Introduce ip_af_t

[kp]

Summary

Introduce an enum that contains only options for AF_INET/AF_INET6. This allows
us to remove the 'default: panic()' case from a number of switch statements,
simplifying the code a litte.

We #ifdef the enum values so that the compiler does not complain if we do not
handle e.g. AF_INET in a NOINET kernel.

Suggested by: glebius
Sponsored by: Rubicon Communications, LLC ("Netgate")

[kp]

Actually remove the now redundant panic() calls.

[kp]

Actually build on make universe.

[zlei]

sys/net/pfvar.h

1599 ↗

(On Diff #143435)

I do not have strong option for this, since now we have assertings such as KASSERT(af == AF_INET || af == AF_INET .

[kp]

sys/net/pfvar.h

1599 ↗

(On Diff #143435)

I like it on the grounds that we can use AF_INET where we mean PF_AF_INET, which means fewer code changes in the rest of pf.

The ifdef are there so we are reminded to correctly ifdef out the relevant code on NOINET/NOINET6 configurations.
It means we get the compiler to tell us if we forget to cover cases. That also means we don't want options we don't support (e.g. AF_INET6 on a NONINET6 kernel), because otherwise the compiler wold complain we forget an option in the switch statement.

[glebius]

I just discussed this with Kristof offline, but now posting for wider discussion. I think this approach should be applied outside pf(4) and used all around the network stack where we select between IPv4 and IPv6. That means, that the selector type name shouldn't have the PF_ or pf_ prefix. That would allow to start using the same technique all around without renaming inside pf(4).

Suggested name (feel free to come with a better one!):

#ifndef AF_INET
/* For binary compatibility reasons assign ip_version_t values to sa_family_t.  This will allow to call into old style code from the new style code and vice versa. */
#define AF_INET         2              /* IPv4 */
#define AF_INET6        28             /* IPv6 */
#endif
typedef enum {
        IPv4 = AF_INET,
        IPv6 = AF_INET6,
} ip_version_t;

[kp]

In D46683#1064617, @glebius wrote:

I just discussed this with Kristof offline, but now posting for wider discussion. I think this approach should be applied outside pf(4) and used all around the network stack where we select between IPv4 and IPv6. That means, that the selector type name shouldn't have the PF_ or pf_ prefix. That would allow to start using the same technique all around without renaming inside pf(4).

I'll see if I can come up with a draft patch for this for wider discussion in the next couple of days.

[kp]

Move to a global ip_version_t rather than pf_af_t

This requires us to include opt_inet and opt_inet6 so we don't define IPv6 in
a NOINET6 kernel (because we'll likely #ifdef out the case AF_INET6 in those
kernels, which would lead to the compliler complaining about us not handling all
enum values).

This also reveals that some code uses #ifdef INET6 but doesn't include
opt_inet6.h. Fix one instance here (which ought to be a separate commit, but is
included here to demonstrate the concern)

[glebius]

I'd suggest to use the enum names IPv4 and IPv6 in the new code, but that is up to you.

[bz]

Can we please not mix the addition of the type with some other changes (which by far do not cover all the cases in the tree to simplify if I am not wrong) or add missing #ifdef INET/#ifdef INET6? It's kind of three changes in 1.

Also I'd suggest simply putting this in socket.h and be done with it.

sys/netinet/in.h

434

Can we please not do CaMeLcAsE?

Also why can we not just do:

typedef enum {
    AF_INET = 2,
    AF_INET6 = 28,
} ip_version_t;

And put it into socket.h instead?

[glebius]

Also I'd suggest simply putting this in socket.h and be done with it.

This has nothing to do with sockets. And suggested code won't compile, of course, since after preprocessor it will end up as 2 = 2.

Bjoern, the whole idea is to make the code more readable for somebody, who doesn't have a 30 year old history of BSD stack in their mind. Some things we are used to, aren't good, they are just what we are used to. They can be made better.

[bz]

In D46683#1066394, @glebius wrote:

Also I'd suggest simply putting this in socket.h and be done with it.

This has nothing to do with sockets. And suggested code won't compile, of course, since after preprocessor it will end up as 2 = 2.

Bjoern, the whole idea is to make the code more readable for somebody, who doesn't have a 30 year old history of BSD stack in their mind. Some things we are used to, aren't good, they are just what we are used to. They can be made better.

Why not? This should work, right?

diff --git sys/sys/socket.h sys/sys/socket.h
index a1b220ce4bae..f773f970b779 100644
--- sys/sys/socket.h
+++ sys/sys/socket.h
@@ -216,12 +216,14 @@ struct accept_filter_arg {
  * Address families.
  */
 #define        AF_UNSPEC       0               /* unspecified */
-#if __BSD_VISIBLE
-#define        AF_LOCAL        AF_UNIX         /* local to host (pipes, portals) */
-#endif
 #define        AF_UNIX         1               /* standardized name for AF_LOCAL */
-#define        AF_INET         2               /* internetwork: UDP, TCP, etc. */
+typedef enum {
+       AF_INET         = 2,            /* internetwork: UDP, TCP, etc. */
+       AF_INET6        = 28,           /* IPv6 */
+} ip_version_t;
+
 #if __BSD_VISIBLE
+#define        AF_LOCAL        AF_UNIX         /* local to host (pipes, portals) */
 #define        AF_IMPLINK      3               /* arpanet imp addresses */
 #define        AF_PUP          4               /* pup protocols: e.g. BSP */
 #define        AF_CHAOS        5               /* mit CHAOS protocols */
@@ -249,9 +251,6 @@ struct accept_filter_arg {
 #define        AF_ISDN         26              /* Integrated Services Digital Network*/
 #define        AF_E164         AF_ISDN         /* CCITT E.164 recommendation */
 #define        pseudo_AF_KEY   27              /* Internal key-management function */
-#endif
-#define        AF_INET6        28              /* IPv6 */
-#if __BSD_VISIBLE
 #define        AF_NATM         29              /* native ATM access */
 #define        AF_ATM          30              /* ATM */
 #define pseudo_AF_HDRCMPLT 31          /* Used by BPF to not rewrite headers

Otherwise if you wanted a real "IP Version Type" then you'd define IPV4 as 4 and IPV6 as 6 but that's not what you are actually after, so maybe the enum is misnamed and should really be ip_af_t (whether here or in in.h)? If you call it af_ip_t it also perfectly fits in the above context.

[kp]

In D46683#1066400, @bz wrote:

diff --git sys/sys/socket.h sys/sys/socket.h
index a1b220ce4bae..f773f970b779 100644
--- sys/sys/socket.h
+++ sys/sys/socket.h
@@ -216,12 +216,14 @@ struct accept_filter_arg {
 #define        AF_UNIX         1               /* standardized name for AF_LOCAL */
-#define        AF_INET         2               /* internetwork: UDP, TCP, etc. */
+typedef enum {
+       AF_INET         = 2,            /* internetwork: UDP, TCP, etc. */
+       AF_INET6        = 28,           /* IPv6 */
+} ip_version_t;
+

Otherwise if you wanted a real "IP Version Type" then you'd define IPV4 as 4 and IPV6 as 6 but that's not what you are actually after, so maybe the enum is misnamed and should really be ip_af_t (whether here or in in.h)? If you call it af_ip_t it also perfectly fits in the above context.

That won't quite work, because the ability to not define the IPv4 version for NOINET or IPv6 version for NOINET6 kernels is important. Otherwise the compiler would end up complaining we don't cover all enum values in the switch block. (Or we'd have to keep a default case, the removal of which was the impetus for introducing this enum in the first place.)
We have to define AF_INET even in a NOINET kernel, so we can't just ifdef that out.

As for the other remarks, I'm happy to rename this, ip_af_t seems reasonable, and I'm also happy to separate out commits for the definition of the new type and the use of it in pf, but I'd like us to agree on the enum definition (and especially the ifdef-ing of non-relevant versions) before I do that work.

[bz]

In D46683#1066486, @kp wrote:

In D46683#1066400, @bz wrote:

diff --git sys/sys/socket.h sys/sys/socket.h
index a1b220ce4bae..f773f970b779 100644
--- sys/sys/socket.h
+++ sys/sys/socket.h
@@ -216,12 +216,14 @@ struct accept_filter_arg {
 #define        AF_UNIX         1               /* standardized name for AF_LOCAL */
-#define        AF_INET         2               /* internetwork: UDP, TCP, etc. */
+typedef enum {
+       AF_INET         = 2,            /* internetwork: UDP, TCP, etc. */
+       AF_INET6        = 28,           /* IPv6 */
+} ip_version_t;
+

Otherwise if you wanted a real "IP Version Type" then you'd define IPV4 as 4 and IPV6 as 6 but that's not what you are actually after, so maybe the enum is misnamed and should really be ip_af_t (whether here or in in.h)? If you call it af_ip_t it also perfectly fits in the above context.

That won't quite work, because the ability to not define the IPv4 version for NOINET or IPv6 version for NOINET6 kernels is important. Otherwise the compiler would end up complaining we don't cover all enum values in the switch block. (Or we'd have to keep a default case, the removal of which was the impetus for introducing this enum in the first place.)
We have to define AF_INET even in a NOINET kernel, so we can't just ifdef that out.

As for the other remarks, I'm happy to rename this, ip_af_t seems reasonable, and I'm also happy to separate out commits for the definition of the new type and the use of it in pf, but I'd like us to agree on the enum definition (and especially the ifdef-ing of non-relevant versions) before I do that work.

Oh doh. Got you now. So the "IPv4" and "IPv6" are never to be used; this is more clever than I had gotten from the proposed commit message; can you leave a comment around the enum definition saying that and in that case we can name the "IPv4" and "IPv6" away to something a lot more complicated so we'll never have a conflict?

And thanks for offering to split this up.

[glebius]

-#define        AF_INET         2               /* internetwork: UDP, TCP, etc. */
+typedef enum {
+       AF_INET         = 2,            /* internetwork: UDP, TCP, etc. */
+       AF_INET6        = 28,           /* IPv6 */
+} ip_version_t;

This will break any third party software that checks for #ifdef AF_INET

Let's just do not touch user visible headers for this change. Let's forget about socket and about address families. Let's focus on that we need a enum to tell IPv4 packet header from IPv6 packet header, used by various kernel KPIs. There shall be no visible user changes.

[kp]

Rename to ip_af_t
Split out the pf changes to a separate commit

[kp]

The pf bits are in D46809

One issue is that this breaks the NOIP kernel. For some reason I did not pick this up on previous iterations (I was sure make universe passed on them, but I don't see how), but we can't have an empty enum definition. clang complains:

/usr/src/sys/netinet/in.h:445:1: error: use of empty enum
  445 | } ip_af_t;
      | ^
1 error generated.

I'm not sure what to do about that. Removing the enum entirely for NOIP configs would mean adding #if defined(INET) || defined(INET6) all over the place, and we're already adding a bunch of #ifdef INET as it is.

[bz]

sys/netinet/in.h

446

In https://reviews.freebsd.org/D46809 @adrian has a valid point; no ip_af_t code must be copiled in the NO-IP kernels as empty enums lead to compile time errors. So you cannot even have the argument type in that case.

[glebius]

I see no good reason to ifdef pieces of enum. Makes sense to disable some code for NOINET or NOINET6 kernels, but what's the point in disabling a piece of declaration?

[kp]

In D46683#1067412, @glebius wrote:

I see no good reason to ifdef pieces of enum. Makes sense to disable some code for NOINET or NOINET6 kernels, but what's the point in disabling a piece of declaration?

If we don't we upset the compiler in LINT-NOINET (and LINT-NOINET6) kernels:

/usr/src/sys/netpfil/pf/pf_lb.c:769:13: error: enumeration value 'IP_AF_4' not handled in switch [-Werror,-Wswitch]
  769 |                                 switch (pd->af) {
      |                                         ^~~~~~

It's fairly common to have code specific to one address family or another not be compiled in those kernels, so we end up with switch statements that don't handle all options. We can prevent this by adding a default: panic("Unknown af %d", af) case, but removing that was the reason to go down this path in the first place.

[adrian]

In D46683#1067664, @kp wrote:

In D46683#1067412, @glebius wrote:

I see no good reason to ifdef pieces of enum. Makes sense to disable some code for NOINET or NOINET6 kernels, but what's the point in disabling a piece of declaration?

If we don't we upset the compiler in LINT-NOINET (and LINT-NOINET6) kernels:

/usr/src/sys/netpfil/pf/pf_lb.c:769:13: error: enumeration value 'IP_AF_4' not handled in switch [-Werror,-Wswitch]
  769 |                                 switch (pd->af) {
      |                                         ^~~~~~

It's fairly common to have code specific to one address family or another not be compiled in those kernels, so we end up with switch statements that don't handle all options. We can prevent this by adding a default: panic("Unknown af %d", af) case, but removing that was the reason to go down this path in the first place.

Right, but then you absolutely need to handle the case that the code UNDER the switch statement not having its pre-requisites met!

It's no good to have a NOINET kernel compile and then you just run a bunch of address struct checks with invalid or zero data. Like, those code paths likely make no sense to be executed in the first place.

[glebius]

In D46683#1067664, @kp wrote:

If we don't we upset the compiler in LINT-NOINET (and LINT-NOINET6) kernels:

Damn, that's too bad :( Putting #ifdefs from opt_foo.h into installable headers is also planting a minefield. Let me think a bit more on possible options here.