Re: kern/53962: npf: weird 'stateful' behavior

[Timo Buhrmester <fstd.lkml%gmail.com@localhost>]

The following reply was made to PR kern/53962; it has been noted by GNATS.

From: Timo Buhrmester <fstd.lkml%gmail.com@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: rmind%netbsd.org@localhost
Subject: Re: kern/53962: npf: weird 'stateful' behavior
Date: Thu, 14 Feb 2019 02:28:27 +0100

 > So that's a workaround I can live with for now
 Turns out that workaround doesn't play nice with NAT.
 
 I've "fixed" things for now by making the interface identifier part
 of the connection key so as to avoid identical rules on different
 interfaces colliding.
 
 Cheers
 Timo
 
 diff --git a/sys/net/npf/npf_conn.c b/sys/net/npf/npf_conn.c
 index 3557132b1f1a..023f845964f0 100644
 --- a/sys/net/npf/npf_conn.c
 +++ b/sys/net/npf/npf_conn.c
 @@ -238,7 +238,7 @@ npf_conn_trackable_p(const npf_cache_t *npc)
  
  static uint32_t
  connkey_setkey(npf_connkey_t *key, uint16_t proto, const void *ipv,
 -    const uint16_t *id, unsigned alen, bool forw)
 +    const uint16_t *id, unsigned alen, bool forw, uint32_t ifid)
  {
  	uint32_t isrc, idst, *k = key->ck_key;
  	const npf_addr_t * const *ips = ipv;
 @@ -263,22 +263,23 @@ connkey_setkey(npf_connkey_t *key, uint16_t proto, const void *ipv,
  
  	k[0] = ((uint32_t)proto << 16) | (alen & 0xffff);
  	k[1] = ((uint32_t)id[isrc] << 16) | id[idst];
 +	k[2] = ifid;
  
  	if (__predict_true(alen == sizeof(in_addr_t))) {
 -		k[2] = ips[isrc]->word32[0];
 -		k[3] = ips[idst]->word32[0];
 +		k[3] = ips[isrc]->word32[0];
 +		k[4] = ips[idst]->word32[0];
  		return 4 * sizeof(uint32_t);
  	} else {
  		const u_int nwords = alen >> 2;
 -		memcpy(&k[2], ips[isrc], alen);
 -		memcpy(&k[2 + nwords], ips[idst], alen);
 +		memcpy(&k[3], ips[isrc], alen);
 +		memcpy(&k[3 + nwords], ips[idst], alen);
  		return (2 + (nwords * 2)) * sizeof(uint32_t);
  	}
  }
  
  static void
  connkey_getkey(const npf_connkey_t *key, uint16_t *proto, npf_addr_t *ips,
 -    uint16_t *id, uint16_t *alen)
 +    uint16_t *id, uint16_t *alen, uint32_t *ifid)
  {
  	const uint32_t *k = key->ck_key;
  
 @@ -286,12 +287,14 @@ connkey_getkey(const npf_connkey_t *key, uint16_t *proto, npf_addr_t *ips,
  	*alen = k[0] & 0xffff;
  	id[NPF_SRC] = k[1] >> 16;
  	id[NPF_DST] = k[1] & 0xffff;
 +	if (ifid)
 +		*ifid = k[2];
  
  	switch (*alen) {
  	case sizeof(struct in6_addr):
  	case sizeof(struct in_addr):
 -		memcpy(&ips[NPF_SRC], &k[2], *alen);
 -		memcpy(&ips[NPF_DST], &k[2 + ((unsigned)*alen >> 2)], *alen);
 +		memcpy(&ips[NPF_SRC], &k[3], *alen);
 +		memcpy(&ips[NPF_DST], &k[3 + ((unsigned)*alen >> 2)], *alen);
  		return;
  	default:
  		KASSERT(0);
 @@ -345,14 +348,14 @@ npf_conn_conkey(const npf_cache_t *npc, npf_connkey_t *key, const bool forw)
  		/* Unsupported protocol. */
  		return 0;
  	}
 -	return connkey_setkey(key, proto, npc->npc_ips, id, alen, forw);
 +	return connkey_setkey(key, proto, npc->npc_ips, id, alen, forw, npc->npc_nbuf->nb_ifid);
  }
  
  static __inline void
  connkey_set_addr(npf_connkey_t *key, const npf_addr_t *naddr, const int di)
  {
  	const u_int alen = key->ck_key[0] & 0xffff;
 -	uint32_t *addr = &key->ck_key[2 + ((alen >> 2) * di)];
 +	uint32_t *addr = &key->ck_key[3 + ((alen >> 2) * di)];
  
  	KASSERT(alen > 0);
  	memcpy(addr, naddr, alen);
 @@ -945,18 +948,21 @@ static prop_dictionary_t
  npf_connkey_export(const npf_connkey_t *key)
  {
  	uint16_t id[2], alen, proto;
 +	uint32_t ifid;
  	prop_dictionary_t kdict;
  	npf_addr_t ips[2];
  	prop_data_t d;
  
  	kdict = prop_dictionary_create();
 -	connkey_getkey(key, &proto, ips, id, &alen);
 +	connkey_getkey(key, &proto, ips, id, &alen, &ifid);
  
  	prop_dictionary_set_uint16(kdict, "proto", proto);
  
  	prop_dictionary_set_uint16(kdict, "sport", id[NPF_SRC]);
  	prop_dictionary_set_uint16(kdict, "dport", id[NPF_DST]);
  
 +	prop_dictionary_set_uint32(kdict, "ifid", ifid);
 +
  	d = prop_data_create_data(&ips[NPF_SRC], alen);
  	prop_dictionary_set_and_rel(kdict, "saddr", d);
  
 @@ -1007,6 +1013,7 @@ npf_connkey_import(prop_dictionary_t kdict, npf_connkey_t *key)
  	prop_object_t sobj, dobj;
  	npf_addr_t const * ips[2];
  	uint16_t alen, proto, id[2];
 +	uint32_t ifid;
  
  	if (!prop_dictionary_get_uint16(kdict, "proto", &proto))
  		return 0;
 @@ -1017,6 +1024,9 @@ npf_connkey_import(prop_dictionary_t kdict, npf_connkey_t *key)
  	if (!prop_dictionary_get_uint16(kdict, "dport", &id[NPF_DST]))
  		return 0;
  
 +	if (!prop_dictionary_get_uint32(kdict, "ifid", &ifid))
 +		return 0;
 +
  	sobj = prop_dictionary_get(kdict, "saddr");
  	if ((ips[NPF_SRC] = prop_data_data_nocopy(sobj)) == NULL)
  		return 0;
 @@ -1029,7 +1039,7 @@ npf_connkey_import(prop_dictionary_t kdict, npf_connkey_t *key)
  	if (alen != prop_data_size(dobj))
  		return 0;
  
 -	return connkey_setkey(key, proto, ips, id, alen, true);
 +	return connkey_setkey(key, proto, ips, id, alen, true, ifid);
  }
  
  /*
 diff --git a/sys/net/npf/npf_conn.h b/sys/net/npf/npf_conn.h
 index debb27e22ee6..2023ec787ec4 100644
 --- a/sys/net/npf/npf_conn.h
 +++ b/sys/net/npf/npf_conn.h
 @@ -47,9 +47,9 @@ typedef struct npf_connkey npf_connkey_t;
  /*
   * See npf_conn_conkey() function for the key layout description.
   */
 -#define	NPF_CONN_NKEYWORDS	(2 + ((sizeof(npf_addr_t) * 2) >> 2))
 +#define	NPF_CONN_NKEYWORDS	(3 + ((sizeof(npf_addr_t) * 2) >> 2))
  #define	NPF_CONN_GETALEN(key)	((key)->ck_key[0] & 0xffff)
 -#define	NPF_CONN_KEYLEN(key)	(8 + (2 * NPF_CONN_GETALEN(key)))
 +#define	NPF_CONN_KEYLEN(key)	(12 + (2 * NPF_CONN_GETALEN(key)))
  
  struct npf_connkey {
  	/* Entry node and back-pointer to the actual connection. */