Re: [PATCH nf-next 1/6] netfilter: nf_tables: extend tracing infrastructure

[Florian Westphal <fw@strlen.de>]

Patrick McHardy <kaber@trash.net> wrote:
> > > > +     if (nla_put_be32(skb, NFTA_TRACE_ID, htonl(hash32_ptr(pkt->skb)))) 
> > 
> > > This could lead to duplicate IDs quite quickly. I can't think of any other
> > > values where we know for sure they will stay constant while the skb is within
> > > netfilter. How about combining this with a per CPU counter or something
> > > similar?
> > 
> > I would not mind, it would result in ID change though for nfqueue case.
> > If thats ok I'll use a pcpu counter that gets incremented when nft_meta
> > sets skb->nftrace=1 and use __this_cpu_read(trace_id) instead (if we
> > have pcpu id counter, is there any point in also using skb address...?)
> 
> Not sure, I guess not.

Ok, I'll move to pcpu id counter in v2

> > > > +	switch (type) {
> > > > +	case NFT_TRACETYPE_POLICY:
> > > > +	case NFT_TRACETYPE_RETURN:
> > > > +	case NFT_TRACETYPE_RULE:
> > > > +		if (nla_put_be32(skb, NFTA_TRACE_VERDICT, htonl(verdict)))
> > > > +			goto nla_put_failure;
> > > 
> > > It would be nice to have the full verdict available, IOW also the jump target.
> > > 
> > > You could simply pass struct nft_verdict to this function and adapt
> > > nft_verdict_dump() to take the attribute value as parameter.
> > 
> > Will add NFTA_TRACE_JUMP/NFTA_TRACE_GOTO for that, ok?
> 
> Any reason not to use the standard verdict encoding? We even have an almost
> ready to use function for that.

Sorry, could you be more specific?
How would you tell userspace where the jump/goto is going to?

What I am doing now in my local version:

NFTA_TRACE_VERDICT is the verbatim result of the hook (i.e. might
contain errno or queue id).

NFTA_TRACE_JUMP_TARGET is the name of the chain that we goto or jump to,
set when verdict == NFT_JUMP/_GOTO.

Alternative suggestions welcome.

This approach means that userspace must try a bit harder to decode
the verdict since we cannot use 'verdict & NF_VERDICT_MASK' if we're
looking at a NFT_* verdict when translating the integer to a string.