Re: [PATCH v6 -next 2/4] netfilter: nftables: add connlabel set support

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Apr 25, 2016 at 06:05:41PM +0100, Patrick McHardy wrote:
> On 25.04, Florian Westphal wrote:
> > Patrick McHardy <kaber@trash.net> wrote:
> > 
> > > The alternative to internally handling it would be to some propagating
> > > validation to immediates / sets which invoke the actual user of the data.
> > > So in the case of helpers, we could replace the name by references to
> > > the helper structures and reverse this during dumping.
> > > 
> > > Regarding connlabels this doesn't really apply though. We expect userspace
> > > to create a reasonable ruleset and anything that does not cause critical
> > > errors is validated in userspace.
> > 
> > Yes.  So we have three choices here (pseudo-code)
> > 
> > memcpy(ct->labels, regs->data[priv->sreg], sizeof(reg));
> > vs.
> > set_bit(priv->imm, ct->labels);
> > 
> > The latter is what the iptables module does, I do not mind if we
> > go for #1 (treat the label area just like an 128bit register and
> > replace it completely with whatever is in the source register).
> > 
> > My only problem is that Pablo suggested #2 whereas you recommend #1.
> > 
> > I don't want to resubmit until there is consensus as to what the
> > preferred solution is.
> > 
> > We could go for a 3rd alternative, namely:
> > 
> > u16 bit = regs->data[priv->sreg];
> > set_bit(bit, ct->labels);
> > 
> > i.e. have userspace place the _bit_ that we want to set in the
> > source register.
> > 
> > If we go for sreg that would be my favored solution.
> > 
> > The only drawback vs #1 is that get and set work differently
> > (get places all labels into dreg, set expects bit to set).
> 
> That seems like a problem. I agree that #3 would generally be fine, but
> we should also really have "ct labels set ct labels" not change the labels,
> that would be highly counterintuitive.

'ct labels set' is actually operating a bitwise level.

I suggested to introduce something like:

'ct labels bitset' so the user becomes aware of this operation.

And we can just disallow "ct labels set ct labels" since it doesn't
make sense for what Florian needs AFAIK.