| 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
| ||
|
Message-ID: <462E29D9.8000600@trash.net>
Date: Tue, 24 Apr 2007 18:01:29 +0200
From: Patrick McHardy <kaber@...sh.net>
To: James Chapman <jchapman@...alix.com>
CC: netdev@...r.kernel.org
Subject: Re: [PATCH 3/5 2.6.21-rc7] l2tp: pppol2tp core
James Chapman wrote:
> +static void pppol2tp_recv_dequeue(struct pppol2tp_session *session)
> +{
> + struct sk_buff *skb;
> + struct sk_buff *tmp;
> +
> + /* If the pkt at the head of the queue has the nr that we
> + * expect to send up next, dequeue it and any other
> + * in-sequence packets behind it.
> + */
> + spin_lock(&session->reorder_q.lock);
> + skb_queue_walk_safe(&session->reorder_q, skb, tmp) {
> + spin_unlock(&session->reorder_q.lock);
> +
> + if (time_after(jiffies, PPPOL2TP_SKB_CB(skb)->expires)) {
> + session->stats.rx_seq_discards++;
> + session->stats.rx_errors++;
> + PRINTK(session->debug, PPPOL2TP_MSG_SEQ, KERN_DEBUG,
> + "%s: oos pkt %hu len %d discarded (too old), "
> + "waiting for %hu, reorder_q_len=%d\n",
> + session->name, PPPOL2TP_SKB_CB(skb)->ns,
> + PPPOL2TP_SKB_CB(skb)->length, session->nr,
> + skb_queue_len(&session->reorder_q));
> + skb_unlink(skb, &session->reorder_q);
> + kfree_skb(skb);
Since you drop the lock above, what prevents this from running
concurrently on two CPUs and freeing the skb twice?
> +static int pppol2tp_recv_core(struct sock *sock, struct sk_buff *skb)
> +{
> + struct pppol2tp_session *session = NULL;
> + struct pppol2tp_tunnel *tunnel;
> + unsigned char *ptr;
> + u16 hdrflags;
> + u16 tunnel_id, session_id;
> + int length;
> +
> + tunnel = pppol2tp_sock_to_tunnel(sock);
> + if (tunnel == NULL)
> + goto error;
> +
> + /* Short packet? */
> + if (skb->len < sizeof(struct udphdr)) {
> + PRINTK(tunnel->debug, PPPOL2TP_MSG_DATA, KERN_INFO,
> + "%s: recv short packet (len=%d)\n", tunnel->name, skb->len);
> + goto error;
> + }
> +
> + /* Point to L2TP header */
> + ptr = skb->data + sizeof(struct udphdr);
> +
> + /* Get L2TP header flags */
> + hdrflags = ntohs(*(u16*)ptr);
__be16 here and elsewhere please.
> +/* The data_ready hook on the UDP socket. Scan the incoming packet list for
> + * packets to process. Only control or bad data packets are delivered to
> + * userspace.
> + */
> +static void pppol2tp_data_ready(struct sock *sk, int len)
> +{
> + struct pppol2tp_tunnel *tunnel;
> + struct sk_buff *skb;
> +
> + tunnel = pppol2tp_sock_to_tunnel(sk);
> + if (tunnel == NULL)
> + goto end;
> +
> + PRINTK(tunnel->debug, PPPOL2TP_MSG_DATA, KERN_DEBUG,
> + "%s: received %d bytes\n", tunnel->name, len);
> +
> + skb = skb_dequeue(&sk->sk_receive_queue);
> + if (skb != NULL) {
> + if (pppol2tp_recv_core(sk, skb)) {
> + skb_queue_head(&sk->sk_receive_queue, skb);
> + tunnel->old_data_ready(sk, len);
Still the ugly old_data_ready/old_sk_destruct and pppol2tp_fget hacks.
What prevents you from using encapsulation sockets to get rid of this
stuff and have ppp_generic filter out non-data frames for userspace
as for other ppp drivers?
> +static int pppol2tp_proc_open(struct inode *inode, struct file *file);
> +static int pppol2tp_proc_release(struct inode *inode, struct file *file);
> +static void *pppol2tp_seq_start(struct seq_file *m, loff_t *_pos);
> +static void *pppol2tp_seq_next(struct seq_file *p, void *v, loff_t *pos);
> +static void pppol2tp_seq_stop(struct seq_file *p, void *v);
> +static int pppol2tp_seq_show(struct seq_file *m, void *v);
Please avoid these by reordering the code.
> +/* Used by sort()
> + */
> +static void pppol2tp_seq_swap(void *a, void *b, int size)
> +{
> + struct pppol2tp_seq_data tmp;
> + struct pppol2tp_seq_data *left = a;
> + struct pppol2tp_seq_data *right = b;
> +
> + tmp = *left;
> + *left = *right;
> + *right = tmp;
> +}
> +
> +/* Used by sort()
> + */
> +static int pppol2tp_seq_cmp(const void *a, const void *b)
> +{
> + const struct pppol2tp_seq_data *left = a;
> + const struct pppol2tp_seq_data *right = b;
> +
> + if (left->tunnel_id < right->tunnel_id)
> + return -1;
> + if (left->tunnel_id > right->tunnel_id)
> + return 1;
> + if (left->session_id < right->session_id)
> + return -1;
> + if (left->session_id > right->session_id)
> + return 1;
> + return 0;
> +}
Why is this needed? Can't userspace sort them itself in case it really
needs some defined ordering?
> +static void pppol2tp_seq_tunnel_show(struct seq_file *m, void *v)
> +{
> + struct pppol2tp_tunnel *tunnel = v;
> +
> + seq_printf(m, "\nTUNNEL '%s', %c %d MAGIC %s\n",
> + tunnel->name,
> + (tunnel == tunnel->sock->sk_user_data) ? 'Y':'N',
> + atomic_read(&tunnel->ref_count) - 1,
> + (tunnel->magic == L2TP_TUNNEL_MAGIC) ? "OK" : "BAD");
> + seq_printf(m, " %08x %llu/%llu/%llu %llu/%llu/%llu\n",
> + tunnel->debug,
> + tunnel->stats.tx_packets, tunnel->stats.tx_bytes,
> + tunnel->stats.tx_errors,
> + tunnel->stats.rx_packets, tunnel->stats.rx_bytes,
> + tunnel->stats.rx_errors);
Still no return value checks.
-
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
Powered by blists - more mailing lists