[PATCH 0/3] q_cake: minor fixes and cleanups

[PATCH 0/3] q_cake: minor fixes and cleanups

[Odin Ugedal]

Some minor changes/fixes to the qdisc cake implementation.

Odin Ugedal (3):
  q_cake: Make fwmark uint instead of int
  q_cake: properly print memlimit
  q_cake: detect overflow in get_size

 tc/q_cake.c  | 15 ++++++++-------
 tc/tc_util.c |  5 +++++
 2 files changed, 13 insertions(+), 7 deletions(-)

-- 
2.26.1

[PATCH 1/3] q_cake: Make fwmark uint instead of int

[Odin Ugedal]

This will help avoid overflow, since setting it to 0xffffffff would
result in -1 when converted to integer, resulting in being "-1", setting
the fwmark to 0x00.

Signed-off-by: Odin Ugedal <odin@ugedal.com>
---
 tc/q_cake.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/tc/q_cake.c b/tc/q_cake.c
index 3c78b176..9ebb270c 100644
--- a/tc/q_cake.c
+++ b/tc/q_cake.c
@@ -97,6 +97,7 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 	unsigned int interval = 0;
 	unsigned int diffserv = 0;
 	unsigned int memlimit = 0;
+	unsigned int fwmark = 0;
 	unsigned int target = 0;
 	__u64 bandwidth = 0;
 	int ack_filter = -1;
@@ -107,7 +108,6 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 	int autorate = -1;
 	int ingress = -1;
 	int overhead = 0;
-	int fwmark = -1;
 	int wash = -1;
 	int nat = -1;
 	int atm = -1;
@@ -335,15 +335,12 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 				return -1;
 			}
 		} else if (strcmp(*argv, "fwmark") == 0) {
-			unsigned int fwm;
-
 			NEXT_ARG();
-			if (get_u32(&fwm, *argv, 0)) {
+			if (get_u32(&fwmark, *argv, 0)) {
 				fprintf(stderr,
 					"Illegal value for \"fwmark\": \"%s\"\n", *argv);
 				return -1;
 			}
-			fwmark = fwm;
 		} else if (strcmp(*argv, "help") == 0) {
 			explain();
 			return -1;
@@ -388,7 +385,7 @@ static int cake_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 	if (memlimit)
 		addattr_l(n, 1024, TCA_CAKE_MEMORY, &memlimit,
 			  sizeof(memlimit));
-	if (fwmark != -1)
+	if (fwmark)
 		addattr_l(n, 1024, TCA_CAKE_FWMARK, &fwmark,
 			  sizeof(fwmark));
 	if (nat != -1)
-- 
2.26.1

[PATCH 2/3] q_cake: properly print memlimit

[Odin Ugedal]

Load memlimit so that it will be printed if it isn't set to zero.

Also add a space to properly print it.

Signed-off-by: Odin Ugedal <odin@ugedal.com>
---
 tc/q_cake.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tc/q_cake.c b/tc/q_cake.c
index 9ebb270c..12d648d7 100644
--- a/tc/q_cake.c
+++ b/tc/q_cake.c
@@ -520,6 +520,10 @@ static int cake_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	    RTA_PAYLOAD(tb[TCA_CAKE_RTT]) >= sizeof(__u32)) {
 		interval = rta_getattr_u32(tb[TCA_CAKE_RTT]);
 	}
+	if (tb[TCA_CAKE_MEMORY] &&
+		RTA_PAYLOAD(tb[TCA_CAKE_MEMORY]) >= sizeof(__u32)) {
+		memlimit = rta_getattr_u32(tb[TCA_CAKE_MEMORY]);
+	}
 	if (tb[TCA_CAKE_FWMARK] &&
 	    RTA_PAYLOAD(tb[TCA_CAKE_FWMARK]) >= sizeof(__u32)) {
 		fwmark = rta_getattr_u32(tb[TCA_CAKE_FWMARK]);
@@ -572,7 +576,7 @@ static int cake_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 
 	if (memlimit) {
 		print_uint(PRINT_JSON, "memlimit", NULL, memlimit);
-		print_string(PRINT_FP, NULL, "memlimit %s",
+		print_string(PRINT_FP, NULL, "memlimit %s ",
 			     sprint_size(memlimit, b1));
 	}
 
-- 
2.26.1

[PATCH 3/3] q_cake: detect overflow in get_size

[Odin Ugedal]

This detects overflow during parsing of value:

eg. running:

$ tc qdisc add dev lo root cake memlimit 11gb

currently gives a memlimit of "3072Mb", while with this patch it errors
with 'illegal value for "memlimit": "11gb"', since memlinit is an
unsigned integer.

Signed-off-by: Odin Ugedal <odin@ugedal.com>
---
 tc/tc_util.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tc/tc_util.c b/tc/tc_util.c
index 5f13d729..68938fb0 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -385,6 +385,11 @@ int get_size(unsigned int *size, const char *str)
 	}
 
 	*size = sz;
+
+	/* detect if an overflow happened */
+	if (*size != floor(sz))
+		return -1;
+
 	return 0;
 }
 
-- 
2.26.1

Re: [PATCH 1/3] q_cake: Make fwmark uint instead of int

[Toke Høiland-Jørgensen]

Odin Ugedal <odin@ugedal.com> writes:

> This will help avoid overflow, since setting it to 0xffffffff would
> result in -1 when converted to integer, resulting in being "-1", setting
> the fwmark to 0x00.
>
> Signed-off-by: Odin Ugedal <odin@ugedal.com>

Thanks!

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCH 2/3] q_cake: properly print memlimit

[Toke Høiland-Jørgensen]

Odin Ugedal <odin@ugedal.com> writes:

> Load memlimit so that it will be printed if it isn't set to zero.
>
> Also add a space to properly print it.
>
> Signed-off-by: Odin Ugedal <odin@ugedal.com>

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCH 3/3] q_cake: detect overflow in get_size

[Toke Høiland-Jørgensen]

Odin Ugedal <odin@ugedal.com> writes:

> This detects overflow during parsing of value:
>
> eg. running:
>
> $ tc qdisc add dev lo root cake memlimit 11gb
>
> currently gives a memlimit of "3072Mb", while with this patch it errors
> with 'illegal value for "memlimit": "11gb"', since memlinit is an
> unsigned integer.
>
> Signed-off-by: Odin Ugedal <odin@ugedal.com>

> ---
>  tc/tc_util.c | 5 +++++

This is not strictly a change to q_cake, so think you have to change the
subject prefix (e.g. to "tc_util: detect overflow in get_size"). You can
still say in the commit message that you found the issue while testing
the cake code, though.

With that change:

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

[PATCH v2 3/3] tc_util: detect overflow in get_size

[Odin Ugedal]

This detects overflow during parsing of value using get_size:

eg. running:

$ tc qdisc add dev lo root cake memlimit 11gb

currently gives a memlimit of "3072Mb", while with this patch it errors
with 'illegal value for "memlimit": "11gb"', since memlinit is an
unsigned integer.

Signed-off-by: Odin Ugedal <odin@ugedal.com>
---
 tc/tc_util.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tc/tc_util.c b/tc/tc_util.c
index 5f13d729..68938fb0 100644
--- a/tc/tc_util.c
+++ b/tc/tc_util.c
@@ -385,6 +385,11 @@ int get_size(unsigned int *size, const char *str)
 	}
 
 	*size = sz;
+
+	/* detect if an overflow happened */
+	if (*size != floor(sz))
+		return -1;
+
 	return 0;
 }
 
-- 
2.26.1

Re: [PATCH 3/3] q_cake: detect overflow in get_size

[Odin Ugedal]

Good catch, thanks! Will (try to) resend patch 3/3 with updated changelog.

tor. 16. apr. 2020 kl. 10:49 skrev Toke Høiland-Jørgensen <toke@redhat.com>:
>
>
> This is not strictly a change to q_cake, so think you have to change the
> subject prefix (e.g. to "tc_util: detect overflow in get_size"). You can
> still say in the commit message that you found the issue while testing
> the cake code, though.
>
> With that change:
>
> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
>

Re: [PATCH v2 3/3] tc_util: detect overflow in get_size

[Toke Høiland-Jørgensen]

Odin Ugedal <odin@ugedal.com> writes:

> This detects overflow during parsing of value using get_size:
>
> eg. running:
>
> $ tc qdisc add dev lo root cake memlimit 11gb
>
> currently gives a memlimit of "3072Mb", while with this patch it errors
> with 'illegal value for "memlimit": "11gb"', since memlinit is an
> unsigned integer.
>
> Signed-off-by: Odin Ugedal <odin@ugedal.com>

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCH 0/3] q_cake: minor fixes and cleanups

[Stephen Hemminger]

On Wed, 15 Apr 2020 16:39:33 +0200
Odin Ugedal <odin@ugedal.com> wrote:

> Some minor changes/fixes to the qdisc cake implementation.
> 
> Odin Ugedal (3):
>   q_cake: Make fwmark uint instead of int
>   q_cake: properly print memlimit
>   q_cake: detect overflow in get_size
> 
>  tc/q_cake.c  | 15 ++++++++-------
>  tc/tc_util.c |  5 +++++
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 

Applied, thanks.