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

[James Chapman <jchapman@katalix.com>]

On  Tue, Jul 07, 2020 at 11:31:28 -0700, Martin KaFai Lau wrote:
> 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.

Thanks Martin.