Patrick McHardy wrote:
> Pablo, I'm looking at a regression introduced by this patch
> and I'm not sure about the intentions:
> 
>> +int nf_ct_expect_related(struct nf_conntrack_expect *expect)
>> +{
>> +       int ret;
>> +
>> +       spin_lock_bh(&nf_conntrack_lock);
>> +       ret = __nf_ct_expect_check(expect);
>> +       if (ret < 0)
>> +               goto out;
> 
> This is unfortunately broken since we return 0 when refreshing an
> existing expectation. This will create an identical expectation
> for each refresh.

Oh sorry.

>>         nf_ct_expect_insert(expect);
>> +       atomic_inc(&expect->use);
> 
> This I don't understand - the caller is holding a reference, why
> do we need another one?

I thought that the expectation timer may expire while delivering the
event, but that cannot happen since we still hold the reference until
the expectation setup is finished (nf_ct_expect_alloc() gets the
refcount, later nf_ct_expect_put() puts it).

>> +       spin_unlock_bh(&nf_conntrack_lock);
>>         nf_ct_expect_event(IPEXP_NEW, expect);
>> -       ret = 0;
>> +       nf_ct_expect_put(expect);
>> +       return ret;
>>  out:
>>         spin_unlock_bh(&nf_conntrack_lock);
>>         return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(nf_ct_expect_related);
>>  
>> +int nf_ct_expect_related_report(struct nf_conntrack_expect *expect,
>> +                               u32 pid, int report)
>> +{
>> +       int ret;
>> +
>> +       spin_lock_bh(&nf_conntrack_lock);
>> +       ret = __nf_ct_expect_check(expect);
>> +       if (ret < 0)
>> +               goto out;
> 
> Same problem
> 
>> +       nf_ct_expect_insert(expect);
>> +out:
>> +       spin_unlock_bh(&nf_conntrack_lock);
>> +       if (ret == 0)
>> +               nf_ct_expect_event_report(IPEXP_NEW, expect, pid,
>> report);
> 
> But here we don't take the reference, despite having the exact
> same situation.

I don't find an answer for that, likely that I got confused for some
reason, sorry.

> The next question would be - why do we need those two functions at
> all? Aside from the apparently unnecessary reference counting, the
> only difference is reporting, and that actually uses the exact
> same code path.

Is the patch attached on the right track?

-- 
"Los honestos son inadaptados sociales" -- Les Luthiers