netdev - Re: [PATCH 1/5 2.6.21-rc4] l2tp: pppol2tp core

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:	Sat, 24 Mar 2007 15:03:44 +0100
From:	Patrick McHardy <kaber@...sh.net>
To:	James Chapman <jchapman@...alix.com>
CC:	netdev@...r.kernel.org
Subject: Re: [PATCH 1/5 2.6.21-rc4] l2tp: pppol2tp core

James Chapman wrote:
> [PPPOL2TP]: Add PPP-over-L2TP driver core.
> 
> This driver handles only L2TP data frames; control frames are handled
> by a userspace application. The dfriver implements L2TP using the
> PPPoX socket family. Data is sent or received using regular socket
> sendmsg() / recvmsg() calls. Kernel parameters of the socket can be
> read or modified using ioctl() or [gs]etsockopt() calls.


The interaction with UDP sockets looks pretty horrible IMO. On the
send side I don't see why you can't simply build the UDP header
yourself instead of doing these set_fs + sendmsgs hacks. On the
receive side I it would be nice if you could use the encapsulation
socket stuff thats also used by IPsec.

A couple of random comments:

- your list locking is broken
- list_for_each_entry_safe is only needed if you remove something
  while iterating, its no replacement for locking
- SOCK_2_SESSION/SOCK_2_TUNNEL are terribly ugly and should
  probably be inline functions that use BUG_ON in case of an
  invalid magic
- You should use skb_queue_walk for queue walking
- You should use endian-annotated types
- pppol2tp_fget: why do you want to open sockets for other processes?
  I hope this can go together with the sendmsg hacks

-
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