Second try. Fixes (thank you, Henning, for fast, actual comments):

  - Old and new pfsync should now co-operate smoothly, assuming you
    do not use routing options on "match" rules until all firewalls
    got updated. If you will not, you'll shoot yourself in the foot:
    stats and traffic may get screwed, not checked if that may
    cause panics. Again: pfsync was NOT tested in real environment.

  - More style(9) compliance. Not sure about struct pf_state change,
    though: will it ever break on architectures with 4-byte pointers
    but 64-bit alignment requirements, if there are any?.. Sorry for
    possibly stupid question.

(Sorry if this'll start a new thread for you, I'm playing with
making smtpd and GMail friends).

Index: sys/net/if_pflog.c
===================================================================
RCS file: /cvs/src/sys/net/if_pflog.c,v
retrieving revision 1.32
diff -u -p -r1.32 if_pflog.c
--- sys/net/if_pflog.c	21 Sep 2010 22:49:14 -0000	1.32
+++ sys/net/if_pflog.c	16 Nov 2010 01:33:03 -0000
@@ -212,7 +212,7 @@ pflogioctl(struct ifnet *ifp, u_long cmd
 int
 pflog_packet(struct pfi_kif *kif, struct mbuf *m, sa_family_t af, u_int8_t dir,
     u_int8_t reason, struct pf_rule *rm, struct pf_rule *am,
-    struct pf_ruleset *ruleset, struct pf_pdesc *pd)
+    struct pf_rule *rtr, struct pf_ruleset *ruleset, struct pf_pdesc *pd)
 {
 #if NBPFILTER > 0
 	struct ifnet *ifn;
@@ -241,6 +241,10 @@ pflog_packet(struct pfi_kif *kif, struct
 			strlcpy(hdr.ruleset, ruleset->anchor->name,
 			    sizeof(hdr.ruleset));
 	}
+	if (rtr == NULL)
+		hdr.rt_rulenr = htonl(-1);
+	else
+		hdr.rt_rulenr = htonl(rtr->nr);
 	if (rm->log & PF_LOG_SOCKET_LOOKUP && !pd->lookup.done)
 		pd->lookup.done = pf_socket_lookup(dir, pd);
 	if (pd->lookup.done > 0) {
Index: sys/net/if_pflog.h
===================================================================
RCS file: /cvs/src/sys/net/if_pflog.h,v
retrieving revision 1.17
diff -u -p -r1.17 if_pflog.h
--- ...
[ message continues ]

On 2010/11/16 04:50, Vadim Zhukov wrote:
[..]

Great stuff, this has annoyed me for a while :) I can't test today but

Afaik the strictest requirements on any arch we support is that
a data type must be aligned to the size of that data type. 32-bit

I might be mistaken but I think this will break compatibility with
the pfsync wire format used by earlier versions.

[ message continues ]

No, it's struct pfsync_state that shows on the wire. You don't think
that pointers being used in pfsync messages, do you? :)

--
  WBR,
  Vadim Zhukov

[ message continues ]

Then the snippet should become:

@@ -807,6 +808,8 @@ struct pf_state {
        struct pf_state_key     *key[2];        /* addresses stack and wire
*/
        struct pfi_kif          *kif;
        struct pfi_kif          *rt_kif;
+       struct pf_rule          *rt_rule;
+       caddr_t                  align;
        u_int64_t                packets[2];
        u_int64_t                bytes[2];
        u_int32_t                creation;

Am I right?

--
  WBR,
  Vadim Zhukov

[ message continues ]

I don't think so. It's the compiler's job to align the struct fields
properly. 

As long as you do not access the field using pointer tricks you're
safe. I do not know if pf is doing that. Another reason to do "manual"
layout is when the struct is used on the wire.

There is a guideline to lay out structs starting with the biggest
field and then working to smaller ones. This guideline is ambiguous
since the size of fields is depedent on the target. Still, some effort
wrt this is preferred since it makes sure the compiler has to insert
padding only in rare circumstances. 


	-Otto

[ message continues ]

you are - we don't send pf_state over the wire any more but use a
seperate pfsync_state.

-- 
Henning Brauer, hb@bsws.de, henning@openbsd.org
BS Web Services, http://bsws.de
Full-Service ISP - Secure Hosting, Mail and DNS Services
Dedicated Servers, Rootservers, Application Hosting

[ message continues ]