Backport OpenBSD's pf Revision 1.1017 fixes to FreeBSD

[kp]

Summary

In OpenBSD back on Tue Mar 7 16:28:37 2017 UTC, a fix was commited to correct an issue where the flowid was stripped off the mbuf handling packets.

[kbowling]

sys/netpfil/pf/pf.c

6020 ↗

(On Diff #46369)

No need for either of these comment lines, this kind of thing will be captured in the commit metadata for the lines

6024 ↗

(On Diff #46369)

I think this is trying to put pf's session ID into the flow ID hash. You can probably use M_HASHTYPE_OPAQUE_HASH here and the line above.

[kp]

I don't think the comparison is quite correct, in that the '== 0' will make it do the reverse of what you what you intend.

Also, I would't think that adding in the _M_HASHTYPE_OPAQUE in the flowid would be required, and as it's an opaque value there doesn't seem to be any reason to do an endianness conversion. It's internal to this machine anyway.

+       if (s && M_HASHTYPE_GET(m) == M_HASHTYPE_NONE) {
+               M_HASHTYPE_SET(m, M_HASHTYPE_OPAQUE_HASH);
+               m->m_pkthdr.flowid = s->id;
+       }

[kp]

This change is probably also required for pf_test6(). OpenBSD unified pf_test() and pf_test6(), but we haven't done that yet.

[nonesuch_longcount.org]

Kristof , I am going to retest using the simplified patch. Do I need to resubmit it in this form ?

+       if (s && M_HASHTYPE_GET(m) == M_HASHTYPE_NONE) {
+               M_HASHTYPE_SET(m, M_HASHTYPE_OPAQUE_HASH);
+               m->m_pkthdr.flowid = s->id;
+       }

[kp]

In D16613#353056, @nonesuch_longcount.org wrote:

Kristof , I am going to retest using the simplified patch. Do I need to resubmit it in this form ?

No, that's okay. I've got the patch and can commit it once we've established it fixes your problem (and doesn't cause any new ones).

[nonesuch_longcount.org]

The 10-STABLE series of mbuf.h does not define a M_HASHTYPE_OPAQUE_HASH just a M_HASHTYPE_OPAQUE . Should I change the patch to work with 10 and make a new for 11 and on where opaque_hash exists ?
Here is a patch updated for the comments above that uses M_HASHTYPE_OPAQUE in place of M_HASHTYPE_OPAQUE_HASH ?

Index: pf.c
===================================================================
--- pf.c	(revision 337280)
+++ pf.c	(working copy)
@@ -1,5 +1,4 @@
-/*-
- * Copyright (c) 2001 Daniel Hartmeier
+/*- * Copyright (c) 2001 Daniel Hartmeier
  * Copyright (c) 2002 - 2008 Henning Brauer
  * Copyright (c) 2012 Gleb Smirnoff <[e-mail]>
  * All rights reserved.
@@ -6016,6 +6015,11 @@
 
 	}
 #endif /* ALTQ */
+      
+       if (s && M_HASHTYPE_GET(m) == M_HASHTYPE_NONE) {
+                M_HASHTYPE_SET(m, M_HASHTYPE_OPAQUE);
+               m->m_pkthdr.flowid = s->id;
+      }
 
 	/*
 	 * connections redirected to loopback should not match sockets

Also would it be in bad form to #define a new version of M_HASHTYPE_OPAQUE_HASH
for 10-STABLE ? Its a simple addition ?

#define M_HASHTYPE_OPAQUE_HASH          M_HASHTYPE_HASH(M_HASHTYPE_OPAQUE)

[kp]

Patches (almost) always go into CURRENT first, before they can be merged back into stable/11 and stable/10.

The distinction between M_HASHTYPE_OPAQUE and M_HASHTYPE_OPAQUE_HASH is the M_HASHTYPE_HASHPROP flag.
It indicates that 'the flow identifier has hash properties'. I think that it's fair to say that the pf state index provides this.

It doesn't look to matter for you though (in stable/10 at least), because the sfxge driver only cares that there is a flowid, not what type it is.

The CURRENT code, at least if RSS is defined, looks like it might not be fixed by this, because rss_m2bucket() returns -1 for M_HASHTYPE_OPAQUE(_HASH). The generic rss code doesn't seem to deal with OPAQUE hash types.
I wonder if we shouldn't be calculating a hash using rss_mbuf_software_hash_v4().

[nonesuch_longcount.org]

This diff removed the extraneous comments and simplified the flowid assignment with use of M_HASHTYPE_OPAQUE_HASH

sys/netpfil/pf/pf.c

6024 ↗

(On Diff #46369)

I am guessing that in openbsd land the bitwise and with the session id was to insure enough uniqueness in the flowid . I have been running for 16hrs off the code bellow but I can't trigger this condition to fire .
Please review and let me know what you think .

Index: pf.c
===================================================================
--- pf.c        (revision 337392)
+++ pf.c        (working copy)
@@ -6094,6 +6094,10 @@
        }
 #endif /* ALTQ */
 
+         if (s && (M_HASHTYPE_GET(m) == M_HASHTYPE_NONE)) {
+                M_HASHTYPE_SET(m, M_HASHTYPE_OPAQUE_HASH);
+                m->m_pkthdr.flowid = M_HASHTYPE_OPAQUE_HASH;
+            }
        /*
         * connections redirected to loopback should not match sockets
         * bound specifically to loopback due to security implications,

[kp]

pf.c

6099

This is wrong. The flowid is supposed to be different for every stream, or at least usually different, so you don't want to assign the constant here.
m->m_pkthdr.flowid = s->id; is what you meant.

Though if this condition never triggers it's presumably also not going to fix your problem.

It might be worth separating out the check for a state (i.e. s is not NULL) from the hash type check, just to make sure the problem doesn't only happen when we don't have a pf state for the packet.

[nonesuch_longcount.org]

This comment was removed by nonesuch_longcount.org.

[nonesuch_longcount.org]

All

After reviewing the original OpenBSD version and talking to one of the authors of the change . I better understand what is going on here.

Here is the openbsd version

if (s && (pd.m->m_pkthdr.ph_flowid & M_FLOWID_VALID) == 0) {
 		pd.m->m_pkthdr.ph_flowid = M_FLOWID_VALID | (M_FLOWID_MASK & bemtoh64(&s->id));
 	}

What they are checking for is if there is a pf session and there is a null / 0 flow id to create a flow id using their normal way .
That being said , this will not work on FreeBSD consider this modified patch I made today.

For clarity M_FLOWID_VALID is 0x8000 and its in the OpenBSD mbuf.h file. Since FreeBSD does not have the same item I am just using
the number.

+         if (s && (m->m_pkthdr.flowid & 0x8000) == 0){
+               M_HASHTYPE_SET(m, M_HASHTYPE_OPAQUE_HASH);
+               m->m_pkthdr.flowid = s->id;
+         }

I think that setting the OPAQUE_HASH on the mbuf and pushing the session id as the flowid works but I am unsure if this is the correct way to set a flowid.
Anyone have any advice ?

[kp]

OpenBSD use M_FLOWID_VALID to indicate if the flowid is set. FreeBSD does it slightly differently, and uses a separate field, usually accessed via M_HASHTYPE_SET(), M_HASHTYPE_GET(), M_HASHTYPE_ISHASH(), ... macros.

Checking for the top bit in flowid is almost certainly not what you want to do here.
Sadly the OpenBSD commit messages don't have any information on why they decided to set the flowid from pf.

I still think the functional equivalent for FreeBSD is this:

+       if (s && M_HASHTYPE_GET(m) == M_HASHTYPE_NONE) {
+               M_HASHTYPE_SET(m, M_HASHTYPE_OPAQUE_HASH);
+               m->m_pkthdr.flowid = s->id;
+       }

But I also think pf doesn't mess with flowids, so I'm not sure this is the correct fix for your problem. We'll need to dig into exactly what's happening, where the flowid gets lost (or not set), and what the appropriate fix is. It may be that the correct solution is to calculate an actual hash (e.g. M_HASHTYPE_RSS_IPV4) in the network driver, rather than changing pf.

[nonesuch_longcount.org]

Kristof

I want to stop this patch. I can not find any way to make it trigger. I feel that this condition was added to openbsd only to deal with a potential edge case . I can not find any way to make it trigger.

[kp]

Commandeering so this can be abandoned.

[kp]

Abandon at the request of the submitter.

Mark, it may be useful to instrument (either with DTrace, or by adding printfs()) your network driver to work out when you do run into issues with flowid not being set.