| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 27 Sep 2021 17:44:37 -0500
From: Alex Elder <elder@...aro.org>
To: Bjorn Andersson <bjorn.andersson@...aro.org>,
Daniele Palmas <dnlplm@...il.com>
Cc: netdev@...r.kernel.org
Subject: Re: [PATCH v2] iplink_rmnet: Allow passing IFLA_RMNET_FLAGS
On 9/23/21 10:33 PM, Bjorn Andersson wrote:
> Extend the rmnet option parser to allow enabling and disabling
> IFLA_RMNET_FLAGS using ip link and add the flags to the pint_op to allow
> inspecting the current settings.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
I think what you're doing looks OK to me, but I know
David had suggestions so I expect I'll get another shot
at review...
I have a couple very minor comments, and they're basically
unrelated. So I guess take it or leave it.
-Alex
> ---
>
> Changes since v1:
> - Landed ABI change to allow setting/clearing individual bits
> - Changed parser to take on/off arguments
> - Added the new v5 chksum bits
> - Made print_flags fancier, with some inspiration from iplink_vlan
>
> ip/iplink_rmnet.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 97 insertions(+)
>
> diff --git a/ip/iplink_rmnet.c b/ip/iplink_rmnet.c
> index 1d16440c6900..f629ca9976d9 100644
> --- a/ip/iplink_rmnet.c
> +++ b/ip/iplink_rmnet.c
> @@ -16,6 +16,12 @@ static void print_explain(FILE *f)
> {
> fprintf(f,
> "Usage: ... rmnet mux_id MUXID\n"
> + " [ingress-deaggregation { on | off } ]\n"
> + " [ingress-commands { on | off } ]\n"
> + " [ingress-chksumv4 { on | off } ]\n"
> + " [ingress-chksumv5 { on | off } ]\n"
> + " [egress-chksumv4 { on | off } ]\n"
> + " [egress-chksumv5 { on | off } ]\n"
Looking at other usage messages, it appears that the dash
rather than underscore is preferable (though not universal).
I.e., I think you did this right.
> "\n"
> "MUXID := 1-254\n"
> );
> @@ -26,9 +32,16 @@ static void explain(void)
> print_explain(stderr);
> }
>
> +static int on_off(const char *msg, const char *arg)
> +{
> + fprintf(stderr, "Error: argument of \"%s\" must be \"on\" or \"off\", not \"%s\"\n", msg, arg);
It would be nice if this function could be defined centrally rather
than repeated. Maybe defined in "lib/utils.c", declared in
"include/utils.h"? Not your problem but if you were so moved,
that could be done in a separate patch.
> + return -1;
> +}
> +
> static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
> struct nlmsghdr *n)
> {
> + struct ifla_rmnet_flags flags = { };
> __u16 mux_id;
>
> while (argc > 0) {
> @@ -37,6 +50,60 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
> if (get_u16(&mux_id, *argv, 0))
> invarg("mux_id is invalid", *argv);
> addattr16(n, 1024, IFLA_RMNET_MUX_ID, mux_id);
> + } else if (matches(*argv, "ingress-deaggregation") == 0) {
> + NEXT_ARG();
With these repeated blocks of code my instinct is to suggest
creating a common called function, but it gets a little ugly.
What I sketch below precludes using NEXT_ARG() where it's
needed.
static int matches_on_off(ifla_rmnet_flags *flags, u32 mask,
const char *name, char *arg)
{
if (matches(arg, name))
return 0;
if (strcmp(arg, "on") == 0)
flags->flag |= mask;
else if (strcmp(arg, "off") == 0) {
flags->flag &= ~mask;
else
return on_off(name, arg);
flags->mask |= mask;
return 1;
}
Then use:
if (matches_on_off(&flags, RMNET_FLAGS_INGRESS_DEAGGREGATION,
"ingress-deaggregation", *argv) < 0)
return -1;
else if (matches_on_off(...))
. . .
So I'm not sure I like how this looks, and once again,
it isn't really necessary for what you're doing here...
> + flags.mask |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
> + if (strcmp(*argv, "on") == 0)
> + flags.flags |= RMNET_FLAGS_INGRESS_DEAGGREGATION;
> + else if (strcmp(*argv, "off") == 0)
> + flags.flags &= ~RMNET_FLAGS_INGRESS_DEAGGREGATION;
> + else
> + return on_off("ingress-deaggregation", *argv);
> + } else if (matches(*argv, "ingress-commands") == 0) {
> + NEXT_ARG();
> + flags.mask |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> + if (strcmp(*argv, "on") == 0)
> + flags.flags |= RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> + else if (strcmp(*argv, "off") == 0)
> + flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_COMMANDS;
> + else
> + return on_off("ingress-commands", *argv);
> + } else if (matches(*argv, "ingress-chksumv4") == 0) {
> + NEXT_ARG();
> + flags.mask |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> + if (strcmp(*argv, "on") == 0)
> + flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> + else if (strcmp(*argv, "off") == 0)
> + flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_CKSUMV4;
> + else
> + return on_off("ingress-chksumv4", *argv);
> + } else if (matches(*argv, "ingress-chksumv5") == 0) {
> + NEXT_ARG();
> + flags.mask |= RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
> + if (strcmp(*argv, "on") == 0)
> + flags.flags |= RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
> + else if (strcmp(*argv, "off") == 0)
> + flags.flags &= ~RMNET_FLAGS_INGRESS_MAP_CKSUMV5;
> + else
> + return on_off("ingress-chksumv5", *argv);
> + } else if (matches(*argv, "egress-chksumv4") == 0) {
> + NEXT_ARG();
> + flags.mask |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
> + if (strcmp(*argv, "on") == 0)
> + flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
> + else if (strcmp(*argv, "off") == 0)
> + flags.flags &= ~RMNET_FLAGS_EGRESS_MAP_CKSUMV4;
> + else
> + return on_off("egress-chksumv4", *argv);
> + } else if (matches(*argv, "egress-chksumv5") == 0) {
> + NEXT_ARG();
> + flags.mask |= RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
> + if (strcmp(*argv, "on") == 0)
> + flags.flags |= RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
> + else if (strcmp(*argv, "off") == 0)
> + flags.flags &= ~RMNET_FLAGS_EGRESS_MAP_CKSUMV5;
> + else
> + return on_off("egress-chksumv5", *argv);
> } else if (matches(*argv, "help") == 0) {
> explain();
> return -1;
> @@ -48,11 +115,33 @@ static int rmnet_parse_opt(struct link_util *lu, int argc, char **argv,
> argc--, argv++;
> }
>
> + if (flags.mask)
> + addattr_l(n, 1024, IFLA_RMNET_FLAGS, &flags, sizeof(flags));
> +
> return 0;
> }
>
> +static void rmnet_print_flags(FILE *fp, __u32 flags)
> +{
> + open_json_array(PRINT_ANY, is_json_context() ? "flags" : "<");
> +#define _PF(f, s) if (flags & RMNET_FLAGS_##f) { \
> + flags &= ~RMNET_FLAGS_##f; \
> + print_string(PRINT_ANY, NULL, flags ? "%s," : "%s", s); \
> + }
> + _PF(INGRESS_DEAGGREGATION, "ingress-deaggregation");
> + _PF(INGRESS_MAP_COMMANDS, "ingress-commands");
> + _PF(INGRESS_MAP_CKSUMV4, "ingress-chksumv4");
> + _PF(INGRESS_MAP_CKSUMV5, "ingress-chksumv5");
> + _PF(EGRESS_MAP_CKSUMV4, "egress-chksumv4");
> + _PF(EGRESS_MAP_CKSUMV5, "egress-chksumv5");
> +#undef _PF
> + close_json_array(PRINT_ANY, "> ");
> +}
> +
> static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
> {
> + struct ifla_vlan_flags *flags;
> +
> if (!tb)
> return;
>
> @@ -64,6 +153,14 @@ static void rmnet_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
> "mux_id",
> "mux_id %u ",
> rta_getattr_u16(tb[IFLA_RMNET_MUX_ID]));
> +
> + if (tb[IFLA_RMNET_FLAGS]) {
> + if (RTA_PAYLOAD(tb[IFLA_RMNET_FLAGS]) < sizeof(*flags))
> + return;
> + flags = RTA_DATA(tb[IFLA_RMNET_FLAGS]);
> +
> + rmnet_print_flags(f, flags->flags);
> + }
> }
>
> static void rmnet_print_help(struct link_util *lu, int argc, char **argv,
>
Powered by blists - more mailing lists