netdev - Re: Kernel memory leak in bnx2x driver with vxlan tunnel

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: <CAEh+42iRs8RPATfj7P+ADUjFV3nTBMiEU3AXVe5vY9GMDbKKDQ@mail.gmail.com>
Date:	Tue, 19 Jan 2016 16:51:57 -0800
From:	Jesse Gross <jesse@...nel.org>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	John <john.phillips5@....com>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Tom Herbert <tom@...bertland.com>, david.roth@....com,
	Pravin B Shelar <pshelar@...ira.com>,
	Thomas Graf <tgraf@...g.ch>
Subject: Re: Kernel memory leak in bnx2x driver with vxlan tunnel

On Tue, Jan 19, 2016 at 4:17 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Tue, 2016-01-19 at 16:00 -0800, Jesse Gross wrote:
>
>> Oh, I wasn't disagreeing that there is a problem. I was just trying to
>> point out the conceptual root of the problem rather than the
>> individual instances in different tunnels implementations.
>>
>> In any case, I think we can solve this by calling
>> skb_release_head_state() in napi_skb_finish() and switching early
>> demux to use skb_valid_dst().
>
> Yes, but it will also have to call skb_dst_drop() or risk a leak of
> prior one.

skb_dst_drop() is the key part but I mentioned
skb_release_head_state() (which calls skb_dst_drop()) as a slightly
more generic solution. But either one is fine.

> So what is the purpose of having a dst if we need to drop it ?
>
> Adding code in GRO would be fine if someone explains me the purpose of
> doing apparently useless work.
>
> (refcounting on dst is not exactly free)

In the GRO case, the dst is only dropped on the packets which have
been merged and therefore need to be freed (the GRO_MERGED_FREE case).
It's not being thrown away for the overall frame, just metadata that
has been duplicated on each individual frame, similar to the metadata
in struct sk_buff itself. And while it is not used by the IP stack
there are other consumers (eBPF/OVS/etc.). This entire process is
controlled by the COLLECT_METADATA flag on tunnels, so there is no
cost in situations where it is not actually used.