Re: [PATCH 1/4] Fix expectaction mask dumping, take #3

[Patrick McHardy <kaber@trash.net>]

Pablo Neira Ayuso wrote:
> [CTNETLINK] Fix expectaction mask dumping
> 
> The expectation mask has some particularities that make handle in a
> different way. The protocol number fields can be set to non-valid
> protocols, ie. l3num is set to 0xFFFF. Since that protocol does not
> exist, the mask tuple will not be dumped. Moreover, this results in a
> kernel panic when nf_conntrack accesses the array of protocol handlers,
> that is PF_MAX (0x1F) long.
> 
> This patch introduces the function ctnetlink_exp_dump_mask, that
> correctly dumps the expectation mask. Such function uses the l3num value
> from the expectation tuple that is a valid layer 3 protocol number.

This part looks fine to me, apart from one minor nitpick :)

> Besides, this modification introduces the attribute CTA_IP_L3NUM.
> Although the layer 3 protocol information is sent in the nfnetlink
> header, if the message contains information about an expectation, it
> will contain information about the master conntrack (just one of the
> tuples), the expectation tuple and the expectation mask. In this case,
> the value of l3num in the expectation mask is not the same that is set
> in the nfnetlink message. That is why we need another field that contain
> the value of l3num.

I'm not sure I understand. The new attribute still contains the same
value as the netlink header, doesn't it? So userspace should currently
have at least two possibilities to get the correct value:

- use the value from the netlink header
- use the value from the tuple that comes with the mask, as the first
  part of your patch does. This seems most logically to me since the
  mask and the tuple belong together.

> @@ -77,33 +75,47 @@ nfattr_failure:
>  }
>  
>  static inline int
> -ctnetlink_dump_tuples(struct sk_buff *skb, 
> -		      const struct nf_conntrack_tuple *tuple)
> +ctnetlink_dump_tuples_ip(struct sk_buff *skb,
> +			 const struct nf_conntrack_tuple *tuple,
> +			 struct nf_conntrack_l3proto *l3proto)
>  {
> -	struct nfattr *nest_parms;
> -	struct nf_conntrack_l3proto *l3proto;
>  	int ret = 0;
> -	
> -	l3proto = nf_ct_l3proto_find_get(tuple->src.l3num);
> -	
> -	nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
> +	struct nfattr *nest_parms = NFA_NEST(skb, CTA_TUPLE_IP);
> +	u_int8_t l3num = (u_int8_t)tuple->src.l3num;

The cast is unnecessary.