netdev - Re: [net-next PATCH 02/02] net/ipv4: VTI support new module for ip

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] [day] [month] [year] [list]

Message-Id: <20120708.234747.2210170159486826750.davem@davemloft.net>
Date:	Sun, 08 Jul 2012 23:47:47 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	saurabh.mohan@...tta.com
Cc:	netdev@...r.kernel.org
Subject: Re: [net-next PATCH 02/02] net/ipv4: VTI support new module for
 ip_vti.

From: Saurabh <saurabh.mohan@...tta.com>
Date: Thu, 28 Jun 2012 18:30:17 -0700

> +#define HASH_SIZE  16
> +#define HASH(addr) (((__force u32)addr^((__force u32)addr>>4))&0xF)

Define HASH such that it masks with (HASH_SIZE - 1) instead of
0xf, so that if HASH_SIZE is changed everything automatically
still works without having to remember to update the value in
HASH()'s definition too.

> +	if (skb->protocol != htons(ETH_P_IP))
> +		goto tx_error;

We are really past the point where we can add major inet protocol
features without supporting ipv6 as well.

> +	if (IS_ERR(rt)) {
> +		dev->stats.tx_carrier_errors++;
> +		goto tx_error_icmp;
> +	}
> +#ifdef CONFIG_XFRM
> +		/* if there is no transform then this tunnel is not functional.
> +		 * Or if the xfrm is not mode tunnel.
> +		 */
> +		if (!rt->dst.xfrm ||
> +		    rt->dst.xfrm->props.mode != XFRM_MODE_TUNNEL) {
> +			stats->tx_carrier_errors++;
> +			goto tx_error_icmp;
> +		}
> +#endif

This code in the CONFIG_XFRM block is not indented properly.

And this is a pointless CONFIG_* check, you can't even register
this tunnel outside of the XFRM code.  In fact the code already
depends upon INET_XFRM_MODE_TUNNEL which therefore automatically
means that CONFIG_XFRM must be set for this code.

> +	}
> +
> +
> +	if (tunnel->err_count > 0) {

Get rid of these extra blank lines.

> +	}
> +
> +
> +	IPCB(skb)->flags &= ~(IPSKB_XFRM_TUNNEL_SIZE | IPSKB_XFRM_TRANSFORMED |

Again.

The reason there are long periods of time between my attempts to
review your code (and probably the reason I'm the only person still
reviewing your work at all) is that I know there are going to be so
many problems to let you know about.  It's really painful to review
your work and I've spent so much time on the coding style and the
simpler issues that I really haven't considered the high level issues
of what your code is trying to do.
--
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