netdev - Re: [patch iproute2 v5 2/3] tc: Add -bs option to batch mode

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9326994a-de20-1eb6-71bc-fb757ad05872@gmail.com>
Date:   Tue, 2 Jan 2018 21:25:11 -0700
From:   David Ahern <dsahern@...il.com>
To:     Chris Mi <chrism@...lanox.com>, netdev@...r.kernel.org
Cc:     gerlitz.or@...il.com, stephen@...workplumber.org,
        marcelo.leitner@...il.com
Subject: Re: [patch iproute2 v5 2/3] tc: Add -bs option to batch mode

You need a patch description here ...

On 1/2/18 7:55 PM, Chris Mi wrote:
>  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];

Reverse xmas tree is the coding standard for net code. Please check all
new code to conform to this standard.

I have not reviewed all of this patch, but I firmly believe the batching
size option needs to be able handle a file with mixed commands. Your use
case is filter and action adds and deletes, but you should allow users
(e.g., test suites) to benefit from this performance speed up with test
cases that have single files with all of the commands.

For example,
$ cat tc.batch
qdisc add dev eth2 ingress
filter add dev eth2 ingress protocol ip pref 21 flower dst_ip
192.168.1.0/16 action drop
filter add dev eth2 ingress protocol ip pref 22 flower dst_ip
192.168.2.0/16 action drop
filter add dev eth2 ingress protocol ip pref 23 flower dst_ip
192.168.3.0/16 action drop
filter add dev eth2 ingress protocol ip pref 24 flower dst_ip
192.168.4.0/16 action drop
filter add dev eth2 ingress protocol ip pref 25 flower dst_ip
192.168.5.0/16 action drop
qdisc del dev eth2 ingress

(and consider this to be a huge file to really stress tc code paths for
example). Right now, the above file fails:

$ tc -b tc.batch -bs 5
Segmentation fault


Also, your changes fail to break out on an error:

$ tc -b tc.batch -bs 1
RTNETLINK answers: File exists
We have an error talking to the kernel, -1
RTNETLINK answers: File exists
We have an error talking to the kernel, -1
RTNETLINK answers: File exists
We have an error talking to the kernel, -1
RTNETLINK answers: File exists
We have an error talking to the kernel, -1
RTNETLINK answers: File exists
We have an error talking to the kernel, -1

where as the existing command does this:
$ tc -b tc.batch
RTNETLINK answers: File exists
We have an error talking to the kernel
Command failed tc.batch:1