[PATCH iproute2-next 0/2] Support showing a block bound by qevent

[PATCH iproute2-next 0/2] Support showing a block bound by qevent

[Petr Machata]

When a list of filters at a given block is requested, tc first validates
that the block exists before doing the filter query. Currently the
validation routine checks ingress and egress blocks. But now that blocks
can be bound to qevents as well, qevent blocks should be looked for as
well:

    # ip link add up type dummy
    # tc qdisc add dev dummy1 root handle 1: \
         red min 30000 max 60000 avpkt 1000 qevent early_drop block 100
    # tc filter add block 100 pref 1234 handle 102 matchall action drop
    # tc filter show block 100
    Cannot find block "100"

This patchset fixes this issue:

    # tc filter show block 100
    filter protocol all pref 1234 matchall chain 0 
    filter protocol all pref 1234 matchall chain 0 handle 0x66 
      not_in_hw
            action order 1: gact action drop
             random type none pass val 0
             index 2 ref 1 bind 1

In patch #1, the helpers and necessary infrastructure is introduced,
including a new qdisc_util callback that implements sniffing out bound
blocks in a given qdisc.

In patch #2, RED implements the new callback.

Petr Machata (2):
  tc: Look for blocks in qevents
  tc: q_red: Implement has_block for RED

 tc/q_red.c     | 17 +++++++++++++++++
 tc/tc_qdisc.c  | 10 ++++++++++
 tc/tc_qevent.c | 15 +++++++++++++++
 tc/tc_qevent.h |  2 ++
 tc/tc_util.h   |  2 ++
 5 files changed, 46 insertions(+)

-- 
2.20.1

[PATCH iproute2-next 1/2] tc: Look for blocks in qevents

[Petr Machata]

When a list of filters at a given block is requested, tc first validates
that the block exists before doing the filter query. Currently the
validation routine checks ingress and egress blocks. But now that blocks
can be bound to qevents as well, qevent blocks should be looked for as
well.

In order to support that, extend struct qdisc_util with a new callback,
has_block. That should report whether, give the attributes in TCA_OPTIONS,
a blocks with a given number is bound to a qevent. In
tc_qdisc_block_exists_cb(), invoke that callback when set.

Add a helper to the tc_qevent module that walks the list of qevents and
looks for a given block. This is meant to be used by the individual qdiscs.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 tc/tc_qdisc.c  | 10 ++++++++++
 tc/tc_qevent.c | 15 +++++++++++++++
 tc/tc_qevent.h |  2 ++
 tc/tc_util.h   |  2 ++
 4 files changed, 29 insertions(+)

diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c
index 8eb08c34..bea8d3c0 100644
--- a/tc/tc_qdisc.c
+++ b/tc/tc_qdisc.c
@@ -477,7 +477,9 @@ static int tc_qdisc_block_exists_cb(struct nlmsghdr *n, void *arg)
 	struct tc_qdisc_block_exists_ctx *ctx = arg;
 	struct tcmsg *t = NLMSG_DATA(n);
 	struct rtattr *tb[TCA_MAX+1];
+	struct qdisc_util *q = NULL;
 	int len = n->nlmsg_len;
+	const char *kind;
 
 	if (n->nlmsg_type != RTM_NEWQDISC)
 		return 0;
@@ -506,6 +508,14 @@ static int tc_qdisc_block_exists_cb(struct nlmsghdr *n, void *arg)
 		if (block == ctx->block_index)
 			ctx->found = true;
 	}
+
+	kind = rta_getattr_str(tb[TCA_KIND]);
+	q = get_qdisc_kind(kind);
+	if (!q)
+		return -1;
+	if (q->has_block)
+		q->has_block(q, tb[TCA_OPTIONS], ctx->block_index, &ctx->found);
+
 	return 0;
 }
 
diff --git a/tc/tc_qevent.c b/tc/tc_qevent.c
index 1f8e6506..2c010fcf 100644
--- a/tc/tc_qevent.c
+++ b/tc/tc_qevent.c
@@ -92,6 +92,21 @@ void qevents_print(struct qevent_util *qevents, FILE *f)
 		close_json_array(PRINT_ANY, "");
 }
 
+bool qevents_have_block(struct qevent_util *qevents, __u32 block_idx)
+{
+	if (!qevents)
+		return false;
+
+	for (; qevents->id; qevents++) {
+		struct qevent_base *qeb = qevents->data;
+
+		if (qeb->block_idx == block_idx)
+			return true;
+	}
+
+	return false;
+}
+
 int qevents_dump(struct qevent_util *qevents, struct nlmsghdr *n)
 {
 	int err;
diff --git a/tc/tc_qevent.h b/tc/tc_qevent.h
index 574e7cff..d60c3f75 100644
--- a/tc/tc_qevent.h
+++ b/tc/tc_qevent.h
@@ -2,6 +2,7 @@
 #ifndef _TC_QEVENT_H_
 #define _TC_QEVENT_H_
 
+#include <stdbool.h>
 #include <linux/types.h>
 #include <libnetlink.h>
 
@@ -37,6 +38,7 @@ int qevent_parse(struct qevent_util *qevents, int *p_argc, char ***p_argv);
 int qevents_read(struct qevent_util *qevents, struct rtattr **tb);
 int qevents_dump(struct qevent_util *qevents, struct nlmsghdr *n);
 void qevents_print(struct qevent_util *qevents, FILE *f);
+bool qevents_have_block(struct qevent_util *qevents, __u32 block_idx);
 
 struct qevent_plain {
 	struct qevent_base base;
diff --git a/tc/tc_util.h b/tc/tc_util.h
index edc39138..c8af4e95 100644
--- a/tc/tc_util.h
+++ b/tc/tc_util.h
@@ -5,6 +5,7 @@
 #define MAX_MSG 16384
 #include <limits.h>
 #include <linux/if.h>
+#include <stdbool.h>
 
 #include <linux/pkt_sched.h>
 #include <linux/pkt_cls.h>
@@ -40,6 +41,7 @@ struct qdisc_util {
 	int (*parse_copt)(struct qdisc_util *qu, int argc,
 			  char **argv, struct nlmsghdr *n, const char *dev);
 	int (*print_copt)(struct qdisc_util *qu, FILE *f, struct rtattr *opt);
+	int (*has_block)(struct qdisc_util *qu, struct rtattr *opt, __u32 block_idx, bool *p_has);
 };
 
 extern __u16 f_proto;
-- 
2.20.1

Re: [PATCH iproute2-next 1/2] tc: Look for blocks in qevents

[Jiri Pirko]

Thu, Jul 16, 2020 at 12:47:59PM CEST, petrm@mellanox.com wrote:
>When a list of filters at a given block is requested, tc first validates
>that the block exists before doing the filter query. Currently the
>validation routine checks ingress and egress blocks. But now that blocks
>can be bound to qevents as well, qevent blocks should be looked for as
>well.
>
>In order to support that, extend struct qdisc_util with a new callback,
>has_block. That should report whether, give the attributes in TCA_OPTIONS,
>a blocks with a given number is bound to a qevent. In
>tc_qdisc_block_exists_cb(), invoke that callback when set.
>
>Add a helper to the tc_qevent module that walks the list of qevents and
>looks for a given block. This is meant to be used by the individual qdiscs.
>
>Signed-off-by: Petr Machata <petrm@mellanox.com>
>---
> tc/tc_qdisc.c  | 10 ++++++++++
> tc/tc_qevent.c | 15 +++++++++++++++
> tc/tc_qevent.h |  2 ++
> tc/tc_util.h   |  2 ++
> 4 files changed, 29 insertions(+)
>
>diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c
>index 8eb08c34..bea8d3c0 100644
>--- a/tc/tc_qdisc.c
>+++ b/tc/tc_qdisc.c
>@@ -477,7 +477,9 @@ static int tc_qdisc_block_exists_cb(struct nlmsghdr *n, void *arg)
> 	struct tc_qdisc_block_exists_ctx *ctx = arg;
> 	struct tcmsg *t = NLMSG_DATA(n);
> 	struct rtattr *tb[TCA_MAX+1];
>+	struct qdisc_util *q = NULL;

Pointless initialization.


> 	int len = n->nlmsg_len;
>+	const char *kind;
> 
> 	if (n->nlmsg_type != RTM_NEWQDISC)
> 		return 0;
>@@ -506,6 +508,14 @@ static int tc_qdisc_block_exists_cb(struct nlmsghdr *n, void *arg)
> 		if (block == ctx->block_index)
> 			ctx->found = true;
> 	}
>+
>+	kind = rta_getattr_str(tb[TCA_KIND]);
>+	q = get_qdisc_kind(kind);
>+	if (!q)
>+		return -1;
>+	if (q->has_block)
>+		q->has_block(q, tb[TCA_OPTIONS], ctx->block_index, &ctx->found);

Op returns int yet you don't use it. Perhaps it can directly return
bool?


>+
> 	return 0;
> }
> 
>diff --git a/tc/tc_qevent.c b/tc/tc_qevent.c
>index 1f8e6506..2c010fcf 100644
>--- a/tc/tc_qevent.c
>+++ b/tc/tc_qevent.c
>@@ -92,6 +92,21 @@ void qevents_print(struct qevent_util *qevents, FILE *f)
> 		close_json_array(PRINT_ANY, "");
> }
> 
>+bool qevents_have_block(struct qevent_util *qevents, __u32 block_idx)
>+{
>+	if (!qevents)
>+		return false;
>+
>+	for (; qevents->id; qevents++) {
>+		struct qevent_base *qeb = qevents->data;
>+
>+		if (qeb->block_idx == block_idx)
>+			return true;
>+	}
>+
>+	return false;
>+}
>+
> int qevents_dump(struct qevent_util *qevents, struct nlmsghdr *n)
> {
> 	int err;
>diff --git a/tc/tc_qevent.h b/tc/tc_qevent.h
>index 574e7cff..d60c3f75 100644
>--- a/tc/tc_qevent.h
>+++ b/tc/tc_qevent.h
>@@ -2,6 +2,7 @@
> #ifndef _TC_QEVENT_H_
> #define _TC_QEVENT_H_
> 
>+#include <stdbool.h>
> #include <linux/types.h>
> #include <libnetlink.h>
> 
>@@ -37,6 +38,7 @@ int qevent_parse(struct qevent_util *qevents, int *p_argc, char ***p_argv);
> int qevents_read(struct qevent_util *qevents, struct rtattr **tb);
> int qevents_dump(struct qevent_util *qevents, struct nlmsghdr *n);
> void qevents_print(struct qevent_util *qevents, FILE *f);
>+bool qevents_have_block(struct qevent_util *qevents, __u32 block_idx);
> 
> struct qevent_plain {
> 	struct qevent_base base;
>diff --git a/tc/tc_util.h b/tc/tc_util.h
>index edc39138..c8af4e95 100644
>--- a/tc/tc_util.h
>+++ b/tc/tc_util.h
>@@ -5,6 +5,7 @@
> #define MAX_MSG 16384
> #include <limits.h>
> #include <linux/if.h>
>+#include <stdbool.h>
> 
> #include <linux/pkt_sched.h>
> #include <linux/pkt_cls.h>
>@@ -40,6 +41,7 @@ struct qdisc_util {
> 	int (*parse_copt)(struct qdisc_util *qu, int argc,
> 			  char **argv, struct nlmsghdr *n, const char *dev);
> 	int (*print_copt)(struct qdisc_util *qu, FILE *f, struct rtattr *opt);
>+	int (*has_block)(struct qdisc_util *qu, struct rtattr *opt, __u32 block_idx, bool *p_has);
> };
> 
> extern __u16 f_proto;
>-- 
>2.20.1
>

Re: [PATCH iproute2-next 1/2] tc: Look for blocks in qevents

[Petr Machata]

Jiri Pirko <jiri@resnulli.us> writes:

>>diff --git a/tc/tc_qdisc.c b/tc/tc_qdisc.c
>>index 8eb08c34..bea8d3c0 100644
>>--- a/tc/tc_qdisc.c
>>+++ b/tc/tc_qdisc.c
>>@@ -477,7 +477,9 @@ static int tc_qdisc_block_exists_cb(struct nlmsghdr *n, void *arg)
>> 	struct tc_qdisc_block_exists_ctx *ctx = arg;
>> 	struct tcmsg *t = NLMSG_DATA(n);
>> 	struct rtattr *tb[TCA_MAX+1];
>>+	struct qdisc_util *q = NULL;
>
> Pointless initialization.

Ack.

>> 	int len = n->nlmsg_len;
>>+	const char *kind;
>>
>> 	if (n->nlmsg_type != RTM_NEWQDISC)
>> 		return 0;
>>@@ -506,6 +508,14 @@ static int tc_qdisc_block_exists_cb(struct nlmsghdr *n, void *arg)
>> 		if (block == ctx->block_index)
>> 			ctx->found = true;
>> 	}
>>+
>>+	kind = rta_getattr_str(tb[TCA_KIND]);
>>+	q = get_qdisc_kind(kind);
>>+	if (!q)
>>+		return -1;
>>+	if (q->has_block)
>>+		q->has_block(q, tb[TCA_OPTIONS], ctx->block_index, &ctx->found);
>
> Op returns int yet you don't use it. Perhaps it can directly return
> bool?

In theory it could return actual errors (such as an attribute reporting
block 0). I'll have it handle the return value through the usual "err ="
stanza.

>
>>+
>> 	return 0;
>> }
>>
>>diff --git a/tc/tc_qevent.c b/tc/tc_qevent.c
>>index 1f8e6506..2c010fcf 100644
>>--- a/tc/tc_qevent.c
>>+++ b/tc/tc_qevent.c
>>@@ -92,6 +92,21 @@ void qevents_print(struct qevent_util *qevents, FILE *f)
>> 		close_json_array(PRINT_ANY, "");
>> }
>>
>>+bool qevents_have_block(struct qevent_util *qevents, __u32 block_idx)
>>+{
>>+	if (!qevents)
>>+		return false;
>>+
>>+	for (; qevents->id; qevents++) {
>>+		struct qevent_base *qeb = qevents->data;
>>+
>>+		if (qeb->block_idx == block_idx)
>>+			return true;
>>+	}
>>+
>>+	return false;
>>+}
>>+
>> int qevents_dump(struct qevent_util *qevents, struct nlmsghdr *n)
>> {
>> 	int err;
>>diff --git a/tc/tc_qevent.h b/tc/tc_qevent.h
>>index 574e7cff..d60c3f75 100644
>>--- a/tc/tc_qevent.h
>>+++ b/tc/tc_qevent.h
>>@@ -2,6 +2,7 @@
>> #ifndef _TC_QEVENT_H_
>> #define _TC_QEVENT_H_
>>
>>+#include <stdbool.h>
>> #include <linux/types.h>
>> #include <libnetlink.h>
>>
>>@@ -37,6 +38,7 @@ int qevent_parse(struct qevent_util *qevents, int *p_argc, char ***p_argv);
>> int qevents_read(struct qevent_util *qevents, struct rtattr **tb);
>> int qevents_dump(struct qevent_util *qevents, struct nlmsghdr *n);
>> void qevents_print(struct qevent_util *qevents, FILE *f);
>>+bool qevents_have_block(struct qevent_util *qevents, __u32 block_idx);
>>
>> struct qevent_plain {
>> 	struct qevent_base base;
>>diff --git a/tc/tc_util.h b/tc/tc_util.h
>>index edc39138..c8af4e95 100644
>>--- a/tc/tc_util.h
>>+++ b/tc/tc_util.h
>>@@ -5,6 +5,7 @@
>> #define MAX_MSG 16384
>> #include <limits.h>
>> #include <linux/if.h>
>>+#include <stdbool.h>
>>
>> #include <linux/pkt_sched.h>
>> #include <linux/pkt_cls.h>
>>@@ -40,6 +41,7 @@ struct qdisc_util {
>> 	int (*parse_copt)(struct qdisc_util *qu, int argc,
>> 			  char **argv, struct nlmsghdr *n, const char *dev);
>> 	int (*print_copt)(struct qdisc_util *qu, FILE *f, struct rtattr *opt);
>>+	int (*has_block)(struct qdisc_util *qu, struct rtattr *opt, __u32 block_idx, bool *p_has);
>> };
>>
>> extern __u16 f_proto;
>>--
>>2.20.1
>>

[PATCH iproute2-next 2/2] tc: q_red: Implement has_block for RED

[Petr Machata]

In order for "tc filter show block X" to find a given block, implement the
has_block callback.

Signed-off-by: Petr Machata <petrm@mellanox.com>
---
 tc/q_red.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tc/q_red.c b/tc/q_red.c
index 97856f03..dfef1bf8 100644
--- a/tc/q_red.c
+++ b/tc/q_red.c
@@ -264,10 +264,27 @@ static int red_print_xstats(struct qdisc_util *qu, FILE *f, struct rtattr *xstat
 	return 0;
 }
 
+static int red_has_block(struct qdisc_util *qu, struct rtattr *opt, __u32 block_idx, bool *p_has)
+{
+	struct rtattr *tb[TCA_RED_MAX + 1];
+
+	if (opt == NULL)
+		return 0;
+
+	parse_rtattr_nested(tb, TCA_RED_MAX, opt);
+
+	qevents_init(qevents);
+	if (qevents_read(qevents, tb))
+		return -1;
+
+	*p_has = qevents_have_block(qevents, block_idx);
+	return 0;
+}
 
 struct qdisc_util red_qdisc_util = {
 	.id		= "red",
 	.parse_qopt	= red_parse_opt,
 	.print_qopt	= red_print_opt,
 	.print_xstats	= red_print_xstats,
+	.has_block	= red_has_block,
 };
-- 
2.20.1