Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752715AbcJJNti (ORCPT ); Mon, 10 Oct 2016 09:49:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37756 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752047AbcJJNtg (ORCPT ); Mon, 10 Oct 2016 09:49:36 -0400 From: Aaron Conole To: Linus Torvalds Cc: Florian Westphal , Al Viro , Andrew Morton , Jens Axboe , "Ted Ts'o" , Christoph Lameter , David Miller , Pablo Neira Ayuso , Linux Kernel Mailing List , linux-fsdevel , Network Development , NetFilter Subject: Re: slab corruption with current -git (was Re: [git pull] vfs pile 1 (splice)) References: <20161010005105.GA18349@breakpoint.cc> Date: Mon, 10 Oct 2016 09:49:33 -0400 In-Reply-To: (Linus Torvalds's message of "Sun, 9 Oct 2016 20:41:17 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Mon, 10 Oct 2016 13:49:36 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2707 Lines: 67 Linus Torvalds writes: > On Sun, Oct 9, 2016 at 7:49 PM, Linus Torvalds > wrote: >> >> There is one *correct* way to remove an entry from a singly linked >> list, and it looks like this: >> >> struct entry **pp, *p; >> >> pp = &head; >> while ((p = *pp) != NULL) { >> if (right_entry(p)) { >> *pp = p->next; >> break; >> } >> pp = &p->next; >> } >> >> and that's it. Nothing else. Sorry, I should have done that. > This COMPLETELY UNTESTED patch tries to fix the nf_hook_entry code to do this. > > I repeat: it's ENTIRELY UNTESTED. I just converted the insertion and > deletion to the proper pattern, but I could easily have gotten the > insertion priority test the wrong way around entirely, for example. Or > it could simply have some other completely broken bug in it. It > compiles for me, but that's all I actually checked. Okay, I'm looking it over. Sorry for the mess. > Note that the "correct way" of doing list operations also almost > inevitably is the shortest way by far, since it gets rid of all the > special cases. So the patch looks nice. It gets rid of the magic > "nf_set_hooks_head()" thing too, because once you do list following > right, the head is no different from any other pointer in the list. > > So the patch stats look good: > > net/netfilter/core.c | 108 ++++++++++++++++----------------------------------- > 1 file changed, 33 insertions(+), 75 deletions(-) > > but again, it's entirely *entirely* untested. Please consider this > just a "this is generally how list insert/delete operations should be > done, avoiding special cases for the first entry". I'll review it, and test it. Can you tell me what steps you took to reproduce the oops? I'll enable slab debugging and try to reproduce without and with this patch (and I'll also look into David's recent email as well). Are you simply creating and removing network namespaces (I did test that, but I should have done a better job)? > ALSO NOTE! The code assumes that the "nf_hook_mutex" locking only > protects the actual *lists*, and that the address to the list can be > looked up without holding the lock. That's generally how things are > done, and it simplifies error handling (because you can do the "there > is no such list at all" test before you do anything else. But again, I > don't actually know the code, and if there is something that actually > expands the number of lists etc that depends on that mutex, then the > list head lookup may need to be inside the lock too. That should be correct, the nf_hook_mutex is only for protecting the lists. > Linus