netdev - Re: [PATCH v4] PPTP: PPP over IPv4 (Point-to-Point Tunneling Protocol)

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]

Message-ID: <1282136626.2194.68.camel@edumazet-laptop>
Date:	Wed, 18 Aug 2010 15:03:46 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Dmitry Kozlov <xeb@...l.ru>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH v4] PPTP: PPP over IPv4 (Point-to-Point Tunneling
 Protocol)

Le mercredi 18 août 2010 à 16:30 +0400, Dmitry Kozlov a écrit :
> This patch contains:
> 1. pptp driver
> 2. gre demultiplexer driver for demultiplexing gre packets with different gre version
>    so ip_gre and pptp may coexists
> 3. ip_gre modification
> 4. other stuff
> 
> Changes from patch v3:
> 1. using rcu instead of read-write lock in gre module
> 2. fixed coding style issues
> 

Hmm. You left a synchronize_rcu() call in add_chan() but this is not
necessary. Please remove it or explain why you think it is needed.

synchronize_rcu(); in del_chan() is now fine, thanks.


You use a mutex for gre_proto_lock, but you dont need a mutex, please
use a spinlock like I suggested. I would have said 'use a mutex' if if
was needed, you can trust me.

A mutex is needed if you can sleep while holding the lock, this is not
the case here. A spinlock is faster and reduces to no-op on !SMP

Also, please remove the synchronize_rcu() call from gre_add_protocol() :
It is not needed at all, like in add_chan().

(You dont have to wait for a RCU grace period when adding something,
only when removing from a data structure)

You sometime have too many tabulations, check :



> +	{
> +		struct flowi fl = { .oif = 0,
> +				    .nl_u = {

ip4_u has one excess level
> +							.ip4_u = {
> +								.daddr = opt->dst_addr.sin_addr.s_addr,
> +								.saddr = opt->src_addr.sin_addr.s_addr,
> +								.tos = RT_TOS(0) } },
> +				    .proto = IPPROTO_GRE };
> +		err = ip_route_output_key(&init_net, &rt, &fl);
> +		if (err)
> +			goto tx_error;
> +	}
> +	tdev = rt->u.dst.dev;
> +
> +	max_headroom = LL_RESERVED_SPACE(tdev) + sizeof(*iph) + sizeof(*hdr) + 2;
> +
> +	

same on this if clause. why so many leading spaces ?

> +	if (
> +					/* version should be 1 */
> +					((header->ver & 0x7F) != PPTP_GRE_VER) ||
> +					/* PPTP-GRE protocol for PPTP */
> +					(ntohs(header->protocol) != PPTP_GRE_PROTO) ||
> +					/* flag C should be clear   */
> +					PPTP_GRE_IS_C(header->flags) ||
> +					/* flag R should be clear   */
> +					PPTP_GRE_IS_R(header->flags) ||
> +					/* flag K should be set     */
> +					(!PPTP_GRE_IS_K(header->flags)) ||
> +					/* routing and recursion ctrl = 0  */
> +					((header->flags&0xF) != 0))
> +					/* if invalid, discard this packet */
> +		goto drop;
> +
> +
> +	po = lookup_chan(htons(header->call_id), iph->saddr);

...

> +
> +	{
> +		struct flowi fl = {
> +				    .nl_u = {

ditto

> +							.ip4_u = {
> +								.daddr = opt->dst_addr.sin_addr.s_addr,
> +								.saddr = opt->src_addr.sin_addr.s_addr,
> +								.tos = RT_CONN_FLAGS(sk) } },
> +				    .proto = IPPROTO_GRE };
> +		security_sk_classify_flow(sk, &fl);
> +		if (ip_route_output_key(&init_net, &rt, &fl)) {
> +			error = -EHOSTUNREACH;
> +			goto end;
> +		}
> +		sk_setup_caps(sk, &rt->u.dst);
> +	}

Thanks


--
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