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

[Patrick McHardy <kaber@trash.net>]

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.