Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753575AbdCBFpV (ORCPT ); Thu, 2 Mar 2017 00:45:21 -0500 Received: from mail-it0-f46.google.com ([209.85.214.46]:37133 "EHLO mail-it0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750823AbdCBFpR (ORCPT ); Thu, 2 Mar 2017 00:45:17 -0500 MIME-Version: 1.0 In-Reply-To: References: From: Eric Dumazet Date: Wed, 1 Mar 2017 21:36:08 -0800 Message-ID: Subject: Re: net: use-after-free in neigh_timer_handler/sock_wfree To: Cong Wang Cc: Dmitry Vyukov , David Miller , netdev , LKML , syzkaller , Alexey Kuznetsov , James Morris Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1655 Lines: 43 On Wed, Mar 1, 2017 at 9:25 PM, Cong Wang wrote: > On Wed, Mar 1, 2017 at 3:15 PM, Eric Dumazet wrote: >> On Wed, Mar 1, 2017 at 3:09 PM, Cong Wang wrote: >> >>> >>> But I doubt skb_orphan() is the solution here, shouldn't we just >>> update sk->sk_wmem_alloc with skb->truesize changes? >> >> Is it worth it ? Apart from syszkaller I mean... >> >> We started with something that had a real impact on real workloads. >> >> 158f323b9868b59967ad96957c4ca388161be321 net: adjust skb->truesize in >> pskb_expand_head() >> >> Note that auditing the stack took me a while. > > I don't know how sk refcnt could work correctly without making > sk_wmem_alloc correctly. We certainly could just call skb_orphan() > is we don't need skb->sk any more, probably like the frag case, > but for this case, the neigh one, the skb's sitting in neigh->arp_queue > are not going to be freed unless in failed case, therefore skb->sk > should not be orphaned so early. There is absolutely no issue in arp/nd case. Many skbs can sit there and it is fine. Same with skbs sitting a long time in a qdisc. Of course we try to not call skb_orphan() unless really needed. tcp_gso_segment() tries very hard to propagate skb ownership to the segments, but even something apparently easy like that took some patches before being done right. (for details : 0d08c42cf9a71530fef5ebcfe368f38f2dd0476f "tcp: gso: fix truesize tracking") conntrack reasm is mostly used in forwarding workloads, where skb->sk is already NULL. Are you thinking of a real workload where skb->sk _needs_ to be kept in ipv6 reasm ?