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

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

Le mercredi 18 août 2010 à 18:14 +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 v4:
> 1. using spinlock instead mutex
> 2. fixed coding style issues
> 

> +static int __init pptp_init_module(void)
> +{
> +	int err = 0;
> +	printk(KERN_INFO "PPTP driver version " PPTP_DRIVER_VERSION "\n");
> +
> +	if (gre_add_protocol(&gre_pptp_protocol, GREPROTO_PPTP) < 0) {
> +		printk(KERN_INFO "PPTP: can't add protocol\n");
> +		goto out;
> +	}
> +
> +	err = proto_register(&pptp_sk_proto, 0);
> +	if (err) {
> +		printk(KERN_INFO "PPTP: can't register sk_proto\n");
> +		goto out_inet_del_protocol;
> +	}
> +
> +	err = register_pppox_proto(PX_PROTO_PPTP, &pppox_pptp_proto);
> +	if (err) {
> +		printk(KERN_INFO "PPTP: can't register pppox_proto\n");
> +		goto out_unregister_sk_proto;
> +	}
> +
> +	callid_sock = (struct pppox_sock **)vmalloc((MAX_CALLID + 1) * sizeof(void *));
> +	memset(callid_sock, 0, (MAX_CALLID + 1) * sizeof(void *));
> +
> +out:
> +	return err;
> +out_unregister_sk_proto:
> +	proto_unregister(&pptp_sk_proto);
> +out_inet_del_protocol:
> +	gre_del_protocol(&gre_pptp_protocol, GREPROTO_PPTP);
> +	return err;
> +}



1) Please test return from vmalloc(), it can be NULL

Also, if you need to clear it, you might call 

__vmalloc(size, GFP_KERNEL | __GFP_ZERO, PAGE_KERNEL);

(and result is already cleared)


2) For new code, it's advised to use :

pr_err(xxx) instead of printk(KERN_ERR xxx)
pr_info(xxx) instead of printk(KERN_INFO xxx)


3) After skb_dst_drop(skb), you dont need to call skb_dst_set(skb,
NULL), as it is already done (for example in pptp_rcv())

4) Since you touch some includes, you also can use __packed
instead of __attribute__ ((__packed__))


5) I feel a bit uncomfortable with lookup_chan_dst()

+static int lookup_chan_dst(__u16 call_id, __be32 d_addr)
+{
+       struct pppox_sock *sock;
+       struct pptp_opt *opt;
+       int i;
+
+       rcu_read_lock();
+       for (i = find_next_bit(callid_bitmap, MAX_CALLID, 1); i < MAX_CALLID; i = find_next_bit(callid_bitmap, MAX_CALLID, i + 1)) {

Split this too long line please, its really awful

+               sock = rcu_dereference(callid_sock[i]);
+               opt = &sock->proto.pptp;
+               if (opt->dst_addr.call_id == call_id && opt->dst_addr.sin_addr.s_addr == d_addr)
+                       break;
+       }
+       rcu_read_unlock();
+
+       return i < MAX_CALLID;
+}

Once you get a bit in bitmask, and rcu_dereference(callid_sock[i]),
there is no guarantee sock is not NULL.

You should add a test.
If sock is not NULL, then you have the guarantee (thanks to RCU) that
pointer is safe until the rcu_read_unlock()


Thanks