Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
>> Patrick McHardy wrote:
>>>>> Pablo, please let me know whether you want me to apply this.
>>>> ctnetlink_change_helper() also calls nf_ct_ext_add() for conntracks that
>>>> are confirmed (in case of a helper update for an existing conntrack).
>>>> That would also trigger the assertion. If we want to support helper
>>>> assignation via ctnetlink for existing conntracks, we will need to add
>>>> locking to the conntrack extension infrastructure to avoid races.
>>>>
>>>> I don't see a clear solution for this yet.
>>> I see, this is indeed a problem. Since the helper is known at the
>>> first event, we could restrict this to only allow manual assignment
>>> for newly created conntracks. Most helpers probably can't properly
>>> cope with connections not seen from the beginning anyways.
>> Indeed, changing the helper in the middle of the road doesn't make too
>> much sense to me either. I can send you a patch for this along today,
>> I'll find some spare time to do it.
>
> Great, thanks Pablo.
I have slightly tested the following patch here. I think it should fix
the problem.
We can revisit ctnetlink_change_helper() later, I think there's some
code there that can be refactorized.
Let me know if you're OK with it.
netfilter: ctnetlink: fix creation of conntrack with helpers
This patch fixes a bug that triggers an assertion if you create
a conntrack entry with a helper and netfilter debugging is enabled.
Basically, we hit the assertion because the confirmation flag is
set before the conntrack extensions are added. To fix this, we
move the extension addition before the aforementioned flag is
set.
This patch also removes the possibility of setting a helper for
existing conntracks. This operation would also trigger the
assertion since we are not allowed to add new extensions for
existing conntracks. We know noone that could benefit from
this operation sanely.
Thanks to Eric Dumazet for initial posting a preliminary patch
to address this issue.
Reported-by: David Ramblewski <David.Ramblewski@xxxxxxxxxxxxxx>
Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
---
net/netfilter/nf_conntrack_netlink.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 8b05f36..2b2af63 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1077,9 +1077,8 @@ ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[])
/* need to zero data of old helper */
memset(&help->help, 0, sizeof(help->help));
} else {
- help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
- if (help == NULL)
- return -ENOMEM;
+ /* we cannot set a helper for an existing conntrack */
+ return -EOPNOTSUPP;
}
rcu_assign_pointer(help->helper, helper);
@@ -1263,7 +1262,6 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
ct->timeout.expires = ntohl(nla_get_be32(cda[CTA_TIMEOUT]));
ct->timeout.expires = jiffies + ct->timeout.expires * HZ;
- ct->status |= IPS_CONFIRMED;
rcu_read_lock();
if (cda[CTA_HELP]) {
@@ -1314,14 +1312,19 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
goto err2;
}
- if (cda[CTA_STATUS]) {
- err = ctnetlink_change_status(ct, cda);
+ if (cda[CTA_NAT_SRC] || cda[CTA_NAT_DST]) {
+ err = ctnetlink_change_nat(ct, cda);
if (err < 0)
goto err2;
}
- if (cda[CTA_NAT_SRC] || cda[CTA_NAT_DST]) {
- err = ctnetlink_change_nat(ct, cda);
+ nf_ct_acct_ext_add(ct, GFP_ATOMIC);
+ nf_ct_ecache_ext_add(ct, 0, 0, GFP_ATOMIC);
+ /* we must add conntrack extensions before confirmation. */
+ ct->status |= IPS_CONFIRMED;
+
+ if (cda[CTA_STATUS]) {
+ err = ctnetlink_change_status(ct, cda);
if (err < 0)
goto err2;
}
@@ -1340,9 +1343,6 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
goto err2;
}
- nf_ct_acct_ext_add(ct, GFP_ATOMIC);
- nf_ct_ecache_ext_add(ct, 0, 0, GFP_ATOMIC);
-
#if defined(CONFIG_NF_CONNTRACK_MARK)
if (cda[CTA_MARK])
ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));
