Re: [PATCH 1/3] netfilter: ctnetlink: cleanup master conntrack assignation

[Pablo Neira Ayuso <pablo@netfilter.org>]

Patrick McHardy wrote:
> Patrick McHardy wrote:
>> Pablo Neira Ayuso wrote:
>>> This patch moves the assignation of the master conntrack to
>>> ctnetlink_create_conntrack(), which is where it really belongs.
>>> This patch is a cleanup.
>>
>> Applied, thanks.
> 
> I've added this patch on top to fix a RCU context imbalance.

Oh, stupid mistake, sorry for that.

> This seems like a good opportunity to say this again: please (everyone)
> compile your code using sparse. It catches this type and more bugs.

I'll do.

> There is a second bug introduced by this patch:
> 
>     add_timer(&ct->timeout);
>     nf_conntrack_hash_insert(ct);
>     rcu_read_unlock();
> 
>     return ct;
> 
> The conntrack lock is not held, it might crash or create double entries.

Hm, but that code is inside ctnetlink_create_conntrack() which is called
with the conntrack lock held.

       if (nlh->nlmsg_flags & NLM_F_CREATE)
                 err = ctnetlink_create_conntrack(cda,
                                                  &otuple,
                                                  &rtuple,
-                                                 master_ct,
                                                  NETLINK_CB(skb).pid,
-                                                 nlmsg_report(nlh));
+                                                 nlmsg_report(nlh),
+                                                 u3);
                spin_unlock_bh(&nf_conntrack_lock);

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