Hi,
On Mon, Oct 07, 2019 at 07:10:37AM -0700, Maciej Żenczykowski wrote:
> unsigned int
> nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state)
> {
> enum ip_conntrack_info ctinfo;
> struct nf_conn *ct, *tmpl;
> u_int8_t protonum;
> int dataoff, ret;
>
> tmpl = nf_ct_get(skb, &ctinfo);
> <-----------
> if (tmpl || ctinfo == IP_CT_UNTRACKED) {
> /* Previously seen (loopback or untracked)? Ignore. */
> if ((tmpl && !nf_ct_is_template(tmpl)) ||
> ctinfo == IP_CT_UNTRACKED) {
> NF_CT_STAT_INC_ATOMIC(state->net, ignore);
> return NF_ACCEPT;
> <----------
> }
> skb->_nfct = 0;
> }
>
> /* rcu_read_lock()ed by nf_hook_thresh */
> dataoff = get_l4proto(skb, skb_network_offset(skb), state->pf,
> &protonum);
> if (dataoff <= 0) {
> pr_debug("not prepared to track yet or error occurred\n");
> NF_CT_STAT_INC_ATOMIC(state->net, error);
> NF_CT_STAT_INC_ATOMIC(state->net, invalid);
> ret = NF_ACCEPT;
> goto out;
> }
>
> ...
>
> out:
> if (tmpl)
> nf_ct_put(tmpl);
> <---------
>
> return ret;
> }
> EXPORT_SYMBOL_GPL(nf_conntrack_in);
>
> ---
>
> Do we leak a nf_ct_get() on tmpl at that first 'return NF_ACCEPT' ?
> ie. should it be 'ret = NF_ACCEPT; goto out;'
This patch only entered for loopback and untracked traffic, in such
case the special handling for the template is not required (because
there is no template conntrack in place).
> I'm confused by:
> include/net/netfilter/nf_conntrack.h:65:
> * beware nf_ct_get() is different and don't inc refcnt.
Yes, this call has this semantics since the very beginning IIRC.
> (internal reference b/141976661 & b/135110479 where we're getting kmemleak
> complaints on 4.14 LTS,
> which would possibly be shut up by this 4.17 'silence fix', but:)
>
> I have this gut feeling that:
> commit 114aa35d06d4920c537b72f9fa935de5dd205260
> 'netfilter: conntrack: silent a memory leak warning'
> is bogus...
>
> By my understanding of kmemleak, such gymnastics shouldn't be needed.
> And there's no other users in the network stack of kmemleak_not_leak()
> [except for 2 staging drivers].
Probably, are you observing a memleak there in conntrack? I see you
searching for reason :-)
