[PATCH iproute2-next] m_mirred: Allow mirred to block

[PATCH iproute2-next] m_mirred: Allow mirred to block

[Victor Nogueira]

So far the mirred action has dealt with syntax that handles
mirror/redirection for netdev. A matching packet is redirected or mirrored
to a target netdev.

In this patch we enable mirred to mirror to a tc block as well.
IOW, the new syntax looks as follows:
... mirred <ingress | egress> <mirror | redirect> [index INDEX] < <blockid BLOCKID> | <dev <devname>> >

Examples of mirroring or redirecting to a tc block:
$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 192.168.0.0/16 action mirred egress mirror blockid 22

$ tc filter add block 22 protocol ip pref 25 \
  flower dst_ip 10.10.10.10/32 action mirred egress redirect blockid 22

Co-developed-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 tc/m_mirred.c | 60 +++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 11 deletions(-)

diff --git a/tc/m_mirred.c b/tc/m_mirred.c
index e5653e67f..db847b1a3 100644
--- a/tc/m_mirred.c
+++ b/tc/m_mirred.c
@@ -29,7 +29,11 @@ explain(void)
 		"\tDIRECTION := <ingress | egress>\n"
 		"\tACTION := <mirror | redirect>\n"
 		"\tINDEX  is the specific policy instance id\n"
-		"\tDEVICENAME is the devicename\n");
+		"\tTARGET := <BLOCK | DEVICE>\n"
+		"\tDEVICE := dev DEVICENAME\n"
+		"\tDEVICENAME is the devicename\n"
+		"\tBLOCK := blockid BLOCKID\n"
+		"\tBLOCKID := 32-bit unsigned block ID\n");
 }
 
 static void
@@ -94,6 +98,7 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
 	struct tc_mirred p = {};
 	struct rtattr *tail;
 	char d[IFNAMSIZ] = {};
+	__u32 blockid = 0;
 
 	while (argc > 0) {
 
@@ -162,15 +167,38 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
 					TCA_INGRESS_REDIR;
 				p.action = TC_ACT_STOLEN;
 				ok++;
-			} else if ((redir || mirror) &&
-				   matches(*argv, "dev") == 0) {
-				NEXT_ARG();
-				if (strlen(d))
-					duparg("dev", *argv);
-
-				strncpy(d, *argv, sizeof(d)-1);
-				argc--;
-				argv++;
+			} else if ((redir || mirror)) {
+				if (matches(*argv, "blockid") == 0) {
+					if (strlen(d)) {
+						fprintf(stderr,
+							"Mustn't specify blockid and dev simultaneously\n");
+						return -1;
+					}
+					NEXT_ARG();
+					if (get_u32(&blockid, *argv, 0) ||
+					    !blockid) {
+						fprintf(stderr,
+							"invalid block ID index value %s",
+							*argv);
+						return -1;
+					}
+					argc--;
+					argv++;
+				}
+				if (argc && matches(*argv, "dev") == 0) {
+					if (blockid) {
+						fprintf(stderr,
+							"Mustn't specify blockid and dev simultaneously\n");
+						return -1;
+					}
+					NEXT_ARG();
+					if (strlen(d))
+						duparg("dev", *argv);
+
+					strncpy(d, *argv, sizeof(d)-1);
+					argc--;
+					argv++;
+				}
 
 				break;
 
@@ -220,6 +248,8 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
 
 	tail = addattr_nest(n, MAX_MSG, tca_id);
 	addattr_l(n, MAX_MSG, TCA_MIRRED_PARMS, &p, sizeof(p));
+	if (blockid)
+		addattr32(n, MAX_MSG, TCA_MIRRED_BLOCKID, blockid);
 	addattr_nest_end(n, tail);
 
 	*argc_p = argc;
@@ -299,7 +329,15 @@ print_mirred(struct action_util *au, FILE *f, struct rtattr *arg)
 		     mirred_action(p->eaction));
 	print_string(PRINT_JSON, "direction", NULL,
 		     mirred_direction(p->eaction));
-	print_string(PRINT_ANY, "to_dev", " to device %s)", dev);
+	if (tb[TCA_MIRRED_BLOCKID]) {
+		const __u32 *blockid = RTA_DATA(tb[TCA_MIRRED_BLOCKID]);
+
+		print_uint(PRINT_ANY, "to_blockid", " to blockid %u)",
+			   *blockid);
+	} else {
+		print_string(PRINT_ANY, "to_dev", " to device %s)", dev);
+	}
+
 	print_action_control(f, " ", p->action, "");
 
 	print_nl();
-- 
2.25.1

Re: [PATCH iproute2-next] m_mirred: Allow mirred to block

[Stephen Hemminger]

On Tue, 23 Jan 2024 13:11:15 -0300
Victor Nogueira <victor@mojatatu.com> wrote:

> +		print_string(PRINT_ANY, "to_dev", " to device %s)", dev);

Suggestion for future.
Use colorized device name here.
		print_color_string(PRINT_ANY, COLOR_IFNAME, "to_dev", " to device %s)", dev);

Re: [PATCH iproute2-next] m_mirred: Allow mirred to block

[Victor Nogueira]

On 23/01/2024 14:18, Stephen Hemminger wrote:
> On Tue, 23 Jan 2024 13:11:15 -0300
> Victor Nogueira <victor@mojatatu.com> wrote:
> 
>> +		print_string(PRINT_ANY, "to_dev", " to device %s)", dev);
> 
> Suggestion for future.
> Use colorized device name here.
> 		print_color_string(PRINT_ANY, COLOR_IFNAME, "to_dev", " to device %s)", dev);

Thank you for the suggestion.
Will send a patch after this one to fix this.

cheers,
Victor

Re: [PATCH iproute2-next] m_mirred: Allow mirred to block

[David Ahern]

On 1/23/24 9:11 AM, Victor Nogueira wrote:
> ---
>  tc/m_mirred.c | 60 +++++++++++++++++++++++++++++++++++++++++----------

missing the man page update

>  1 file changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/tc/m_mirred.c b/tc/m_mirred.c
> index e5653e67f..db847b1a3 100644
> --- a/tc/m_mirred.c
> +++ b/tc/m_mirred.c
> @@ -162,15 +167,38 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
>  					TCA_INGRESS_REDIR;
>  				p.action = TC_ACT_STOLEN;
>  				ok++;
> -			} else if ((redir || mirror) &&
> -				   matches(*argv, "dev") == 0) {
> -				NEXT_ARG();
> -				if (strlen(d))
> -					duparg("dev", *argv);
> -
> -				strncpy(d, *argv, sizeof(d)-1);
> -				argc--;
> -				argv++;
> +			} else if ((redir || mirror)) {
> +				if (matches(*argv, "blockid") == 0) {

Not accepting any more uses of matches.

> +					if (strlen(d)) {
> +						fprintf(stderr,
> +							"Mustn't specify blockid and dev simultaneously\n");
> +						return -1;
> +					}
> +					NEXT_ARG();
> +					if (get_u32(&blockid, *argv, 0) ||
> +					    !blockid) {
> +						fprintf(stderr,
> +							"invalid block ID index value %s",
> +							*argv);
> +						return -1;
> +					}
> +					argc--;
> +					argv++;
> +				}
> +				if (argc && matches(*argv, "dev") == 0) {

This one is legacy, but I really would like to see it as 'dev' (versus
just 'd' or 'de'). Such a change is risky from a backwards
compatibility, so I guess we are stuck with it.


> +					if (blockid) {
> +						fprintf(stderr,
> +							"Mustn't specify blockid and dev simultaneously\n");
> +						return -1;
> +					}
> +					NEXT_ARG();
> +					if (strlen(d))
> +						duparg("dev", *argv);
> +
> +					strncpy(d, *argv, sizeof(d)-1);
> +					argc--;
> +					argv++;
> +				}
>  
>  				break;
>  

Re: [PATCH iproute2-next] m_mirred: Allow mirred to block

[Victor Nogueira]

On 23/01/2024 15:17, David Ahern wrote:
> On 1/23/24 9:11 AM, Victor Nogueira wrote:
>> ---
>>   tc/m_mirred.c | 60 +++++++++++++++++++++++++++++++++++++++++----------
> 
> missing the man page update
> 
>> [...]
>> diff --git a/tc/m_mirred.c b/tc/m_mirred.c
>> index e5653e67f..db847b1a3 100644
>> --- a/tc/m_mirred.c
>> +++ b/tc/m_mirred.c
>> @@ -162,15 +167,38 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p,
>>   					TCA_INGRESS_REDIR;
>>   				p.action = TC_ACT_STOLEN;
>>   				ok++;
>> -			} else if ((redir || mirror) &&
>> -				   matches(*argv, "dev") == 0) {
>> -				NEXT_ARG();
>> -				if (strlen(d))
>> -					duparg("dev", *argv);
>> -
>> -				strncpy(d, *argv, sizeof(d)-1);
>> -				argc--;
>> -				argv++;
>> +			} else if ((redir || mirror)) {
>> +				if (matches(*argv, "blockid") == 0) {
> 
> Not accepting any more uses of matches.
> [...]   

Thank you for the catches.
Sent a v2 fixing these issues.

cheers,
Victor