netdev - Re: [PATCH bpf] bpf, sockmap: Fix map type error in sock_map_del

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]

Message-ID: <64c8806537c3a_a427920846@john.notmuch>
Date: Mon, 31 Jul 2023 20:47:49 -0700
From: John Fastabend <john.fastabend@...il.com>
To: Xu Kuohai <xukuohai@...weicloud.com>, 
 Martin KaFai Lau <martin.lau@...ux.dev>, 
 Xu Kuohai <xukuohai@...weicloud.com>, 
 John Fastabend <john.fastabend@...il.com>
Cc: bpf@...r.kernel.org, 
 netdev@...r.kernel.org, 
 Jakub Sitnicki <jakub@...udflare.com>, 
 "David S . Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH bpf] bpf, sockmap: Fix map type error in sock_map_del_link

Xu Kuohai wrote:
> On 8/1/2023 9:19 AM, Martin KaFai Lau wrote:
> > On 7/28/23 3:56 AM, Xu Kuohai wrote:
> >> sock_map_del_link() operates on both SOCKMAP and SOCKHASH, although
> >> both types have member named "progs", the offset of "progs" member in
> >> these two types is different, so "progs" should be accessed with the
> >> real map type.
> > 
> > The patch makes sense to me. Can a test be written to trigger it?
> > 
> 
> Thank you for the review. I have a messy prog that triggers memleak
> caused by this issue. I'll try to simplify it to a test.
> 
> > John, please review.
> > 
> > 
> > .
> 
> 

Thanks good catch. One thing I don't see any tests for is deleting a
socket from a sockmap and then trying to use it? My guess is almost
no one deletes sockets from a map except on sock close. Maybe to
reproduce,

 1. connect a bunch of sockets to sockhash with verdict prog
 2. remove the sockets
 3. remove the sockhash
 4. that should leak the bpf ref cnt so we could check for the
    prog still existing?

Reviewed-by: John Fastabend <john.fastabend@...il.com>