Date:   Thu, 18 Jan 2018 15:40:52 +0000
From:   James Chapman <jchapman@...alix.com>
To:     Guillaume Nault <g.nault@...halink.fr>,
        David Miller <davem@...emloft.net>
Cc:     Tom Herbert <tom@...bertland.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] kcm: do not attach sockets if sk_user_data is
 already used

On 18 January 2018 at 15:18, Guillaume Nault <g.nault@...halink.fr> wrote:
> On Wed, Jan 17, 2018 at 02:25:38PM -0500, David Miller wrote:
>> From: James Chapman <jchapman@...alix.com>
>> Date: Wed, 17 Jan 2018 11:13:33 +0000
>>
>> > On 16 January 2018 at 19:00, David Miller <davem@...emloft.net> wrote:
>> >> From: Tom Herbert <tom@...bertland.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().
>>
> But, if I read it correctly, that doesn't prevent it from being passed
> to kcm_attach() later on, which will overwrite sk_user_data (unless we
> update the locking scheme and refuse to overwrite sk_user_data in a
> race-free way).
>
> BTW couldn't the gprs_dev pointer be embedded in struct pep_sock?
> This way pep_sk(sk)->gp could be used instead of sk->sk_user_data.
> That'd probably be a violation of the phonet's layering, as that'd
> tie gprs_dev to pep sockets. OTOH, only pep sockets can currently be
> attached to gprs_dev, so in practice that might be a reasonable
> compromise.
>
>> 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.
>>
> SunRPC seems to possibly set sk_user_data on user sockets: svc_addsock()
> gets a socket using sockfd_lookup() then passes it to svc_setup_socket()
> which in turn sets sk_user_data. I don't know anything about SunRPC, so
> I might very well have missed important details, but I believe such a
> socket could be passed to KCM which could lead to the same kind of
> issues as for L2TP. Other RPCs look safe to me.
>
>> 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).
>>
> Most UDP encapsulations only use kernel sockets though. It seems that
> only L2TP and GTP use setup_udp_tunnel_sock() with userpsace sockets.
> So it might be feasible to restrict usage of sk_user_data to kernel
> sockets only.
>
> For L2TP, we probably can adapt l2tp_sock_to_tunnel() so that it does
> a lookup in a hashtable indexed by the socket pointer, rather than
> dereferencing sk_user_data. That doesn't look very satisfying to me,
> but that's the only way I found so far.

L2TP needs a way to get at its local data from the socket in the data path.

> We also have another user of sk_user_data in l2tp_ppp, but since it
> uses its own socket type, I guess we could simply embed the pointer in
> its parent structure.
>
>> 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...
>>
> So, if I understand correctly, we can either restrict sk_user_data to
> kernel sockets so that KCM couldn't act on them (but then why would we
> make an exception for KCM and allow it to set sk_user_data on
> non-kernel sockets?).
> Or we could agree on a locking scheme for sk_user_data and update all
> users so that they'd fail instead of overwriting it when it's not NULL.
>
> Assuming my understanding is correct, do you have any preference for
> fixing this issue? Or any other ideas?

Could we add a new pointer, say, encap_user_data to struct udp_sock
and use it instead of sk_user_data for UDP-encap sockets?

Powered by blists - more mailing lists