| Message ID | 1433355401-21332-1-git-send-email-pablo@netfilter.org |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
On 03.06, Pablo Neira Ayuso wrote: > before: > > table ip filter { > chain test { > cpu { 67108864, 50331648, 33554432} > } > } > > after: > > table ip filter { > chain test { > cpu { 4, 3, 2 } > } > } > > Related to 525323352904 ("expr: add set_elem_expr as container for set element > attributes"). > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > Note: This patch applies to the next-4.1 branch. > > src/netlink_delinearize.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c > index b1ce911..ea60c86 100644 > --- a/src/netlink_delinearize.c > +++ b/src/netlink_delinearize.c > @@ -824,6 +824,10 @@ static void integer_type_postprocess(struct expr *expr) > integer_type_postprocess(i); > } > break; > + case EXPR_SET_ELEM: > + expr_set_type(expr->key, expr->dtype, expr->byteorder); > + integer_type_postprocess(expr->key); The fundamental problem is that this is that integers are a special case where we don't always have enough context to properly restore. Literal sets can work because we by definition have a LHS, but it is impossible to fix for named sets. IMO the correct solution is to use endian encoding for integers. Special case the special case, basically. By that I don't mean internally using be_integer and le_integer, but to only do it on the lowest possible level, set type declarations. Our default encoding is BE, so we add a LE integer type and *only* use it for set type declarations and immediately when delinearizing map it back to a regular integer, but adjust the byteorder appropriately. This will fix both problems, and we can (need to) get rid of the integer_type_postprocess(), which can only work for literal sets. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 04, 2015 at 04:23:40AM +0200, Patrick McHardy wrote: > On 03.06, Pablo Neira Ayuso wrote: > > before: > > > > table ip filter { > > chain test { > > cpu { 67108864, 50331648, 33554432} > > } > > } > > > > after: > > > > table ip filter { > > chain test { > > cpu { 4, 3, 2 } > > } > > } > > > > Related to 525323352904 ("expr: add set_elem_expr as container for set element > > attributes"). > > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > --- > > Note: This patch applies to the next-4.1 branch. > > > > src/netlink_delinearize.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c > > index b1ce911..ea60c86 100644 > > --- a/src/netlink_delinearize.c > > +++ b/src/netlink_delinearize.c > > @@ -824,6 +824,10 @@ static void integer_type_postprocess(struct expr *expr) > > integer_type_postprocess(i); > > } > > break; > > + case EXPR_SET_ELEM: > > + expr_set_type(expr->key, expr->dtype, expr->byteorder); > > + integer_type_postprocess(expr->key); > > > The fundamental problem is that this is that integers are a special case where > we don't always have enough context to properly restore. Literal sets can work > because we by definition have a LHS, but it is impossible to fix for named sets. > > IMO the correct solution is to use endian encoding for integers. Special case > the special case, basically. By that I don't mean internally using be_integer > and le_integer, but to only do it on the lowest possible level, set type > declarations. Our default encoding is BE, so we add a LE integer type and *only* > use it for set type declarations and immediately when delinearizing map it back > to a regular integer, but adjust the byteorder appropriately. > > This will fix both problems, and we can (need to) get rid of the > integer_type_postprocess(), which can only work for literal sets. I think I'm still in trouble to resolve another (follow up related) problem: If I want to create a named set that contains 'meta cpu' this selector currently relies on the generic integer type: # nft add set filter mycpuset { type integer\; } <cmdline>:1:19-33: Error: unqualified key data type specified in set definition add set test test { type integer; } ^^^^^^^^^^^^^^^ this fails because integer_type lacks of byteorder and size. I made a quick patch to add specific types for set declarations, eg. # nft add set test test { type cpu32; } We would have, let's say: u8, cpu16, cpu32, cpu64, be16, be32, be64, whose base datatype is the generic integer_type. This would also allow us to define named sets that contain elements to be use with any kind of (integer_type) selector. As you said, when declaring named sets we need to know the type since we have no LHS as this is not yet referenced, this would provide the specific datatype definition. But if we end up having these specific integer types are in place, I fail to see why we should not use them consistently everywhere, no matter if this is a literal set, named set or any kind of simple comparison. Am I missing anything? Please advice. Thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04.06, Pablo Neira Ayuso wrote: > On Thu, Jun 04, 2015 at 04:23:40AM +0200, Patrick McHardy wrote: > > On 03.06, Pablo Neira Ayuso wrote: > > The fundamental problem is that this is that integers are a special case where > > we don't always have enough context to properly restore. Literal sets can work > > because we by definition have a LHS, but it is impossible to fix for named sets. > > > > IMO the correct solution is to use endian encoding for integers. Special case > > the special case, basically. By that I don't mean internally using be_integer > > and le_integer, but to only do it on the lowest possible level, set type > > declarations. Our default encoding is BE, so we add a LE integer type and *only* > > use it for set type declarations and immediately when delinearizing map it back > > to a regular integer, but adjust the byteorder appropriately. > > > > This will fix both problems, and we can (need to) get rid of the > > integer_type_postprocess(), which can only work for literal sets. > > I think I'm still in trouble to resolve another (follow up related) > problem: > > If I want to create a named set that contains 'meta cpu' this selector > currently relies on the generic integer type: > > # nft add set filter mycpuset { type integer\; } > <cmdline>:1:19-33: Error: unqualified key data type specified in set definition > add set test test { type integer; } > ^^^^^^^^^^^^^^^ > > this fails because integer_type lacks of byteorder and size. Right. > I made a quick patch to add specific types for set declarations, eg. > > # nft add set test test { type cpu32; } > > We would have, let's say: u8, cpu16, cpu32, cpu64, be16, be32, be64, > whose base datatype is the generic integer_type. > > This would also allow us to define named sets that contain elements to > be use with any kind of (integer_type) selector. > > As you said, when declaring named sets we need to know the type since > we have no LHS as this is not yet referenced, this would provide the > specific datatype definition. > > But if we end up having these specific integer types are in place, I > fail to see why we should not use them consistently everywhere, no > matter if this is a literal set, named set or any kind of simple > comparison. > > Am I missing anything? > > Please advice. Thanks! The main reason is that we have integer type specific handling that would then need to check for all possible types. Internally we don't really need those types if we properly qualify an instantiated integer since our datatypes specify both byteorder and size. So my idea was to map those as soon as we have all the required information to keep the code simpler. Regarding the set declarations - I do not like the idea of having the user deal with byteorder and type widths directly. I'd rather add a cpu_id type for this specific case. I don't think we have more meta types that use plain integers. This leaves the question of how to deal with packet data. This uXX/beXX types unfortunately also don't solve this completely. The DCCP protocol for instance uses 48 bits for its sequence numbers. Its of course rather unlikely that it makes any sense to use sets for this, but it illustrates the problem. An alternative idea would be that instead of specifying a data type, we allow to specify an expression type that the set should hold. F.i. add set filter test { typeof meta cpu; } add set filter test { typeof dccp seqno; } What do you think about that? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 05, 2015 at 12:08:30PM +0200, Patrick McHardy wrote: > On 04.06, Pablo Neira Ayuso wrote: [...] > > I made a quick patch to add specific types for set declarations, eg. > > > > # nft add set test test { type cpu32; } > > > > We would have, let's say: u8, cpu16, cpu32, cpu64, be16, be32, be64, > > whose base datatype is the generic integer_type. > > > > This would also allow us to define named sets that contain elements to > > be use with any kind of (integer_type) selector. > > > > As you said, when declaring named sets we need to know the type since > > we have no LHS as this is not yet referenced, this would provide the > > specific datatype definition. > > > > But if we end up having these specific integer types are in place, I > > fail to see why we should not use them consistently everywhere, no > > matter if this is a literal set, named set or any kind of simple > > comparison. > > > > Am I missing anything? > > > > Please advice. Thanks! > > The main reason is that we have integer type specific handling that would > then need to check for all possible types. Internally we don't really need > those types if we properly qualify an instantiated integer since our > datatypes specify both byteorder and size. So my idea was to map those > as soon as we have all the required information to keep the code simpler. > > Regarding the set declarations - I do not like the idea of having the > user deal with byteorder and type widths directly. I'd rather add a > cpu_id type for this specific case. > > I don't think we have more meta types that use plain integers. This > leaves the question of how to deal with packet data. This uXX/beXX > types unfortunately also don't solve this completely. The DCCP protocol > for instance uses 48 bits for its sequence numbers. Its of course > rather unlikely that it makes any sense to use sets for this, but > it illustrates the problem. > > An alternative idea would be that instead of specifying a data type, > we allow to specify an expression type that the set should hold. > F.i. > > add set filter test { typeof meta cpu; } > add set filter test { typeof dccp seqno; } > > What do you think about that? I like your typeof idea so the user doesn't need to know the specific datatype. However, when listing an unreferenced named set back to userspace I think we won't have enough context to reconstruct this from the keytype, right? Going back to the original endianess problem in literal sets using integer types, eg. cpu { 50331648} meta length { 50331648} cgroup { 50331648} The choices I see here at this moment is: 1) Keep integer_type_postprocess() in place and rely on the LHS. Basically, something very similar to what I sent in this original patch. 2) Add something like host_integer_type so we can encode the endianness into the set keytype, then use it from the evaluation step. Thus, we can kill integer_type_postprocess(). --- a/src/evaluate.c +++ b/src/evaluate.c @@ -942,6 +942,10 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, str switch (rel->op) { case OP_LOOKUP: + if (left->dtype == &integer_type && + left->byteorder == BYTEORDER_HOST_ENDIAN) + left->dtype = &host_integer_type; I think we can use this type for set declarations too while keytypes in both literal and named sets will look the same. I'm still in doubt on how to address the integer size without specific types. Let me know if you can pull out any better solution from your hat :). Thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05.06, Pablo Neira Ayuso wrote: > On Fri, Jun 05, 2015 at 12:08:30PM +0200, Patrick McHardy wrote: > > On 04.06, Pablo Neira Ayuso wrote: > [...] > > > I made a quick patch to add specific types for set declarations, eg. > > > > > > # nft add set test test { type cpu32; } > > > > > > We would have, let's say: u8, cpu16, cpu32, cpu64, be16, be32, be64, > > > whose base datatype is the generic integer_type. > > > > > > This would also allow us to define named sets that contain elements to > > > be use with any kind of (integer_type) selector. > > > > > > As you said, when declaring named sets we need to know the type since > > > we have no LHS as this is not yet referenced, this would provide the > > > specific datatype definition. > > > > > > But if we end up having these specific integer types are in place, I > > > fail to see why we should not use them consistently everywhere, no > > > matter if this is a literal set, named set or any kind of simple > > > comparison. > > > > > > Am I missing anything? > > > > > > Please advice. Thanks! > > > > The main reason is that we have integer type specific handling that would > > then need to check for all possible types. Internally we don't really need > > those types if we properly qualify an instantiated integer since our > > datatypes specify both byteorder and size. So my idea was to map those > > as soon as we have all the required information to keep the code simpler. > > > > Regarding the set declarations - I do not like the idea of having the > > user deal with byteorder and type widths directly. I'd rather add a > > cpu_id type for this specific case. > > > > I don't think we have more meta types that use plain integers. This > > leaves the question of how to deal with packet data. This uXX/beXX > > types unfortunately also don't solve this completely. The DCCP protocol > > for instance uses 48 bits for its sequence numbers. Its of course > > rather unlikely that it makes any sense to use sets for this, but > > it illustrates the problem. > > > > An alternative idea would be that instead of specifying a data type, > > we allow to specify an expression type that the set should hold. > > F.i. > > > > add set filter test { typeof meta cpu; } > > add set filter test { typeof dccp seqno; } > > > > What do you think about that? > > I like your typeof idea so the user doesn't need to know the specific > datatype. However, when listing an unreferenced named set back to > userspace I think we won't have enough context to reconstruct this > from the keytype, right? Yeah, we still need some encoding for that. We currently use 6 bits for the data type, so if we'd take on of those bits to indicate its an expression not a type, that leaves 5 to encode the expression type and subtype. That won't be enough for encoding a full expression type, at least for payload and exthdr. Long term we should move type encoding to set userdata IMO since it also removes limitation in the size of concatenations. > Going back to the original endianess problem in literal sets using > integer types, eg. > > cpu { 50331648} > meta length { 50331648} > cgroup { 50331648} > > The choices I see here at this moment is: > > 1) Keep integer_type_postprocess() in place and rely on the LHS. > Basically, something very similar to what I sent in this original > patch. > > 2) Add something like host_integer_type so we can encode the > endianness into the set keytype, then use it from the evaluation > step. Thus, we can kill integer_type_postprocess(). > > --- a/src/evaluate.c > +++ b/src/evaluate.c > @@ -942,6 +942,10 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, str > > switch (rel->op) { > case OP_LOOKUP: > + if (left->dtype == &integer_type && > + left->byteorder == BYTEORDER_HOST_ENDIAN) > + left->dtype = &host_integer_type; > > I think we can use this type for set declarations too while keytypes > in both literal and named sets will look the same. I'm still in doubt > on how to address the integer size without specific types. > > Let me know if you can pull out any better solution from your hat :). Given that we don't have a clean way to fix set declarations for unqualified types so far, I think your original patch is fine. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c index b1ce911..ea60c86 100644 --- a/src/netlink_delinearize.c +++ b/src/netlink_delinearize.c @@ -824,6 +824,10 @@ static void integer_type_postprocess(struct expr *expr) integer_type_postprocess(i); } break; + case EXPR_SET_ELEM: + expr_set_type(expr->key, expr->dtype, expr->byteorder); + integer_type_postprocess(expr->key); + break; default: break; }
before: table ip filter { chain test { cpu { 67108864, 50331648, 33554432} } } after: table ip filter { chain test { cpu { 4, 3, 2 } } } Related to 525323352904 ("expr: add set_elem_expr as container for set element attributes"). Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- Note: This patch applies to the next-4.1 branch. src/netlink_delinearize.c | 4 ++++ 1 file changed, 4 insertions(+)