Re: [PATCH] net/sched: add ACT_CSUM action to update packets checksums

[Eric Dumazet <eric.dumazet@gmail.com>]

Le lundi 16 août 2010 à 23:15 +0200, Grégoire Baron a écrit :
> net/sched: add ACT_CSUM action to update packets checksums
> 
> ACT_CSUM can be called just after ACT_PEDIT in order to re-compute some
> altered checksums in IPv4 and IPv6 packets. The following checksums are
> supported by this patch:
>  - IPv4: IPv4 header, ICMP, IGMP, TCP, UDP & UDPLite
>  - IPv6: ICMPv6, TCP, UDP & UDPLite
> It's possible to request in the same action to update different kind of
> checksums, if the packets flow mix TCP, UDP and UDPLite, ...
> 
> An example of usage is done in the associated iproute2 patch.
> 
> Signed-off-by: Gregoire Baron <baronchon@n7mm.org>

Impressive :)

One style note :

> +
> +	switch (iph->frag_off & htons(IP_OFFSET) ? 0 : iph->protocol) {
> +	case IPPROTO_ICMP:
> +	{
> +		if (update_flags & TCA_CSUM_UPDATE_FLAG_ICMP)
> +			if (!tcf_csum_ipv4_icmp(skb, iph,
> +						iph->ihl * 4, ntohs(iph->tot_len)))
> +				goto fail;
> +		break;
> +	}
> +	case IPPROTO_IGMP:
> +	{
> +		if (update_flags & TCA_CSUM_UPDATE_FLAG_IGMP)
> +			if (!tcf_csum_ipv4_igmp(skb, iph,
> +						iph->ihl * 4, ntohs(iph->tot_len)))
> +				goto fail;
> +		break;
> +	}

You add extra block delimiters (thus two lines) per switch cases, while
not necessary. Please remove them.

And one note about tcf_csum_dump()

static int tcf_csum_dump(struct sk_buff *skb,
                        struct tc_action *a, int bind, int ref)
{
       unsigned char *b = skb_tail_pointer(skb);
       struct tcf_csum *p = a->priv;
       struct tc_csum *opt;
       struct tcf_t t;
       int s;

       s = sizeof(*opt);

       /* netlink spinlocks held above us - must use ATOMIC */
       opt = kzalloc(s, GFP_ATOMIC);
       if (unlikely(!opt))
               return -ENOBUFS;

Please dont use kzalloc() here for such a small variable (24 bytes), use
an automatic one (on stack)

struct tc_csum parms = {
	.update_flags = p->update_flags,
	.index = p->tcf_index,
	.action = p->tcf_action,
	.refcnt = p->tcf_refcnt - ref,
	.bindcnt = p->tcf_bindcnt - bind,
};

(Using such a construct make sure holes are zero filled, thus we dont
leak kernel memory to user)

NLA_PUT(skb, TCA_CSUM_PARMS, sizeof(parms), &parms);


Hmm, I can see existing code have some leaks, I'll post a separate
patch...