On Wed, Jan 17, 2018 at 11:25 AM, David Miller <da...@davemloft.net> wrote: > From: James Chapman <jchap...@katalix.com> > Date: Wed, 17 Jan 2018 11:13:33 +0000 > >> On 16 January 2018 at 19:00, David Miller <da...@davemloft.net> wrote: >>> From: Tom Herbert <t...@herbertland.com> >>> Date: Tue, 16 Jan 2018 09:36:41 -0800 >>> >>>> sk_user_data is set with the sk_callback lock held in code below. >>>> Should be able to take the lock earlier can do this check under the >>>> lock. >>> >>> csock, and this csk, is obtained from an arbitrary one of the >>> process's FDs. It can be any socket type or family, and that socket's >>> family might set sk_user_data without the callback lock. >>> >>> The only socket type check is making sure it is not another PF_KCM >>> socket. So that doesn't help with this problem. >> >> Is it the intention to update all socket code over time to write >> sk_user_data within the sk_callback lock? If so, I'm happy to address >> that in the l2tp code (and update the kcm patch to check sk_user_data >> within the sk_callback lock). Or is the preferred solution to restrict >> KCM to specific socket families, as suggested by Guillaume earlier in >> the thread? > > I think we have a more fundamental issue here. > > sk->sk_user_data is a place where RPC layer specific data is hung off > of. By this definition SunRPC, RXRPC, RDS, TIPC, and KCM are all > using it correctly. > > Phonet has a similar issue to the one seen here, it tests and changes > sk_user_data under lock_sock(). The only requirement it makes is > that the socket type is not SOCK_STREAM. However, this one might be OK > since only pep_sock sockets can be passed down into gprs_attach(). > > Most of these cases like SunRPC, RXRPC, etc. are fine because they > only graft on top of TCP and UDP sockets. > > The weird situation here is that L2TP does tunneling and stores it's > private state in sk->sk_user_data like an RPC layer would. And KCM > allows basically any socket type to be attached. > > The RPC layers create their sockets internally, so I cannot see a way > that those can be sent to a KCM attach operations. And I think that > is why this RPC invariant is important for sk_user_data usage. > > If all else was equal, even though it doesn't make much sense to KCM > attach L2TP sockets to KCM, I would suggest to change L2TP to store > it's private stuff elsewhere. > > But that is not the case. Anything using the generic UDP > encapsulation layer is going to make use of sk->sk_user_data like this > (see setup_udp_tunnel_sock). > > It looks like over time we've accumulated this new class of uses > of sk->sk_user_data, ho hum... > > And it's not like we can add a test to KCM to avoid these socket > types, because they will look like normal UDP datagram sockets. > > What a mess... > > Furthermore, even if you add a test to KCM, you will now need to > add the same test to L2TP and anything else which uses sk_user_data > for tunneling and for which userspace has access to the socket fd. > > And it will be racy, indeed, until all such users align to the same > precise locking scheme for tests and updates to sk_user_data. > > Again, what a mess... > It's not so surprising that sk_user_data is being used for so many purposes, it's quite a powerful and useful notion. So, to a large extent I think it's a victim of it's own success.
Aligning to one locking scheme is the first task to clean this. The second would be how to deal with multiple simulataneous use on a socket (or maybe not allow). I've thought about having a chain of sk_user_data, but that's only useful is the write/read callback are also chained. All this starts to look like STREAMS at some point ;-) Tom
