[nftables RFC PATCH 0/1] implementing icmp code filtering

[nftables RFC PATCH 0/1] implementing icmp code filtering

[Eric Leblond]

Hello,

I've started to work on completing the reject implementation. I've
got a decent patchset but one specific point in my current work
is not valid for me.

There is multiple ICMP code lists depending on the value of the type. So
the straightforward implementation updating proto_icmp has some issue:

 const struct proto_desc proto_icmp = {
 	.name		= "icmp",
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.templates	= {
 		[ICMPHDR_TYPE]		= ICMPHDR_TYPE("type", &icmp_type_type, type),
-		[ICMPHDR_CODE]		= ICMPHDR_FIELD("code", code),
+		[ICMPHDR_CODE]		= ICMPHDR_TYPE("code", &icmp_code_type, code),
 
We have only one icmp_code_type symbo_table possible. For the userspace
to kernel way this is not an issue. As matching on the key name will
lead to a single value. Reverse order is not working as we need to know
the context (here the type) for converting the code to a name.

A possible solution could be to declare a subtype in the symbol_table
that could be used to set the context. For example, we could do
something like:

+static const struct symbol_table icmp_code_tbl = {
+       .symbols        = {
+               SYMBOL_WITH_SUBTYPE("network-unreachable",           ICMP_NET_UNREACH, TYPE_ICMP_CODE_REJECT),

It we set the type to TYPE_ICMP_CODE_REJECT when sending the netlink
message to kernel, then when reading back from kernel we will know
what translation to use.

An alternative approach is to use multiple symbol_table and then create
a structure aggregating the different symbol_table in one object that can
be given as parameters to ICMPHDR_TYPE.

What do you think ? Any better idea ?

BR,
--
Eric

[nftables RFC PATCH] icmp: add code filtering with name

[Eric Leblond]

This patch define names for code field allowing to use string
instead of integer when filtering on icmp code.

Signed-off-by: Eric Leblond <eric@regit.org>
---
 include/datatype.h |  2 ++
 src/proto.c        | 43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/include/datatype.h b/include/datatype.h
index 9e609cf..033ae08 100644
--- a/include/datatype.h
+++ b/include/datatype.h
@@ -34,6 +34,7 @@
  * @TYPE_CT_DIR:	conntrack direction
  * @TYPE_CT_STATUS:	conntrack status (bitmask subtype)
  * @TYPE_ICMP6_TYPE:	ICMPv6 type codes (integer subtype)
+ * @TYPE_ICMP_CODE:	ICMP code codes (integer subtype)
  */
 enum datatypes {
 	TYPE_INVALID,
@@ -66,6 +67,7 @@ enum datatypes {
 	TYPE_CT_DIR,
 	TYPE_CT_STATUS,
 	TYPE_ICMP6_TYPE,
+	TYPE_ICMP_CODE,
 	__TYPE_MAX
 };
 #define TYPE_MAX		(__TYPE_MAX - 1)
diff --git a/src/proto.c b/src/proto.c
index cc073af..bb56766 100644
--- a/src/proto.c
+++ b/src/proto.c
@@ -315,12 +315,52 @@ static const struct datatype icmp_type_type = {
 #define ICMPHDR_TYPE(__name, __type, __member) \
 	HDR_TYPE(__name, __type, struct icmphdr, __member)
 
+static const struct symbol_table icmp_code_tbl = {
+	.symbols	= {
+		SYMBOL("network-unreachable",		ICMP_NET_UNREACH),
+		SYMBOL("host-unreachable",		ICMP_HOST_UNREACH),
+		SYMBOL("protocol-unreachable",		ICMP_PROT_UNREACH),
+		SYMBOL("port-unreachable",		ICMP_PORT_UNREACH),
+		SYMBOL("fragmentation-needed",		ICMP_FRAG_NEEDED),
+		SYMBOL("source-route-failed",		ICMP_SR_FAILED),
+		SYMBOL("network-unknown",		ICMP_NET_UNKNOWN),
+		SYMBOL("host-unknown",			ICMP_HOST_UNKNOWN),
+		SYMBOL("host-isolated",			ICMP_HOST_ISOLATED),
+		SYMBOL("network-prohibited",		ICMP_NET_ANO),
+		SYMBOL("host-prohibited",		ICMP_HOST_ANO),
+		SYMBOL("TOS-network-unreachable",	ICMP_NET_UNR_TOS),
+		SYMBOL("TOS-host-unreachable",		ICMP_HOST_UNR_TOS),
+		SYMBOL("communication-prohibited",	ICMP_PKT_FILTERED),
+		SYMBOL("host-precedence-violation",	ICMP_PREC_VIOLATION),
+		SYMBOL("precedence-cutoff",		ICMP_PREC_CUTOFF),
+
+		SYMBOL("network-redirect",		ICMP_REDIR_NET),
+		SYMBOL("host-redirect",			ICMP_REDIR_HOST),
+		SYMBOL("network-redirect-tos",		ICMP_REDIR_NETTOS),
+		SYMBOL("host-redirect-tos",		ICMP_REDIR_HOSTTOS),
+
+		SYMBOL("ttl-exceeded",			ICMP_EXC_TTL),
+		SYMBOL("fragtime-exceeded",		ICMP_EXC_FRAGTIME),
+		SYMBOL_LIST_END
+	},
+};
+
+static const struct datatype icmp_code_type = {
+	.type		= TYPE_ICMP_CODE,
+	.name		= "icmp_code",
+	.desc		= "ICMP code",
+	.byteorder	= BYTEORDER_BIG_ENDIAN,
+	.size		= BITS_PER_BYTE,
+	.basetype	= &integer_type,
+	.sym_tbl	= &icmp_code_tbl,
+};
+
 const struct proto_desc proto_icmp = {
 	.name		= "icmp",
 	.base		= PROTO_BASE_TRANSPORT_HDR,
 	.templates	= {
 		[ICMPHDR_TYPE]		= ICMPHDR_TYPE("type", &icmp_type_type, type),
-		[ICMPHDR_CODE]		= ICMPHDR_FIELD("code", code),
+		[ICMPHDR_CODE]		= ICMPHDR_TYPE("code", &icmp_code_type, code),
 		[ICMPHDR_CHECKSUM]	= ICMPHDR_FIELD("checksum", checksum),
 		[ICMPHDR_ID]		= ICMPHDR_FIELD("id", un.echo.id),
 		[ICMPHDR_SEQ]		= ICMPHDR_FIELD("sequence", un.echo.sequence),
@@ -806,6 +846,7 @@ const struct proto_desc proto_eth = {
 static void __init proto_init(void)
 {
 	datatype_register(&icmp_type_type);
+	datatype_register(&icmp_code_type);
 	datatype_register(&tcp_flag_type);
 	datatype_register(&dccp_pkttype_type);
 	datatype_register(&arpop_type);
-- 
1.9.0.rc3

Re: [nftables RFC PATCH 0/1] implementing icmp code filtering

[Patrick McHardy]

On Mon, Feb 10, 2014 at 01:33:24PM +0100, Eric Leblond wrote:
> 
> Hello,
> 
> I've started to work on completing the reject implementation. I've
> got a decent patchset but one specific point in my current work
> is not valid for me.
> 
> There is multiple ICMP code lists depending on the value of the type. So
> the straightforward implementation updating proto_icmp has some issue:
> 
>  const struct proto_desc proto_icmp = {
>  	.name		= "icmp",
>  	.base		= PROTO_BASE_TRANSPORT_HDR,
>  	.templates	= {
>  		[ICMPHDR_TYPE]		= ICMPHDR_TYPE("type", &icmp_type_type, type),
> -		[ICMPHDR_CODE]		= ICMPHDR_FIELD("code", code),
> +		[ICMPHDR_CODE]		= ICMPHDR_TYPE("code", &icmp_code_type, code),
>  
> We have only one icmp_code_type symbo_table possible. For the userspace
> to kernel way this is not an issue. As matching on the key name will
> lead to a single value. Reverse order is not working as we need to know
> the context (here the type) for converting the code to a name.
> 
> A possible solution could be to declare a subtype in the symbol_table
> that could be used to set the context. For example, we could do
> something like:
> 
> +static const struct symbol_table icmp_code_tbl = {
> +       .symbols        = {
> +               SYMBOL_WITH_SUBTYPE("network-unreachable",           ICMP_NET_UNREACH, TYPE_ICMP_CODE_REJECT),
> 
> It we set the type to TYPE_ICMP_CODE_REJECT when sending the netlink
> message to kernel, then when reading back from kernel we will know
> what translation to use.
> 
> An alternative approach is to use multiple symbol_table and then create
> a structure aggregating the different symbol_table in one object that can
> be given as parameters to ICMPHDR_TYPE.
> 
> What do you think ? Any better idea ?

I think we should use per-type code tables and complete the type during
evaluation. Basically once we see a "icmp type unreachable" we complete
the type of the "icmp code" expression (if any) to icmp_unreachable_code.

We do something similar in ct_expr_update_type() for ct expressions.