From: Petr Machata <petrm@nvidia.com>
To: Ido Schimmel <idosch@nvidia.com>
Cc: <netdev@vger.kernel.org>, <stephen@networkplumber.org>,
<dsahern@gmail.com>, <gnault@redhat.com>, <petrm@nvidia.com>
Subject: Re: [PATCH iproute2-next 4/5] iprule: Add port mask support
Date: Mon, 24 Feb 2025 15:03:25 +0100 [thread overview]
Message-ID: <87o6yrju35.fsf@nvidia.com> (raw)
In-Reply-To: <20250224065241.236141-5-idosch@nvidia.com>
Ido Schimmel <idosch@nvidia.com> writes:
> Add port mask support, allowing users to specify a source or destination
> port with an optional mask. Example:
>
> # ip rule add sport 80 table 100
> # ip rule add sport 90/0xffff table 200
> # ip rule add dport 1000-2000 table 300
> # ip rule add sport 0x123/0xfff table 400
> # ip rule add dport 0x4/0xff table 500
> # ip rule add dport 0x8/0xf table 600
> # ip rule del dport 0x8/0xf table 600
>
> In non-JSON output, the mask is not printed in case of exact match:
>
> $ ip rule show
> 0: from all lookup local
> 32761: from all dport 0x4/0xff lookup 500
> 32762: from all sport 0x123/0xfff lookup 400
> 32763: from all dport 1000-2000 lookup 300
> 32764: from all sport 90 lookup 200
> 32765: from all sport 80 lookup 100
> 32766: from all lookup main
> 32767: from all lookup default
>
> Dump can be filtered by port value and mask:
>
> $ ip rule show sport 80
> 32765: from all sport 80 lookup 100
> $ ip rule show sport 90
> 32764: from all sport 90 lookup 200
> $ ip rule show sport 0x123/0x0fff
> 32762: from all sport 0x123/0xfff lookup 400
> $ ip rule show dport 4/0xff
> 32761: from all dport 0x4/0xff lookup 500
>
> In JSON output, the port mask is printed as an hexadecimal string to be
> consistent with other masks. The port value is printed as an integer in
> order not to break existing scripts:
>
> $ ip -j -p rule show sport 0x123/0xfff table 400
> [ {
> "priority": 32762,
> "src": "all",
> "sport": 291,
> "sport_mask": "0xfff",
> "table": "400"
> } ]
>
> The mask attribute is only sent to the kernel in case of inexact match
> so that iproute2 will continue working with kernels that do not support
> the attribute.
>
> Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Two minor suggestions and a couple notes to self below. Looks OK overall.
> ---
> ip/iprule.c | 103 +++++++++++++++++++++++++++++++++++++-----
> man/man8/ip-rule.8.in | 14 +++---
> 2 files changed, 100 insertions(+), 17 deletions(-)
>
> diff --git a/ip/iprule.c b/ip/iprule.c
> index 64d389bebb76..fbe69a3b6293 100644
> --- a/ip/iprule.c
> +++ b/ip/iprule.c
> @@ -23,6 +23,8 @@
> #include "ip_common.h"
> #include "json_print.h"
>
> +#define PORT_MAX_MASK 0xFFFF
> +
> enum list_action {
> IPRULE_LIST,
> IPRULE_FLUSH,
> @@ -44,8 +46,8 @@ static void usage(void)
> " [ iif STRING ] [ oif STRING ] [ pref NUMBER ] [ l3mdev ]\n"
> " [ uidrange NUMBER-NUMBER ]\n"
> " [ ipproto PROTOCOL ]\n"
> - " [ sport [ NUMBER | NUMBER-NUMBER ]\n"
> - " [ dport [ NUMBER | NUMBER-NUMBER ] ]\n"
> + " [ sport [ NUMBER[/MASK] | NUMBER-NUMBER ]\n"
> + " [ dport [ NUMBER[/MASK] | NUMBER-NUMBER ] ]\n"
> " [ dscp DSCP ] [ flowlabel FLOWLABEL[/MASK] ]\n"
> "ACTION := [ table TABLE_ID ]\n"
> " [ protocol PROTO ]\n"
> @@ -80,6 +82,7 @@ static struct
> int protocolmask;
> struct fib_rule_port_range sport;
> struct fib_rule_port_range dport;
> + __u16 sport_mask, dport_mask;
> __u8 ipproto;
> } filter;
>
> @@ -186,8 +189,9 @@ static bool filter_nlmsg(struct nlmsghdr *n, struct rtattr **tb, int host_len)
> return false;
> }
>
> - if (filter.sport.start) {
> + if (filter.sport_mask) {
OK, sport_mask now implies sport.start because of the changes in
iprule_port_parse().
> const struct fib_rule_port_range *r;
> + __u16 sport_mask = PORT_MAX_MASK;
>
> if (!tb[FRA_SPORT_RANGE])
> return false;
> @@ -196,10 +200,16 @@ static bool filter_nlmsg(struct nlmsghdr *n, struct rtattr **tb, int host_len)
> if (r->start != filter.sport.start ||
> r->end != filter.sport.end)
> return false;
> +
> + if (tb[FRA_SPORT_MASK])
> + sport_mask = rta_getattr_u16(tb[FRA_SPORT_MASK]);
> + if (filter.sport_mask != sport_mask)
> + return false;
> }
>
> - if (filter.dport.start) {
> + if (filter.dport_mask) {
> const struct fib_rule_port_range *r;
> + __u16 dport_mask = PORT_MAX_MASK;
>
> if (!tb[FRA_DPORT_RANGE])
> return false;
> @@ -208,6 +218,11 @@ static bool filter_nlmsg(struct nlmsghdr *n, struct rtattr **tb, int host_len)
> if (r->start != filter.dport.start ||
> r->end != filter.dport.end)
> return false;
> +
> + if (tb[FRA_DPORT_MASK])
> + dport_mask = rta_getattr_u16(tb[FRA_DPORT_MASK]);
> + if (filter.dport_mask != dport_mask)
> + return false;
> }
>
> if (filter.tun_id) {
> @@ -390,7 +405,26 @@ int print_rule(struct nlmsghdr *n, void *arg)
> struct fib_rule_port_range *r = RTA_DATA(tb[FRA_SPORT_RANGE]);
>
> if (r->start == r->end) {
> - print_uint(PRINT_ANY, "sport", " sport %u", r->start);
> + if (tb[FRA_SPORT_MASK]) {
> + __u16 mask;
> +
> + mask = rta_getattr_u16(tb[FRA_SPORT_MASK]);
> + print_uint(PRINT_JSON, "sport", NULL, r->start);
> + print_0xhex(PRINT_JSON, "sport_mask", NULL,
> + mask);
> + if (mask == PORT_MAX_MASK) {
> + print_uint(PRINT_FP, NULL, " sport %u",
> + r->start);
> + } else {
> + print_0xhex(PRINT_FP, NULL,
> + " sport %#x", r->start);
Looks good, for JSON we always emit as uint, for FP we emit uint in
backward-compatible scenarios.
> + print_0xhex(PRINT_FP, NULL, "/%#x",
> + mask);
> + }
> + } else {
> + print_uint(PRINT_ANY, "sport", " sport %u",
> + r->start);
Hm, yeah, and on an old kernel we don't even get the mask in JSON.
Makes sense.
> + }
> } else {
> print_uint(PRINT_ANY, "sport_start", " sport %u",
> r->start);
> @@ -402,7 +436,26 @@ int print_rule(struct nlmsghdr *n, void *arg)
> struct fib_rule_port_range *r = RTA_DATA(tb[FRA_DPORT_RANGE]);
>
> if (r->start == r->end) {
> - print_uint(PRINT_ANY, "dport", " dport %u", r->start);
> + if (tb[FRA_DPORT_MASK]) {
> + __u16 mask;
> +
> + mask = rta_getattr_u16(tb[FRA_DPORT_MASK]);
> + print_uint(PRINT_JSON, "dport", NULL, r->start);
> + print_0xhex(PRINT_JSON, "dport_mask", NULL,
> + mask);
> + if (mask == 0xFFFF) {
> + print_uint(PRINT_FP, NULL, " dport %u",
> + r->start);
> + } else {
> + print_0xhex(PRINT_FP, NULL,
> + " dport %#x", r->start);
> + print_0xhex(PRINT_FP, NULL, "/%#x",
> + mask);
> + }
> + } else {
> + print_uint(PRINT_ANY, "dport", " dport %u",
> + r->start);
> + }
> } else {
> print_uint(PRINT_ANY, "dport_start", " dport %u",
> r->start);
> @@ -600,10 +653,13 @@ static int flush_rule(struct nlmsghdr *n, void *arg)
> return 0;
> }
>
> -static void iprule_port_parse(char *arg, struct fib_rule_port_range *r)
> +static void iprule_port_parse(char *arg, struct fib_rule_port_range *r,
> + __u16 *mask)
> {
> char *sep;
>
> + *mask = PORT_MAX_MASK;
> +
> sep = strchr(arg, '-');
> if (sep) {
> *sep = '\0';
> @@ -617,6 +673,21 @@ static void iprule_port_parse(char *arg, struct fib_rule_port_range *r)
> return;
> }
>
> + sep = strchr(arg, '/');
> + if (sep) {
> + *sep = '\0';
> +
> + if (get_u16(&r->start, arg, 0))
> + invarg("invalid port", arg);
> +
> + r->end = r->start;
> +
> + if (get_u16(mask, sep + 1, 0))
> + invarg("invalid mask", sep + 1);
> +
> + return;
> + }
> +
> if (get_u16(&r->start, arg, 0))
> invarg("invalid port", arg);
>
I think this duplicates the port number parsing unnecessarily. How
about:
+ sep = strchr(arg, '/');
+ if (sep) {
+ *sep = '\0';
+ if (get_u16(mask, sep + 1, 0))
+ invarg("invalid mask", sep + 1);
+ }
+
if (get_u16(&r->start, arg, 0))
invarg("invalid port", arg);
> @@ -770,10 +841,12 @@ static int iprule_list_flush_or_save(int argc, char **argv, int action)
> filter.ipproto = ipproto;
> } else if (strcmp(*argv, "sport") == 0) {
> NEXT_ARG();
> - iprule_port_parse(*argv, &filter.sport);
> + iprule_port_parse(*argv, &filter.sport,
> + &filter.sport_mask);
> } else if (strcmp(*argv, "dport") == 0) {
> NEXT_ARG();
> - iprule_port_parse(*argv, &filter.dport);
> + iprule_port_parse(*argv, &filter.dport,
> + &filter.dport_mask);
> } else if (strcmp(*argv, "dscp") == 0) {
> __u32 dscp;
>
> @@ -1043,18 +1116,26 @@ static int iprule_modify(int cmd, int argc, char **argv)
> addattr8(&req.n, sizeof(req), FRA_IP_PROTO, ipproto);
> } else if (strcmp(*argv, "sport") == 0) {
> struct fib_rule_port_range r;
> + __u16 sport_mask;
>
> NEXT_ARG();
> - iprule_port_parse(*argv, &r);
> + iprule_port_parse(*argv, &r, &sport_mask);
> addattr_l(&req.n, sizeof(req), FRA_SPORT_RANGE, &r,
> sizeof(r));
> + if (sport_mask != PORT_MAX_MASK)
> + addattr16(&req.n, sizeof(req), FRA_SPORT_MASK,
> + sport_mask);
> } else if (strcmp(*argv, "dport") == 0) {
> struct fib_rule_port_range r;
> + __u16 dport_mask;
>
> NEXT_ARG();
> - iprule_port_parse(*argv, &r);
> + iprule_port_parse(*argv, &r, &dport_mask);
> addattr_l(&req.n, sizeof(req), FRA_DPORT_RANGE, &r,
> sizeof(r));
> + if (dport_mask != PORT_MAX_MASK)
> + addattr16(&req.n, sizeof(req), FRA_DPORT_MASK,
> + dport_mask);
> } else if (strcmp(*argv, "dscp") == 0) {
> __u32 dscp;
>
> diff --git a/man/man8/ip-rule.8.in b/man/man8/ip-rule.8.in
> index 6fc741d4f470..4945ccd55076 100644
> --- a/man/man8/ip-rule.8.in
> +++ b/man/man8/ip-rule.8.in
> @@ -52,10 +52,10 @@ ip-rule \- routing policy database management
> .B ipproto
> .IR PROTOCOL " ] [ "
> .BR sport " [ "
> -.IR NUMBER " | "
> +.IR NUMBER\fR[\fB/\fIMASK "] | "
> .IR NUMBER "-" NUMBER " ] ] [ "
> .BR dport " [ "
> -.IR NUMBER " | "
> +.IR NUMBER\fR[\fB/\fIMASK "] | "
> .IR NUMBER "-" NUMBER " ] ] [ "
> .B tun_id
> .IR TUN_ID " ] [ "
> @@ -270,12 +270,14 @@ value to match.
> select the ip protocol value to match.
>
> .TP
> -.BI sport " NUMBER | NUMBER-NUMBER"
> -select the source port value to match. supports port range.
> +.BI sport " NUMBER\fR[\fB/\fIMASK\fR] | NUMBER-NUMBER"
> +select the source port value to match with an optional mask. supports port
> +range.
s/supports/Supports/. And below.
>
> .TP
> -.BI dport " NUMBER | NUMBER-NUMBER"
> -select the destination port value to match. supports port range.
> +.BI dport " NUMBER\fR[\fB/\fIMASK\fR] | NUMBER-NUMBER"
> +select the destination port value to match with an optional mask. supports port
> +range.
>
> .TP
> .BI priority " PREFERENCE"
next prev parent reply other threads:[~2025-02-24 14:28 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-02-24 6:52 [PATCH iproute2-next 0/5] iprule: Add mask support for L4 ports and DSCP Ido Schimmel
2025-02-24 6:52 ` [PATCH iproute2-next 1/5] Sync uAPI headers Ido Schimmel
2025-02-24 6:52 ` [PATCH iproute2-next 2/5] iprule: Move port parsing to a function Ido Schimmel
2025-02-24 14:47 ` Petr Machata
2025-02-24 6:52 ` [PATCH iproute2-next 3/5] iprule: Allow specifying ports in hexadecimal notation Ido Schimmel
2025-02-24 14:47 ` Petr Machata
2025-02-24 6:52 ` [PATCH iproute2-next 4/5] iprule: Add port mask support Ido Schimmel
2025-02-24 14:03 ` Petr Machata [this message]
2025-02-24 16:17 ` Ido Schimmel
2025-02-24 6:52 ` [PATCH iproute2-next 5/5] iprule: Add DSCP " Ido Schimmel
2025-02-24 14:35 ` Petr Machata
2025-02-24 16:27 ` Ido Schimmel
2025-02-25 8:58 ` Petr Machata
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87o6yrju35.fsf@nvidia.com \
--to=petrm@nvidia.com \
--cc=dsahern@gmail.com \
--cc=gnault@redhat.com \
--cc=idosch@nvidia.com \
--cc=netdev@vger.kernel.org \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.