Patrick McHardy wrote:> Eric Dumazet wrote:>> In my own analysis, I found death_by_timeout() might be problematic,>> with RCU and lockless lookups.>>>> static void death_by_timeout(unsigned long ul_conntrack)>> {>> struct nf_conn *ct = (void *)ul_conntrack;>>>> if (!test_bit(IPS_DYING_BIT, &ct->status) &&>> unlikely(nf_conntrack_event(IPCT_DESTROY, ct) < 0)) {>> /* destroy event was not delivered */>> nf_ct_delete_from_lists(ct);>> << HERE >>>>>> nf_ct_insert_dying_list(ct);>> return;>> }>> set_bit(IPS_DYING_BIT, &ct->status);>> nf_ct_delete_from_lists(ct);>> nf_ct_put(ct);>> }>>>>>> We delete ct from a list and insert it in a new list.>>>> I believe a reader could "*catch*" ct while doing a lookup and miss>> the end>> of its chain. (nulls algo check the null value at the end of lookup>> and can>> decide to restart the lookup if the null value is not the expected one)>>>> We need to change nf_conntrack_init_net() and use a different "null">> value,>> guaranteed not being used in regular lists> > Good catch. This is a new bug, but it shouldn't matter in this case> since nf_conntrack_event() can't fail unless you have a userspace> listener that makes use of reliable delivery, which I think hasn't> even been released yet.

Indeed. I didn't include user-space support for this yet in my tree, sothis should not be the problem. Thanks for the catch anyway!