Re: [PATCH nf-next 3/6] netfilter: nf_tables: disable old tracing if listener is present

[Florian Westphal <fw@strlen.de>]

Patrick McHardy <kaber@trash.net> wrote:
> On 24.11, Florian Westphal wrote:
> > We already have an attribute for verdicts, but that won't be enough.
> > Adding NFTA_TRACE_FILENAME/LINENO seems wrong to me since that would
> > be a pure development thing and we already have tools like perf probe
> > for that.
> 
> Yes absolutely. We have a few things that provide stable identifiers, f.i.
> hooks, helper names, protocol names, subsystems (conntrack/NAT/TCP names
> will not change), ...

Ok, so that should work within this proposed set, we would need to add
new attributes and types:

e.g. NFT_TRACETYPE_CT which then can set NFTA_TRACE_HELPER_NAME or any other
new attr we want/need.

> If we need more granularity we need to define some stable sub-identifiers.
> 
> > I don't see anything in the current proposal that would not allow to use
> > it for further netfilter trace events, however, at least for conntrack
> > i suspect it might make more sense to extend ctnetlink instead.
> 
> Sure, I'm just saying we should consider this. ctnetlink is more about
> ct events, not really about the packets.

Right.

> I just think it makes sense to
> have a unified view of tracing because you run into ordering issues if you
> send events using multiple subsystems. There's also a big advantage in
> being able to use a simple tool like nft for all your tracing needs compared
> to combining nft, nfnetlink_log and ctnetlink data to get the full picture.

Yes, I see.  This makes sense.  I'm withdrawing the ctnetlink comment.
We have extensions for nflog and nfqueue to serialize the CT object
entirely withing nfnetlink_log/queue, might also add
NFTA_TRACE_CT as a nested attribute that contains the entire ct object.

> I'm all for including all the data we need to get a good picture like you
> have now, I just want to make sure we don't make decisions that prevent us
> from extending it to other subsystems later on.

Ok.  Basically I'm going to ignore all of your comments here, since I
would first like to get the v1 'right' before extending it to more
subsystems.

IFF you see anything that prevents us from doing such extension, please
yell.  But AFAICS the proposed attributes and types should be fine wrt.
extensibility.

I'm going to add new netlink group for the trace events and will address
all the other comments.

Thanks for reviewing!