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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 17 Aug 2010 07:19:53 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Grégoire Baron <baronchon@...m.org>
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>
Subject: Re: [PATCH] net/sched: add ACT_CSUM action to update packets
 checksums

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@...m.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...



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html