Patrick McHardy <kaber@xxxxxxxxx> wrote: > On 21.04, Florian Westphal wrote: > > Instead of taking the value to set from a source register, userspace > > passes the bit that we should set as an immediate netlink value. > > > > This follows a similar approach that xtables 'connlabel' > > match uses, so when user inputs > > > > ct label set bar > > > > then we will set the bit used by the 'bar' label and leave the rest alone. > > Pablo suggested to re-use the immediate attributes already used by > > nft_immediate, nft_bitwise and nft_cmp to re-use as much code as > > possible. > > > > Just add new NFTA_CT_IMM that contains nested data attributes. > > We can then use nft_data_init and nft_data_dump for this as well. > > What's the argument against using immediate and a register? http://marc.info/?l=netfilter-devel&m=145800804914781&w=2 <quote> > However, with nft, the input is just a register with arbitrary runtime > content. > > We therefore ask for the upper ceiling we currently have, which is > enough room to store 128 bits. We can probably allow passing the label value as attribute to the nft_ct expression so you don't have to use the upper ceiling. Patrick suggested something similar for nft_ct set helper support. </unqote> > > diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c > > index 4f66cb9..a461c3e 100644 > > --- a/net/netfilter/nft_ct.c > > +++ b/net/netfilter/nft_ct.c > > @@ -31,6 +31,15 @@ struct nft_ct_reg { > > }; > > }; > > > > +struct nft_ct_imm { > > + enum nft_ct_keys key:8; > > + union { > > + u8 set_bit; > > + } imm; > > + unsigned int imm_len:8; > > + struct nft_data immediate; > > Is this really needed? It should be possible to reconstruct from the bit, no? Yes, I can change that but it seemed much more simple to just stash the original value/length and dump that via if (nft_data_dump(skb, NFTA_CT_IMM, &priv->immediate, NFT_DATA_VALUE, priv->imm_len) < 0) Rather than rebuilding nft_data for all supported keys (right now its only one, but there was interest to add helper assignment support too). > > + err = nft_ct_l3proto_try_module_get(ctx->afi->family); > > + if (err < 0) > > + return err; > > What about the inet table? nft_ct_l3proto_try_module_get() dispatches to IPV4 and IPV6 if afi->family is INET. > > +#ifdef CONFIG_NF_CONNTRACK_LABELS > > + case NFT_CT_LABELS: > > + err = -EINVAL; > > + if (tb[NFTA_CT_DIRECTION] || priv->imm_len != sizeof(u32)) > > Why do we need to store imm_len if the size is fixed? We don't, but then dumping would be something like struct nft_data immediate = {}; switch (key) { case NFT_CT_LABELS: size = sizeof(u32); immedate.data[0] = htonl(priv->immediate.data[0]); break; case ... } if (nft_data_dump(skb, NFTA_CT_IMM, &immediate, NFT_DATA_VALUE, size)) ... If that is preferred I can change it accordingly. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
