diff mbox

[nft,1/4] datatype: missing byteorder in string_type

Message ID 20141214194708.GA17579@salvia
State RFC
Delegated to: Pablo Neira
Headers show

Commit Message

Pablo Neira Ayuso Dec. 14, 2014, 7:47 p.m. UTC 
On Sun, Dec 14, 2014 at 07:01:10PM +0100, Patrick McHardy wrote:
> Am 14. Dezember 2014 18:01:00 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
> >nft add rule filter input iifname { "lo", "eth0" } counter
> >
> >Now the listing shows:
> >
> >	iifname { "lo", "eth0"}
> >
> >instead of:
> >
> >	iifname { "", ""}
> 
> Again wondering what broke this. Let me check when I am at home,
> IIRC we have some check for strings somewhere in the netlink code
> that relies in this.

Attached an alternative to this patch. That I can remember, this is
broken since quite some time.

Let me know, thanks.

Comments

Pablo Neira Ayuso Dec. 15, 2014, 11:43 a.m. UTC | #1 
Hi Patrick!

On Sun, Dec 14, 2014 at 08:47:08PM +0100, Pablo Neira Ayuso wrote:
> On Sun, Dec 14, 2014 at 07:01:10PM +0100, Patrick McHardy wrote:
> > Am 14. Dezember 2014 18:01:00 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
> > >nft add rule filter input iifname { "lo", "eth0" } counter
> > >
> > >Now the listing shows:
> > >
> > >	iifname { "lo", "eth0"}
> > >
> > >instead of:
> > >
> > >	iifname { "", ""}
> > 
> > Again wondering what broke this. Let me check when I am at home,
> > IIRC we have some check for strings somewhere in the netlink code
> > that relies in this.
> 
> Attached an alternative to this patch. That I can remember, this is
> broken since quite some time.

Any concern with this second approach?

Let me know if you prefer I keep this away from this release.

Thanks!

> From ddd263f8ceccf4f30784b9316e2cadfa35e5678d Mon Sep 17 00:00:00 2001
> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Date: Sat, 13 Dec 2014 18:29:37 +0100
> Subject: [PATCH] datatype: fix listing of string elements
> 
> Generalise 0451b82 ("src: generate set members using integer_type in
> the appropriate byteorder") to handle string_type too, since this
> datatype doesn't have any specific byteorder.
> 
> nft add rule filter input iifname { "lo", "eth0" } counter
> 
> Now the listing shows:
> 
> 	iifname { "lo", "eth0"}
> 
> instead of:
> 
> 	iifname { "", ""}
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  src/netlink_delinearize.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
> index e9a04dd..8762cc9 100644
> --- a/src/netlink_delinearize.c
> +++ b/src/netlink_delinearize.c
> @@ -742,7 +742,7 @@ static void payload_dependency_store(struct rule_pp_ctx *ctx,
>  	ctx->pdep  = stmt;
>  }
>  
> -static void integer_type_postprocess(struct expr *expr)
> +static void lookup_postprocess(struct expr *expr)
>  {
>  	struct expr *i;
>  
> @@ -757,7 +757,7 @@ static void integer_type_postprocess(struct expr *expr)
>  	case EXPR_SET_REF:
>  		list_for_each_entry(i, &expr->set->init->expressions, list) {
>  			expr_set_type(i, expr->dtype, expr->byteorder);
> -			integer_type_postprocess(i);
> +			lookup_postprocess(i);
>  		}
>  		break;
>  	default:
> @@ -831,8 +831,12 @@ static void meta_match_postprocess(struct rule_pp_ctx *ctx,
>  	case OP_LOOKUP:
>  		expr_set_type(expr->right, expr->left->dtype,
>  			      expr->left->byteorder);
> -		if (expr->right->dtype == &integer_type)
> -			integer_type_postprocess(expr->right);
> +		switch (expr->right->dtype->type) {
> +		case TYPE_STRING:
> +		case TYPE_INTEGER:
> +			lookup_postprocess(expr->right);
> +			break;
> +		}
>  		break;
>  	default:
>  		break;
> -- 
> 1.7.10.4
> 

--
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
Patrick McHardy Dec. 15, 2014, 11:51 a.m. UTC | #2 
Am 15. Dezember 2014 12:43:32 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
>Hi Patrick!
>
>On Sun, Dec 14, 2014 at 08:47:08PM +0100, Pablo Neira Ayuso wrote:
>> On Sun, Dec 14, 2014 at 07:01:10PM +0100, Patrick McHardy wrote:
>> > Am 14. Dezember 2014 18:01:00 MEZ, schrieb Pablo Neira Ayuso
><pablo@netfilter.org>:
>> > >nft add rule filter input iifname { "lo", "eth0" } counter
>> > >
>> > >Now the listing shows:
>> > >
>> > >	iifname { "lo", "eth0"}
>> > >
>> > >instead of:
>> > >
>> > >	iifname { "", ""}
>> > 
>> > Again wondering what broke this. Let me check when I am at home,
>> > IIRC we have some check for strings somewhere in the netlink code
>> > that relies in this.
>> 
>> Attached an alternative to this patch. That I can remember, this is
>> broken since quite some time.
>
>Any concern with this second approach?
>
>Let me know if you prefer I keep this away from this release.

Sorry, didn't manage to look at it so far. Will manage some time this afternoon.

Cheers,
Patrick

>
>Thanks!
>
>> From ddd263f8ceccf4f30784b9316e2cadfa35e5678d Mon Sep 17 00:00:00
>2001
>> From: Pablo Neira Ayuso <pablo@netfilter.org>
>> Date: Sat, 13 Dec 2014 18:29:37 +0100
>> Subject: [PATCH] datatype: fix listing of string elements
>> 
>> Generalise 0451b82 ("src: generate set members using integer_type in
>> the appropriate byteorder") to handle string_type too, since this
>> datatype doesn't have any specific byteorder.
>> 
>> nft add rule filter input iifname { "lo", "eth0" } counter
>> 
>> Now the listing shows:
>> 
>> 	iifname { "lo", "eth0"}
>> 
>> instead of:
>> 
>> 	iifname { "", ""}
>> 
>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>> ---
>>  src/netlink_delinearize.c |   12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
>> index e9a04dd..8762cc9 100644
>> --- a/src/netlink_delinearize.c
>> +++ b/src/netlink_delinearize.c
>> @@ -742,7 +742,7 @@ static void payload_dependency_store(struct
>rule_pp_ctx *ctx,
>>  	ctx->pdep  = stmt;
>>  }
>>  
>> -static void integer_type_postprocess(struct expr *expr)
>> +static void lookup_postprocess(struct expr *expr)
>>  {
>>  	struct expr *i;
>>  
>> @@ -757,7 +757,7 @@ static void integer_type_postprocess(struct expr
>*expr)
>>  	case EXPR_SET_REF:
>>  		list_for_each_entry(i, &expr->set->init->expressions, list) {
>>  			expr_set_type(i, expr->dtype, expr->byteorder);
>> -			integer_type_postprocess(i);
>> +			lookup_postprocess(i);
>>  		}
>>  		break;
>>  	default:
>> @@ -831,8 +831,12 @@ static void meta_match_postprocess(struct
>rule_pp_ctx *ctx,
>>  	case OP_LOOKUP:
>>  		expr_set_type(expr->right, expr->left->dtype,
>>  			      expr->left->byteorder);
>> -		if (expr->right->dtype == &integer_type)
>> -			integer_type_postprocess(expr->right);
>> +		switch (expr->right->dtype->type) {
>> +		case TYPE_STRING:
>> +		case TYPE_INTEGER:
>> +			lookup_postprocess(expr->right);
>> +			break;
>> +		}
>>  		break;
>>  	default:
>>  		break;
>> -- 
>> 1.7.10.4
>> 


--
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
Patrick McHardy Dec. 15, 2014, 2:16 p.m. UTC | #3 
Am 15. Dezember 2014 12:43:32 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
>Hi Patrick!
>
>On Sun, Dec 14, 2014 at 08:47:08PM +0100, Pablo Neira Ayuso wrote:
>> On Sun, Dec 14, 2014 at 07:01:10PM +0100, Patrick McHardy wrote:
>> > Am 14. Dezember 2014 18:01:00 MEZ, schrieb Pablo Neira Ayuso
><pablo@netfilter.org>:
>> > >nft add rule filter input iifname { "lo", "eth0" } counter
>> > >
>> > >Now the listing shows:
>> > >
>> > >	iifname { "lo", "eth0"}
>> > >
>> > >instead of:
>> > >
>> > >	iifname { "", ""}
>> > 
>> > Again wondering what broke this. Let me check when I am at home,
>> > IIRC we have some check for strings somewhere in the netlink code
>> > that relies in this.
>> 
>> Attached an alternative to this patch. That I can remember, this is
>> broken since quite some time.
>
>Any concern with this second approach?
>
>Let me know if you prefer I keep this away from this release.

Actually the first one is fine, I thought it would affect string postprocessing in delinearization, but it's fine.

>
>Thanks!
>
>> From ddd263f8ceccf4f30784b9316e2cadfa35e5678d Mon Sep 17 00:00:00
>2001
>> From: Pablo Neira Ayuso <pablo@netfilter.org>
>> Date: Sat, 13 Dec 2014 18:29:37 +0100
>> Subject: [PATCH] datatype: fix listing of string elements
>> 
>> Generalise 0451b82 ("src: generate set members using integer_type in
>> the appropriate byteorder") to handle string_type too, since this
>> datatype doesn't have any specific byteorder.
>> 
>> nft add rule filter input iifname { "lo", "eth0" } counter
>> 
>> Now the listing shows:
>> 
>> 	iifname { "lo", "eth0"}
>> 
>> instead of:
>> 
>> 	iifname { "", ""}
>> 
>> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>> ---
>>  src/netlink_delinearize.c |   12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
>> index e9a04dd..8762cc9 100644
>> --- a/src/netlink_delinearize.c
>> +++ b/src/netlink_delinearize.c
>> @@ -742,7 +742,7 @@ static void payload_dependency_store(struct
>rule_pp_ctx *ctx,
>>  	ctx->pdep  = stmt;
>>  }
>>  
>> -static void integer_type_postprocess(struct expr *expr)
>> +static void lookup_postprocess(struct expr *expr)
>>  {
>>  	struct expr *i;
>>  
>> @@ -757,7 +757,7 @@ static void integer_type_postprocess(struct expr
>*expr)
>>  	case EXPR_SET_REF:
>>  		list_for_each_entry(i, &expr->set->init->expressions, list) {
>>  			expr_set_type(i, expr->dtype, expr->byteorder);
>> -			integer_type_postprocess(i);
>> +			lookup_postprocess(i);
>>  		}
>>  		break;
>>  	default:
>> @@ -831,8 +831,12 @@ static void meta_match_postprocess(struct
>rule_pp_ctx *ctx,
>>  	case OP_LOOKUP:
>>  		expr_set_type(expr->right, expr->left->dtype,
>>  			      expr->left->byteorder);
>> -		if (expr->right->dtype == &integer_type)
>> -			integer_type_postprocess(expr->right);
>> +		switch (expr->right->dtype->type) {
>> +		case TYPE_STRING:
>> +		case TYPE_INTEGER:
>> +			lookup_postprocess(expr->right);
>> +			break;
>> +		}
>>  		break;
>>  	default:
>>  		break;
>> -- 
>> 1.7.10.4
>> 


--
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
Pablo Neira Ayuso Dec. 15, 2014, 5:24 p.m. UTC | #4 
On Mon, Dec 15, 2014 at 03:16:19PM +0100, Patrick McHardy wrote:
> Am 15. Dezember 2014 12:43:32 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
> >Hi Patrick!
> >
> >On Sun, Dec 14, 2014 at 08:47:08PM +0100, Pablo Neira Ayuso wrote:
> >> On Sun, Dec 14, 2014 at 07:01:10PM +0100, Patrick McHardy wrote:
> >> > Am 14. Dezember 2014 18:01:00 MEZ, schrieb Pablo Neira Ayuso
> ><pablo@netfilter.org>:
> >> > >nft add rule filter input iifname { "lo", "eth0" } counter
> >> > >
> >> > >Now the listing shows:
> >> > >
> >> > >	iifname { "lo", "eth0"}
> >> > >
> >> > >instead of:
> >> > >
> >> > >	iifname { "", ""}
> >> > 
> >> > Again wondering what broke this. Let me check when I am at home,
> >> > IIRC we have some check for strings somewhere in the netlink code
> >> > that relies in this.
> >> 
> >> Attached an alternative to this patch. That I can remember, this is
> >> broken since quite some time.
> >
> >Any concern with this second approach?
> >
> >Let me know if you prefer I keep this away from this release.
> 
> Actually the first one is fine, I thought it would affect string
> postprocessing in delinearization, but it's fine.

Thanks Patrick! I have applied the first patch.
--
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 mbox

Patch

From ddd263f8ceccf4f30784b9316e2cadfa35e5678d Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Sat, 13 Dec 2014 18:29:37 +0100
Subject: [PATCH] datatype: fix listing of string elements

Generalise 0451b82 ("src: generate set members using integer_type in
the appropriate byteorder") to handle string_type too, since this
datatype doesn't have any specific byteorder.

nft add rule filter input iifname { "lo", "eth0" } counter

Now the listing shows:

	iifname { "lo", "eth0"}

instead of:

	iifname { "", ""}

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/netlink_delinearize.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index e9a04dd..8762cc9 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -742,7 +742,7 @@  static void payload_dependency_store(struct rule_pp_ctx *ctx,
 	ctx->pdep  = stmt;
 }
 
-static void integer_type_postprocess(struct expr *expr)
+static void lookup_postprocess(struct expr *expr)
 {
 	struct expr *i;
 
@@ -757,7 +757,7 @@  static void integer_type_postprocess(struct expr *expr)
 	case EXPR_SET_REF:
 		list_for_each_entry(i, &expr->set->init->expressions, list) {
 			expr_set_type(i, expr->dtype, expr->byteorder);
-			integer_type_postprocess(i);
+			lookup_postprocess(i);
 		}
 		break;
 	default:
@@ -831,8 +831,12 @@  static void meta_match_postprocess(struct rule_pp_ctx *ctx,
 	case OP_LOOKUP:
 		expr_set_type(expr->right, expr->left->dtype,
 			      expr->left->byteorder);
-		if (expr->right->dtype == &integer_type)
-			integer_type_postprocess(expr->right);
+		switch (expr->right->dtype->type) {
+		case TYPE_STRING:
+		case TYPE_INTEGER:
+			lookup_postprocess(expr->right);
+			break;
+		}
 		break;
 	default:
 		break;
-- 
1.7.10.4