Hi Pablo,
On Fri, Mar 17, 2017 at 10:09 PM, Gao Feng <fgao@xxxxxxxxxx> wrote:
> Hi Pablo,
>
> On Fri, Mar 17, 2017 at 9:08 PM, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
>> On Tue, Mar 14, 2017 at 02:26:06PM +0800, fgao@xxxxxxxxxx wrote:
>>> From: Gao Feng <fgao@xxxxxxxxxx>
>>>
>>> The helper module permits the helper modules register expectfn, and
>>> it could be hold by external caller. But when the module is unloaded,
>>> there may be some pending expect nodes which still hold the function
>>> reference. It may cause unexpected behavior, even panic.
>>>
>>> Now it would delete the expect nodes which uses the expectfn when
>>> unregister expectfn. And it must use the rcu_read_lock to protect
>>> the expectfn until insert it or doesn't access it ever.
>>>
>>> There is only one caller which doesn't hold the rcu lock now.
>>> It is ctnetlink_create_expect.
>>>
>>> BTW, nf_ct_helper_expectfn_find_by_name/symbol invokes the rcu lock
>>> to protect the lookup. But it is not enough, because it returns the
>>> nf_ct_helper_expectfn pointer which should be protected by rcu lock.
>>> Actually the caller should hold the rcu lock instead of these funcs.
>>> I will refine it in the cleanup patch.
>>>
>>> Signed-off-by: Gao Feng <fgao@xxxxxxxxxx>
>>> ---
>>> net/netfilter/nf_conntrack_helper.c | 23 +++++++++++++++++++++++
>>> net/netfilter/nf_conntrack_netlink.c | 3 +++
>>> 2 files changed, 26 insertions(+)
>>>
>>> diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c
>>> index 6dc44d9..2be820b 100644
>>> --- a/net/netfilter/nf_conntrack_helper.c
>>> +++ b/net/netfilter/nf_conntrack_helper.c
>>> @@ -305,9 +305,32 @@ void nf_ct_helper_expectfn_register(struct nf_ct_helper_expectfn *n)
>>>
>>> void nf_ct_helper_expectfn_unregister(struct nf_ct_helper_expectfn *n)
>>> {
>>> + struct nf_conntrack_expect *exp;
>>> + const struct hlist_node *next;
>>> + u32 i;
>>> +
>>> spin_lock_bh(&nf_conntrack_expect_lock);
>>> list_del_rcu(&n->head);
>>> spin_unlock_bh(&nf_conntrack_expect_lock);
>>> +
>>> + /* Make sure everyone who holds the expect func already
>>> + * has inserted it
>>> + */
>>> + synchronize_rcu();
>>> +
>>> + /* Get rid of expectations used the dying expectfn */
>>> + spin_lock_bh(&nf_conntrack_expect_lock);
>>> + for (i = 0; i < nf_ct_expect_hsize; i++) {
>>> + hlist_for_each_entry_safe(exp, next,
>>> + &nf_ct_expect_hash[i], hnode) {
>>> + if (exp->expectfn == n->expectfn &&
>>> + del_timer(&exp->timeout)) {
>>> + nf_ct_unlink_expect(exp);
>>> + nf_ct_expect_put(exp);
>>> + }
>>> + }
>>> + }
>>
>> Hm. So the problem is when, eg. nf_nat_sip, gets removed via rmmod.
>>
>> I think we should do very similar as it happens in
>> nf_conntrack_helper_unregister().
>>
>> Actually not use the exp->expectfn as reference, but:
>>
>> 1) remove any expectation that belong to this helper.
>> 2) remove any conntrack entry, with NAT is place, that belongs to this
>> helper.
>>
>> I would like to see a function that we can call both from
>> nf_conntrack_helper_unregister() and the exit path (module removal) of
>> conntrack NAT helpers, so we consolidate the code.
>>
>> Does this look good to you?
>
> It is good to use one function to deal with the
> nf_conntrack_helper_unregister and nf_ct_helper_expectfn_unregister.
>
> But there is one problem for nf_ct_helper_expectfn_unregister.
> Let's look at the caller ctnetlink_alloc_expect, it only saves expectfn pointer.
> And there is no any relation between the exp->expectfn and
> exp->helper, because they could be specified by different nlattr data.
> For example, the helper name could be "pptp", while the expectfn could
> be "sip". Actually they are different helper modules.
> Even though it is valid that the exp->help is NULL, because ctlink
> accepts that the cda[CTA_EXPECT_HELP_NAME] is NULL.
>
> So I think it could not use the helper as the identifier when remove
> the expectations in nf_ct_helper_expectfn_unregister. Unless we
> introduce another member in expectation to save which module it
> belongs to.
>
> Best Regards
> Feng
I prepare to use the module as the identifier, is it ok?
When unregister the helper or expectfn, it traverses all expectations
and removes nodes whose belong to the module.
Regards
Feng
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
