Re: [PATCH net] l2tp: add sk_reuseport checks to l2tp_validate_socket

[Martin KaFai Lau <kafai@fb.com>]

On Mon, Jul 06, 2020 at 12:45:36PM -0700, David Miller wrote:
> From: James Chapman <jchapman@katalix.com>
> Date: Mon, 6 Jul 2020 13:12:59 +0100
> 
> > The crash occurs in the socket destroy path. bpf_sk_reuseport_detach
> > assumes ownership of sk_user_data if sk_reuseport is set and writes a
> > NULL pointer to the memory pointed to by
> > sk_user_data. bpf_sk_reuseport_detach is called via
> > udp_lib_unhash. l2tp does its socket cleanup through sk_destruct,
> > which fetches private data through sk_user_data. The BUG_ON fires
> > because this data has been corrupted.
> 
> The ownership of sk_user_data has to be handled more cleanly.
> 
> BPF really has no business taking over this as it is for the protocols
> to use and what L2TP is doing is quite natural and normal.  Exactly
> what sk_user_data was designed to be used for.
> 
> I'm not applying this, please take this up with the BPF folks.  They
> need to store their metadata elsewhere.
Thanks for the report.

The sk_user_data is used when a sk is added to the bpf's reuseport_sockarray.
Before it can be added, the bpf side does check if the sk_user_data has already been
used or not.  It is the similar check like other usages on sk_user_data.

The missing part is the reuseport_detach_sock() should check if a
sk is currently in a reuseport_sockarray before calling bpf_sk_reuseport_detach().
It can be solved by remembering if a sk is added to the reuseport_sockarray.
I will work on a fix by doing this.