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. 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. 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". 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. Linus