Messages in this thread | | | Date | Fri, 10 Feb 2023 17:07:24 +0100 | From | Pablo Neira Ayuso <> | Subject | Re: [PATCH] net: netfilter: fix possible refcount leak in ctnetlink_create_conntrack() |
| |
Hi Florian,
On Fri, Feb 10, 2023 at 11:32:50AM +0100, Florian Westphal wrote: > Hangyu Hua <hbh25y@gmail.com> wrote: > > nf_ct_put() needs to be called to put the refcount got by > > nf_conntrack_find_get() to avoid refcount leak when > > nf_conntrack_hash_check_insert() fails. > > > > Fixes: 7d367e06688d ("netfilter: ctnetlink: fix soft lockup when netlink adds new entries (v2)") > > Signed-off-by: Hangyu Hua <hbh25y@gmail.com> > > --- > > net/netfilter/nf_conntrack_netlink.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c > > index 1286ae7d4609..ca4d5bb1ea52 100644 > > --- a/net/netfilter/nf_conntrack_netlink.c > > +++ b/net/netfilter/nf_conntrack_netlink.c > > @@ -2375,12 +2375,15 @@ ctnetlink_create_conntrack(struct net *net, > > > > err = nf_conntrack_hash_check_insert(ct); > > if (err < 0) > > - goto err2; > > + goto err3; > > Ouch, looks like this is broken in more than one way? > > nf_conntrack_hash_check_insert() can call nf_ct_kill() > and return an error, in that case ct->master reference > is already dropped for us. > > One way would be to return 0 in that case (in > nf_conntrack_hash_check_insert()). What do you think?
This is misleading to the user that adds an entry via ctnetlink?
ETIMEDOUT also looks a bit confusing to report to userspace. Rewinding: if the intention is to deal with stale conntrack extension, for example, helper module has been removed while this entry was added. Then, probably call EAGAIN so nfnetlink has a chance to retry transparently?
BTW, I think we should remove:
NF_CT_STAT_INC_ATOMIC(net, drop);
that is under nf_ct_ext_valid_post(), no packet is dropped in this path.
Thanks.
| |