[PATCH nft 0/3] make the mss and wscale fields optional for synproxy object

[PATCH nft 0/3] make the mss and wscale fields optional for synproxy object

[Zhongqiu Duan]

Patch 1 makes the fields of the synproxy object optional during parsing and
printing. It also makes the order of the fields during parsing insensitive.

Patch 2/3 to print the synproxy more pretty.

Zhongqiu Duan (3):
  src: make the mss and wscale fields optional for synproxy object
  src: do not print unnecessary space for the synproxy object
  src: only print the mss and wscale of synproxy if they are present

 src/json.c                             |  9 ++--
 src/parser_bison.y                     | 63 ++++++++------------------
 src/parser_json.c                      | 26 +++++++----
 src/rule.c                             |  4 +-
 src/statement.c                        |  2 +-
 tests/shell/testcases/json/single_flag |  4 +-
 6 files changed, 48 insertions(+), 60 deletions(-)

-- 
2.43.0

[PATCH nft 1/3] src: make the mss and wscale fields optional for synproxy object

[Zhongqiu Duan]

The mss and wscale fields is optional for synproxy statement, this patch
to make the same behavior for synproxy object, and also makes the
timestamp and sack-perm flags no longer order-sensitive.

Signed-off-by: Zhongqiu Duan <dzq.aishenghu0@gmail.com>
---
 src/json.c         |  9 ++++---
 src/parser_bison.y | 63 +++++++++++++++-------------------------------
 src/parser_json.c  | 26 ++++++++++++-------
 3 files changed, 42 insertions(+), 56 deletions(-)

diff --git a/src/json.c b/src/json.c
index f0430776851c..15ddc4dab790 100644
--- a/src/json.c
+++ b/src/json.c
@@ -469,10 +469,11 @@ static json_t *obj_print_json(const struct obj *obj)
 		json_decref(tmp);
 		break;
 	case NFT_OBJECT_SYNPROXY:
-		tmp = nft_json_pack("{s:i, s:i}",
-				    "mss", obj->synproxy.mss,
-				    "wscale", obj->synproxy.wscale);
-
+		tmp = json_object();
+		if (obj->synproxy.flags & NF_SYNPROXY_OPT_MSS)
+			json_object_set_new(tmp, "mss", json_integer(obj->synproxy.mss));
+		if (obj->synproxy.flags & NF_SYNPROXY_OPT_WSCALE)
+			json_object_set_new(tmp, "wscale", json_integer(obj->synproxy.wscale));
 		flags = json_array();
 		if (obj->synproxy.flags & NF_SYNPROXY_OPT_TIMESTAMP)
 			json_array_append_new(flags, json_string("timestamp"));
diff --git a/src/parser_bison.y b/src/parser_bison.y
index f9cc909836bc..45f2cb4a11f2 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -964,8 +964,6 @@ int nft_lex(void *, void *, void *);
 %destructor { free_const($$); }	monitor_event
 %type <val>			monitor_object	monitor_format
 
-%type <val>			synproxy_ts	synproxy_sack
-
 %type <expr>			tcp_hdr_expr
 %destructor { expr_free($$); }	tcp_hdr_expr
 %type <val>			tcp_hdr_field
@@ -2662,7 +2660,7 @@ secmark_block		:	/* empty */	{ $$ = $<obj>-1; }
 synproxy_block		:	/* empty */	{ $$ = $<obj>-1; }
 			|	synproxy_block	common_block
 			|	synproxy_block	stmt_separator
-			|	synproxy_block	synproxy_config
+			|	synproxy_block	synproxy_config_arg
 			{
 				$$ = $1;
 			}
@@ -3807,58 +3805,37 @@ synproxy_arg		:	MSS	NUM
 			}
 			;
 
-synproxy_config		:	MSS	NUM	WSCALE	NUM	synproxy_ts	synproxy_sack
+synproxy_config		:	synproxy_config_arg
 			{
-				struct synproxy *synproxy;
-				uint32_t flags = 0;
-
-				synproxy = &$<obj>0->synproxy;
-				synproxy->mss = $2;
-				flags |= NF_SYNPROXY_OPT_MSS;
-				synproxy->wscale = $4;
-				flags |= NF_SYNPROXY_OPT_WSCALE;
-				if ($5)
-					flags |= $5;
-				if ($6)
-					flags |= $6;
-				synproxy->flags = flags;
-			}
-			|	MSS	NUM	stmt_separator	WSCALE	NUM	stmt_separator	synproxy_ts	synproxy_sack
-			{
-				struct synproxy *synproxy;
-				uint32_t flags = 0;
-
-				synproxy = &$<obj>0->synproxy;
-				synproxy->mss = $2;
-				flags |= NF_SYNPROXY_OPT_MSS;
-				synproxy->wscale = $5;
-				flags |= NF_SYNPROXY_OPT_WSCALE;
-				if ($7)
-					flags |= $7;
-				if ($8)
-					flags |= $8;
-				synproxy->flags = flags;
+				$<obj>$	= $<obj>0;
 			}
+			|	synproxy_config	synproxy_config_arg
 			;
 
-synproxy_obj		:	/* empty */
+synproxy_config_arg	:	MSS	NUM
 			{
-				$$ = obj_alloc(&@$);
-				$$->type = NFT_OBJECT_SYNPROXY;
+				$<obj>0->synproxy.mss = $2;
+				$<obj>0->synproxy.flags |= NF_SYNPROXY_OPT_MSS;
+			}
+			|	WSCALE	NUM
+			{
+				$<obj>0->synproxy.wscale = $2;
+				$<obj>0->synproxy.flags |= NF_SYNPROXY_OPT_WSCALE;
 			}
-			;
-
-synproxy_ts		:	/* empty */	{ $$ = 0; }
 			|	TIMESTAMP
 			{
-				$$ = NF_SYNPROXY_OPT_TIMESTAMP;
+				$<obj>0->synproxy.flags |= NF_SYNPROXY_OPT_TIMESTAMP;
+			}
+			|	SACK_PERM
+			{
+				$<obj>0->synproxy.flags |= NF_SYNPROXY_OPT_SACK_PERM;
 			}
 			;
 
-synproxy_sack		:	/* empty */	{ $$ = 0; }
-			|	SACK_PERM
+synproxy_obj		:	/* empty */
 			{
-				$$ = NF_SYNPROXY_OPT_SACK_PERM;
+				$$ = obj_alloc(&@$);
+				$$->type = NFT_OBJECT_SYNPROXY;
 			}
 			;
 
diff --git a/src/parser_json.c b/src/parser_json.c
index 08657f2849a5..dc1431e1711c 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3493,7 +3493,7 @@ static struct cmd *json_parse_cmd_add_object(struct json_ctx *ctx,
 {
 	const char *family, *tmp, *rate_unit = "packets", *burst_unit = "bytes";
 	uint32_t l3proto = NFPROTO_UNSPEC;
-	int inv = 0, flags = 0, i, j;
+	int inv = 0, flags = 0, i;
 	struct handle h = { 0 };
 	struct obj *obj;
 
@@ -3667,14 +3667,22 @@ static struct cmd *json_parse_cmd_add_object(struct json_ctx *ctx,
 		break;
 	case CMD_OBJ_SYNPROXY:
 		obj->type = NFT_OBJECT_SYNPROXY;
-		if (json_unpack_err(ctx, root, "{s:i, s:i}",
-				    "mss", &i, "wscale", &j))
-			goto err_free_obj;
-
-		obj->synproxy.mss = i;
-		obj->synproxy.wscale = j;
-		obj->synproxy.flags |= NF_SYNPROXY_OPT_MSS;
-		obj->synproxy.flags |= NF_SYNPROXY_OPT_WSCALE;
+		if (!json_unpack(root, "{s:i}", "mss", &i)) {
+			if (i < 0) {
+				json_error(ctx, "Invalid synproxy mss value '%d'", i);
+				goto err_free_obj;
+			}
+			obj->synproxy.mss = i;
+			obj->synproxy.flags |= NF_SYNPROXY_OPT_MSS;
+		}
+		if (!json_unpack(root, "{s:i}", "wscale", &i)) {
+			if (i < 0) {
+				json_error(ctx, "Invalid synproxy wscale value '%d'", i);
+				goto err_free_obj;
+			}
+			obj->synproxy.wscale = i;
+			obj->synproxy.flags |= NF_SYNPROXY_OPT_WSCALE;
+		}
 		flags = parse_flags_array(ctx, root, "flags",
 					  json_parse_synproxy_flag);
 		if (flags < 0)
-- 
2.43.0

Re: [PATCH nft 1/3] src: make the mss and wscale fields optional for synproxy object

[Florian Westphal]

Zhongqiu Duan <dzq.aishenghu0@gmail.com> wrote:
> The mss and wscale fields is optional for synproxy statement, this patch
> to make the same behavior for synproxy object, and also makes the
> timestamp and sack-perm flags no longer order-sensitive.

Whats the use case for omitting the mss field?
It seems this should be made mandatory, no?

Also I think we should reject wscale > 14 from the parsers (can be done
in extra patch).

And also reject it in kernel by updating the nla_policy in
net/netfiler/nft_synproxy.c in the kernel.

Re: [PATCH nft 1/3] src: make the mss and wscale fields optional for synproxy object

[Zhongqiu Duan]

On Fri, Jul 4, 2025 at 8:27 PM Florian Westphal <fw@strlen.de> wrote:
>
> Zhongqiu Duan <dzq.aishenghu0@gmail.com> wrote:
> > The mss and wscale fields is optional for synproxy statement, this patch
> > to make the same behavior for synproxy object, and also makes the
> > timestamp and sack-perm flags no longer order-sensitive.
>
> Whats the use case for omitting the mss field?
> It seems this should be made mandatory, no?
>

Agree, I think mss should be set in almost all cases.

This patch is mainly to keep the same syntax support between the synproxy
statement and object.

> Also I think we should reject wscale > 14 from the parsers (can be done
> in extra patch).
>
> And also reject it in kernel by updating the nla_policy in
> net/netfiler/nft_synproxy.c in the kernel.

[PATCH nft 2/3] src: do not print unnecessary space for the synproxy object

[Zhongqiu Duan]

If timestamp is not enabled in the synproxy object, an additional space
will be print before the sack-perm flag.

Before this patch:

table inet t {
	synproxy s {
		mss 1460
		wscale 8
		 sack-perm
	}
}

After this patch:

table inet t {
	synproxy s {
		mss 1460
		wscale 8
		sack-perm
	}
}

Signed-off-by: Zhongqiu Duan <dzq.aishenghu0@gmail.com>
---
 src/rule.c                             | 4 +++-
 tests/shell/testcases/json/single_flag | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index c0f7570e233c..af3dd39c69d0 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1951,7 +1951,9 @@ static void obj_print_data(const struct obj *obj,
 		}
 		if (flags & (NF_SYNPROXY_OPT_TIMESTAMP | NF_SYNPROXY_OPT_SACK_PERM)) {
 			nft_print(octx, "%s%s%s", opts->nl, opts->tab, opts->tab);
-			nft_print(octx, "%s %s", ts_str, sack_str);
+			nft_print(octx, "%s%s%s", ts_str,
+				  flags & NF_SYNPROXY_OPT_TIMESTAMP ? " " : "",
+				  sack_str);
 		}
 		nft_print(octx, "%s", opts->stmt_separator);
 		}
diff --git a/tests/shell/testcases/json/single_flag b/tests/shell/testcases/json/single_flag
index f0a608ad8412..b8fd96170a33 100755
--- a/tests/shell/testcases/json/single_flag
+++ b/tests/shell/testcases/json/single_flag
@@ -157,13 +157,13 @@ STD_SYNPROXY_OBJ_1="table ip t {
 	synproxy s {
 		mss 1280
 		wscale 64
-		 sack-perm
+		sack-perm
 	}
 }"
 JSON_SYNPROXY_OBJ_1='{"nftables": [{"table": {"family": "ip", "name": "t", "handle": 0}}, {"synproxy": {"family": "ip", "name": "s", "table": "t", "handle": 0, "mss": 1280, "wscale": 64, "flags": "sack-perm"}}]}'
 JSON_SYNPROXY_OBJ_1_EQUIV=$(sed 's/\("flags":\) \([^}]*\)/\1 [\2]/' <<< "$JSON_SYNPROXY_OBJ_1")
 
-STD_SYNPROXY_OBJ_2=$(sed 's/ \(sack-perm\)/timestamp \1/' <<< "$STD_SYNPROXY_OBJ_1")
+STD_SYNPROXY_OBJ_2=$(sed 's/\(sack-perm\)/timestamp \1/' <<< "$STD_SYNPROXY_OBJ_1")
 JSON_SYNPROXY_OBJ_2=$(sed 's/\("flags":\) \("sack-perm"\)/\1 ["timestamp", \2]/' <<< "$JSON_SYNPROXY_OBJ_1")
 
 back_n_forth "$STD_SYNPROXY_OBJ_1" "$JSON_SYNPROXY_OBJ_1"
-- 
2.43.0

Re: [PATCH nft 2/3] src: do not print unnecessary space for the synproxy object

[Zhongqiu Duan]

On Fri, Jul 4, 2025 at 7:40 PM Zhongqiu Duan <dzq.aishenghu0@gmail.com> wrote:
>
> If timestamp is not enabled in the synproxy object, an additional space
> will be print before the sack-perm flag.
>
> Before this patch:
>
> table inet t {
>         synproxy s {
>                 mss 1460
>                 wscale 8
>                  sack-perm
>         }
> }
>
> After this patch:
>
> table inet t {
>         synproxy s {
>                 mss 1460
>                 wscale 8
>                 sack-perm
>         }
> }
>
> Signed-off-by: Zhongqiu Duan <dzq.aishenghu0@gmail.com>
> ---
>  src/rule.c                             | 4 +++-
>  tests/shell/testcases/json/single_flag | 4 ++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/rule.c b/src/rule.c
> index c0f7570e233c..af3dd39c69d0 100644
> --- a/src/rule.c
> +++ b/src/rule.c
> @@ -1951,7 +1951,9 @@ static void obj_print_data(const struct obj *obj,
>                 }
>                 if (flags & (NF_SYNPROXY_OPT_TIMESTAMP | NF_SYNPROXY_OPT_SACK_PERM)) {
>                         nft_print(octx, "%s%s%s", opts->nl, opts->tab, opts->tab);
> -                       nft_print(octx, "%s %s", ts_str, sack_str);
> +                       nft_print(octx, "%s%s%s", ts_str,
> +                                 flags & NF_SYNPROXY_OPT_TIMESTAMP ? " " : "",
> +                                 sack_str);
>                 }
>                 nft_print(octx, "%s", opts->stmt_separator);
>                 }

Emmm, this will add an additional space after the timestamp if the timestamp
is set but sack-perm does not.

It could be:
---
diff --git a/src/rule.c b/src/rule.c
index c0f7570e233c..3761e05a22e3 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1950,8 +1950,10 @@ static void obj_print_data(const struct obj *obj,
                        nft_print(octx, "wscale %u", obj->synproxy.wscale);
                }
                if (flags & (NF_SYNPROXY_OPT_TIMESTAMP |
NF_SYNPROXY_OPT_SACK_PERM)) {
+                       bool space = ((flags & NF_SYNPROXY_OPT_TIMESTAMP) &&
+                                     (flags & NF_SYNPROXY_OPT_SACK_PERM));
                        nft_print(octx, "%s%s%s", opts->nl, opts->tab,
opts->tab);
-                       nft_print(octx, "%s %s", ts_str, sack_str);
+                       nft_print(octx, "%s%s%s", ts_str, space ? " "
: "", sack_str);

                }
                nft_print(octx, "%s", opts->stmt_separator);
                }

> diff --git a/tests/shell/testcases/json/single_flag b/tests/shell/testcases/json/single_flag
> index f0a608ad8412..b8fd96170a33 100755
> --- a/tests/shell/testcases/json/single_flag
> +++ b/tests/shell/testcases/json/single_flag
> @@ -157,13 +157,13 @@ STD_SYNPROXY_OBJ_1="table ip t {
>         synproxy s {
>                 mss 1280
>                 wscale 64
> -                sack-perm
> +               sack-perm
>         }
>  }"
>  JSON_SYNPROXY_OBJ_1='{"nftables": [{"table": {"family": "ip", "name": "t", "handle": 0}}, {"synproxy": {"family": "ip", "name": "s", "table": "t", "handle": 0, "mss": 1280, "wscale": 64, "flags": "sack-perm"}}]}'
>  JSON_SYNPROXY_OBJ_1_EQUIV=$(sed 's/\("flags":\) \([^}]*\)/\1 [\2]/' <<< "$JSON_SYNPROXY_OBJ_1")
>
> -STD_SYNPROXY_OBJ_2=$(sed 's/ \(sack-perm\)/timestamp \1/' <<< "$STD_SYNPROXY_OBJ_1")
> +STD_SYNPROXY_OBJ_2=$(sed 's/\(sack-perm\)/timestamp \1/' <<< "$STD_SYNPROXY_OBJ_1")
>  JSON_SYNPROXY_OBJ_2=$(sed 's/\("flags":\) \("sack-perm"\)/\1 ["timestamp", \2]/' <<< "$JSON_SYNPROXY_OBJ_1")
>
>  back_n_forth "$STD_SYNPROXY_OBJ_1" "$JSON_SYNPROXY_OBJ_1"
> --
> 2.43.0
>

[PATCH nft 3/3] src: only print the mss and wscale of synproxy if they are present

[Zhongqiu Duan]

The listing of the synproxy statement will print a zero value
for unpresented fields.

e.g., the rule add by `nft add rule inet t c synproxy wscale 8 sack-perm`
will print as 'synproxy mss 0 wscale 8 sack-perm'.

Signed-off-by: Zhongqiu Duan <dzq.aishenghu0@gmail.com>
---
 src/statement.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/statement.c b/src/statement.c
index 695b57a6cc65..ced002f63115 100644
--- a/src/statement.c
+++ b/src/statement.c
@@ -1058,7 +1058,7 @@ static void synproxy_stmt_print(const struct stmt *stmt,
 	const char *ts_str = synproxy_timestamp_to_str(flags);
 	const char *sack_str = synproxy_sack_to_str(flags);
 
-	if (flags & (NF_SYNPROXY_OPT_MSS | NF_SYNPROXY_OPT_WSCALE))
+	if ((flags & NF_SYNPROXY_OPT_MSS) && (flags & NF_SYNPROXY_OPT_WSCALE))
 		nft_print(octx, "synproxy mss %u wscale %u%s%s",
 			  stmt->synproxy.mss, stmt->synproxy.wscale,
 			  ts_str, sack_str);
-- 
2.43.0

Re: [PATCH nft 3/3] src: only print the mss and wscale of synproxy if they are present

[Florian Westphal]

Zhongqiu Duan <dzq.aishenghu0@gmail.com> wrote:
> The listing of the synproxy statement will print a zero value
> for unpresented fields.
> 
> e.g., the rule add by `nft add rule inet t c synproxy wscale 8 sack-perm`
> will print as 'synproxy mss 0 wscale 8 sack-perm'.
> 
> Signed-off-by: Zhongqiu Duan <dzq.aishenghu0@gmail.com>
> ---
>  src/statement.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/statement.c b/src/statement.c
> index 695b57a6cc65..ced002f63115 100644
> --- a/src/statement.c
> +++ b/src/statement.c
> @@ -1058,7 +1058,7 @@ static void synproxy_stmt_print(const struct stmt *stmt,
>  	const char *ts_str = synproxy_timestamp_to_str(flags);
>  	const char *sack_str = synproxy_sack_to_str(flags);
>  
> -	if (flags & (NF_SYNPROXY_OPT_MSS | NF_SYNPROXY_OPT_WSCALE))
> +	if ((flags & NF_SYNPROXY_OPT_MSS) && (flags & NF_SYNPROXY_OPT_WSCALE))
>  		nft_print(octx, "synproxy mss %u wscale %u%s%s",
>  			  stmt->synproxy.mss, stmt->synproxy.wscale,
>  			  ts_str, sack_str);

This looks wrong, this will now only print it if both are set.

Re: [PATCH nft 3/3] src: only print the mss and wscale of synproxy if they are present

[Zhongqiu Duan]

On Fri, Jul 4, 2025 at 7:54 PM Florian Westphal <fw@strlen.de> wrote:
>
> Zhongqiu Duan <dzq.aishenghu0@gmail.com> wrote:
> > The listing of the synproxy statement will print a zero value
> > for unpresented fields.
> >
> > e.g., the rule add by `nft add rule inet t c synproxy wscale 8 sack-perm`
> > will print as 'synproxy mss 0 wscale 8 sack-perm'.
> >
> > Signed-off-by: Zhongqiu Duan <dzq.aishenghu0@gmail.com>
> > ---
> >  src/statement.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/src/statement.c b/src/statement.c
> > index 695b57a6cc65..ced002f63115 100644
> > --- a/src/statement.c
> > +++ b/src/statement.c
> > @@ -1058,7 +1058,7 @@ static void synproxy_stmt_print(const struct stmt *stmt,
> >       const char *ts_str = synproxy_timestamp_to_str(flags);
> >       const char *sack_str = synproxy_sack_to_str(flags);
> >
> > -     if (flags & (NF_SYNPROXY_OPT_MSS | NF_SYNPROXY_OPT_WSCALE))
> > +     if ((flags & NF_SYNPROXY_OPT_MSS) && (flags & NF_SYNPROXY_OPT_WSCALE))
> >               nft_print(octx, "synproxy mss %u wscale %u%s%s",
> >                         stmt->synproxy.mss, stmt->synproxy.wscale,
> >                         ts_str, sack_str);
>
> This looks wrong, this will now only print it if both are set.

The following else branch will handle the other conditions.

Regards,
Zhongqiu

Re: [PATCH nft 3/3] src: only print the mss and wscale of synproxy if they are present

[Florian Westphal]

Zhongqiu Duan <dzq.aishenghu0@gmail.com> wrote:
> > > -     if (flags & (NF_SYNPROXY_OPT_MSS | NF_SYNPROXY_OPT_WSCALE))
> > > +     if ((flags & NF_SYNPROXY_OPT_MSS) && (flags & NF_SYNPROXY_OPT_WSCALE))
> > >               nft_print(octx, "synproxy mss %u wscale %u%s%s",
> > >                         stmt->synproxy.mss, stmt->synproxy.wscale,
> > >                         ts_str, sack_str);
> >
> > This looks wrong, this will now only print it if both are set.
> 
> The following else branch will handle the other conditions.

Urgh, indeed, orginal conditional makes no sense.