| 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: Wed, 27 Dec 2017 09:39:15 -0600
From: David Ahern <dsahern@...il.com>
To: Chris Mi <chrism@...lanox.com>, netdev@...r.kernel.org
Cc: gerlitz.or@...il.com, stephen@...workplumber.org
Subject: Re: [patch iproute2 v3 3/4] tc: Add -bs option to batch mode
On 12/25/17 2:46 AM, Chris Mi wrote:
> Signed-off-by: Chris Mi <chrism@...lanox.com>
> ---
> tc/m_action.c | 91 +++++++++++++++++++++++++++++++++----------
> tc/tc.c | 47 ++++++++++++++++++----
> tc/tc_common.h | 8 +++-
> tc/tc_filter.c | 121 +++++++++++++++++++++++++++++++++++++++++----------------
> 4 files changed, 204 insertions(+), 63 deletions(-)
>
> diff --git a/tc/m_action.c b/tc/m_action.c
> index fc422364..c4c3b862 100644
> --- a/tc/m_action.c
> +++ b/tc/m_action.c
> @@ -23,6 +23,7 @@
> #include <arpa/inet.h>
> #include <string.h>
> #include <dlfcn.h>
> +#include <errno.h>
>
> #include "utils.h"
> #include "tc_common.h"
> @@ -546,40 +547,88 @@ bad_val:
> return ret;
> }
>
> +typedef struct {
> + struct nlmsghdr n;
> + struct tcamsg t;
> + char buf[MAX_MSG];
> +} tc_action_req;
> +
> +static tc_action_req *action_reqs;
> +static struct iovec msg_iov[MSG_IOV_MAX];
> +
> +void free_action_reqs(void)
> +{
> + free(action_reqs);
> +}
> +
> +static tc_action_req *get_action_req(int batch_size, int index)
> +{
> + tc_action_req *req;
> +
> + if (action_reqs == NULL) {
> + action_reqs = malloc(batch_size * sizeof (tc_action_req));
> + if (action_reqs == NULL)
> + return NULL;
> + }
> + req = &action_reqs[index];
> + memset(req, 0, sizeof (*req));
> +
> + return req;
> +}
> +
> static int tc_action_modify(int cmd, unsigned int flags,
> - int *argc_p, char ***argv_p)
> + int *argc_p, char ***argv_p,
> + int batch_size, int index, bool send)
> {
> int argc = *argc_p;
> char **argv = *argv_p;
> int ret = 0;
> - struct {
> - struct nlmsghdr n;
> - struct tcamsg t;
> - char buf[MAX_MSG];
> - } req = {
> - .n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg)),
> - .n.nlmsg_flags = NLM_F_REQUEST | flags,
> - .n.nlmsg_type = cmd,
> - .t.tca_family = AF_UNSPEC,
> + tc_action_req *req;
> + struct sockaddr_nl nladdr = { .nl_family = AF_NETLINK };
> + struct iovec *iov = &msg_iov[index];
> +
> + req = get_action_req(batch_size, index);
> + if (req == NULL) {
> + fprintf(stderr, "get_action_req error: not enough buffer\n");
> + return -ENOMEM;
> + }
> +
> + req->n.nlmsg_len = NLMSG_LENGTH(sizeof(struct tcamsg));
> + req->n.nlmsg_flags = NLM_F_REQUEST | flags;
> + req->n.nlmsg_type = cmd;
> + req->t.tca_family = AF_UNSPEC;
> + struct rtattr *tail = NLMSG_TAIL(&req->n);
> +
> + struct msghdr msg = {
> + .msg_name = &nladdr,
> + .msg_namelen = sizeof(nladdr),
> + .msg_iov = msg_iov,
> + .msg_iovlen = index + 1,
> };
> - struct rtattr *tail = NLMSG_TAIL(&req.n);
>
> argc -= 1;
> argv += 1;
> - if (parse_action(&argc, &argv, TCA_ACT_TAB, &req.n)) {
> + if (parse_action(&argc, &argv, TCA_ACT_TAB, &req->n)) {
> fprintf(stderr, "Illegal \"action\"\n");
> return -1;
> }
> - tail->rta_len = (void *) NLMSG_TAIL(&req.n) - (void *) tail;
> + tail->rta_len = (void *) NLMSG_TAIL(&req->n) - (void *) tail;
> +
> + *argc_p = argc;
> + *argv_p = argv;
> +
> + iov->iov_base = &req->n;
> + iov->iov_len = req->n.nlmsg_len;
> +
> + if (!send)
> + return 0;
>
> - if (rtnl_talk(&rth, &req.n, NULL) < 0) {
> + ret = rtnl_talk_msg(&rth, &msg, NULL);
> + if (ret < 0) {
> fprintf(stderr, "We have an error talking to the kernel\n");
> ret = -1;
> }
>
> - *argc_p = argc;
> - *argv_p = argv;
> -
> return ret;
> }
>
> @@ -679,7 +728,7 @@ bad_val:
> return ret;
> }
>
> -int do_action(int argc, char **argv)
> +int do_action(int argc, char **argv, int batch_size, int index, bool send)
> {
>
> int ret = 0;
> @@ -689,12 +738,14 @@ int do_action(int argc, char **argv)
> if (matches(*argv, "add") == 0) {
> ret = tc_action_modify(RTM_NEWACTION,
> NLM_F_EXCL | NLM_F_CREATE,
> - &argc, &argv);
> + &argc, &argv, batch_size,
> + index, send);
> } else if (matches(*argv, "change") == 0 ||
> matches(*argv, "replace") == 0) {
> ret = tc_action_modify(RTM_NEWACTION,
> NLM_F_CREATE | NLM_F_REPLACE,
> - &argc, &argv);
> + &argc, &argv, batch_size,
> + index, send);
> } else if (matches(*argv, "delete") == 0) {
> argc -= 1;
> argv += 1;
> diff --git a/tc/tc.c b/tc/tc.c
> index ad9f07e9..7ea2fc89 100644
> --- a/tc/tc.c
> +++ b/tc/tc.c
> @@ -189,20 +189,20 @@ static void usage(void)
> fprintf(stderr, "Usage: tc [ OPTIONS ] OBJECT { COMMAND | help }\n"
> " tc [-force] -batch filename\n"
> "where OBJECT := { qdisc | class | filter | action | monitor | exec }\n"
> - " OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] | -b[atch] [filename] | -n[etns] name |\n"
> + " OPTIONS := { -s[tatistics] | -d[etails] | -r[aw] | -p[retty] | -b[atch] [filename] | -bs | -batchsize [size] | -n[etns] name |\n"
> " -nm | -nam[es] | { -cf | -conf } path } | -j[son]\n");
> }
>
> -static int do_cmd(int argc, char **argv)
> +static int do_cmd(int argc, char **argv, int batch_size, int index, bool send)
> {
> if (matches(*argv, "qdisc") == 0)
> return do_qdisc(argc-1, argv+1);
> if (matches(*argv, "class") == 0)
> return do_class(argc-1, argv+1);
> if (matches(*argv, "filter") == 0)
> - return do_filter(argc-1, argv+1);
> + return do_filter(argc-1, argv+1, batch_size, index, send);
> if (matches(*argv, "actions") == 0)
> - return do_action(argc-1, argv+1);
> + return do_action(argc-1, argv+1, batch_size, index, send);
> if (matches(*argv, "monitor") == 0)
> return do_tcmonitor(argc-1, argv+1);
> if (matches(*argv, "exec") == 0)
> @@ -217,11 +217,16 @@ static int do_cmd(int argc, char **argv)
> return -1;
> }
>
> -static int batch(const char *name)
> +static int batch(const char *name, int batch_size)
> {
> + int msg_iov_index = 0;
> char *line = NULL;
> size_t len = 0;
> int ret = 0;
> + bool send;
> +
> + if (batch_size > 1)
> + setcmdlinetotal(name);
>
> batch_mode = 1;
> if (name && strcmp(name, "-") != 0) {
> @@ -248,15 +253,30 @@ static int batch(const char *name)
> if (largc == 0)
> continue; /* blank line */
>
> - if (do_cmd(largc, largv)) {
> + /*
> + * In batch mode, if we haven't accumulated enough commands
> + * and this is not the last command, don't send the message
> + * immediately.
> + */
> + if (batch_size > 1 && msg_iov_index + 1 != batch_size
> + && cmdlineno != cmdlinetotal)
> + send = false;
> + else
> + send = true;
> +
> + ret = do_cmd(largc, largv, batch_size, msg_iov_index++, send);
What happens if tc commands are interlaced in the file -- qdisc add,
class add, filter add, then a delete, show, exec, etc.? Right now each
command is handled one at a time so an add followed by a delete will
work. Your proposed batching loop won't work for this case as some
commands are executed when that line is reached and others are batched
for later send. Not all of the tc commands need to be batched in a
single message so perhaps those commands cause the queue to be flushed
(ie, message sent), then that command is executed and you start the
batching over.
Further, I really think the batching can be done without the global
variables and without the command handlers knowing it is batching or
part of an iov. e.g., in the case of batching try having the commands
malloc the request buffer and return the pointer back to this loop in
which case this loop calls rtnl_talk_msg and frees the buffers.
> + if (ret < 0) {
> fprintf(stderr, "Command failed %s:%d\n", name, cmdlineno);
> ret = 1;
> if (!force)
> break;
> }
> + msg_iov_index %= batch_size;
> }
> if (line)
> free(line);
> + free_filter_reqs();
> + free_action_reqs();
>
> rtnl_close(&rth);
> return ret;
Powered by blists - more mailing lists