Florian Westphal <fw@xxxxxxxxx> wrote: > Patrick McHardy <kaber@xxxxxxxxx> wrote: > > > If not, I see no choice other than resubmitting the original V1 kernel > > > patch that simply copied the entire sreg into the label area, this way > > > no userspace changes are needed. > > > > I have to follow up on the previous discussion. Just wondering, what's wrong > > with simply memcpy'ing and supplying the full set of labels? > > Ok, here is a summary. > > iptables: userspace gives u16 indicating bit, kernel test_bit()s or > set_bits depending if set or get is wanted. > > The checkentry hook tells the kernel that label extension for X bits should be > allocated for new conntracks (where X is what userspace gave in the > match struct). > > > nftables: > get operation simply copies the label area to the sreg. > Since we only support up to 129 labels at the moment this fits into > an nft register. > > nft ct label bla > > simply expands to a bit test on the sreg filled from nft_ct expression. > > There is currently no 'please enable labels for new conntracks'. > > First proposal for set operation was to simply expect userspace to > populate the label space for us and memcpy the sreg to the label area. > > So > > nft label set bla > > will *replace* the entire area with the sreg content, i.e. we delete > any other label(s) present. > > To only set a label, one has to use > > ct labels set ct labels | bla > > I have no preference; this is the most simple patch/approach since > it requires no userspace changes (except documenting it). > > Second drawback is that we don't know which label(s) will be set or > requested so we either have to unconditionally allocate 16 byte > in the conntrack extension, or, alternatively, raise the 'use this many > bits when allocating a new conntrack' variable in the packet path (should > not be a huge deal since after some time all conntracks would have > sufficient space). > > 3rd drawback is that its not atomic anymore since we replace > the entire thing (but I don't think this is a problem in practice). Disregard this. I think i have a very simple solution for this, by reusing V1 and passing the sreg also as the mask argument in nf_connlabels_replace() [ which is the helper to alter labels via ctnetlink ], i.e. nf_connlabels_replace(ct, ®s->data[priv->sreg], ®s->data[priv->sreg], NF_CT_LABELS_MAX_SIZE / sizeof(u32))) It gives xtables-connlabel like semantics, i.e. ct label set bla will only set bla, leaving all other labels alone. This even matches the 'get' part since 'ct label bla' already uses a '&' op, not eq. Only drawback is that nft will ask to allocate enough room for 128 labels (maximum) in the ct extension area as we don't have the individual highest desired label bit that xtables can use to ask for a lower number. I think its not a big deal. Will test and submit patch in the next few hours. Sorry for not thinking of this sooner, I forgot that this mask argument even existed 8-/ -- 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
