[Swan-dev] pfkey2 extension sets

[D. Hugh Redelmeier]

PF_Key messages are made up of lumps called "extensions".

Each extension has a type which is essentially a value from an enum 
sadb_extension_t (defined in linux/include/libreswan/pfkeyv2.h).

A comment before the definition of that enum says:

 * NOTE that there is a limit of 31 extensions due to current implementation
 * in pfkeyv2_ext_bits.c

It means that there is a limit on "extension types", not extensions.  I'll 
go fix that...

But:

1) in linux/net/ipsec/pfkey_v2_ext_bits.c, the key table has entries of 
type pfkey_ext_track which is used to represent a set of extension types 
(a mechanism similar to lset_t in Pluto).

2) in linux/include/libreswan/pfkey.h, pfkey_ext_track is typedefed to
be the type uint64_t.

So: the limit mentioned in the comment would appear to be 64, not 32.

But:

The table in linux/net/ipsec/pfkey_v2_ext_bits.c is initialized with
expressions like
                 	1ULL << SADB_EXT_RESERVED |
                                1ULL << SADB_EXT_ADDRESS_SRC |
			...
whereas this would be more correct:
                 	(pfkey_ext_track)1 << SADB_EXT_RESERVED |
                                (pfkey_ext_track)1 << SADB_EXT_ADDRESS_SRC |
			...

			(pfkey_ext_track)1

===> the pfkey code should have and use a macro like LELEM

But:

Still other places compute a set from an element like this:
	1 << SADB_EXT_SA
Example: linux/net/ipsec/pfkey_v2_parse.c.
Notice that this is a signed int expression: not very sensible for a
set.

But:

Formatted printing of these set values seems to cast them to unsigned
long long to format them.  This may or may not be a 64-bit type.  Why
not use PRIx64 for the format?  Oh and give it a name, defined
adjacent to pfkey_ext_track, so that the two definitions can be
updated in concert.

==> I suspect that 32 bits are actually enough.  But I haven't checked
very carefully.

Oh, and SADB_EXT_SA is defined in
	include/linux/pfkeyv2.h
whereas K_SADB_EXT_SA is defined in
	linux/include/libreswan/pfkeyv2.h
to be the same value.  Luckily, by reference.  This pattern holds for
all extension types.

==> why do we have two sets of symbols for the same values?

==> why do we have two headers with the same basename?

The two sets of names are NOT co-extensive.  They differ from 20 on.

#define SADB_X_EXT_NAT_T_TYPE           20
        K_SADB_X_EXT_ADDRESS_DST2=  20,
        K_SADB_X_EXT_NAT_T_TYPE=    27,

==> how did this mess happen?  Can we fix it?

And another thing.  Lots of expressions on the sets seem to be
awkward.

Example (notice odd use of arithmentic "-" as a set operation):
	(unsigned long long)(1 << SADB_EXT_SA) -
		(extensions_seen & (1 << SADB_EXT_SA))
  better:
	LELEM(SADB_EXT_SA) & ~extensions_seen

Example:
	(unsigned long long)K_SADB_X_EXT_ADDRESS_DELFLOW -
		(extensions_seen & K_SADB_X_EXT_ADDRESS_DELFLOW),
  better:
	K_SADB_X_EXT_ADDRESS_DELFLOW & ~extensions_seen

Example:
	extensions_seen =  1;
  better:
	extensions_seen = LELEM(K_SADB_EXT_RESERVED);

And another thing.
#define SADB_RESERVED	    0

	K_SADB_RESERVED=SADB_RESERVED,

	K_SADB_EXT_RESERVED=SADB_RESERVED,

#define SADB_EXT_RESERVED		0

=> surely K_SADB_EXT_RESERVED should be defined as SADB_EXT_RESERVED,
not SADB_RESERVED.  I'll fix that..

And I've only started looking.