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] [day] [month] [year] [list]

Message-ID: <6813e836-9f9d-0dd7-3ca0-f95afd8141c2@mellanox.com>
Date:   Thu, 4 Jan 2018 15:32:48 +0800
From:   Chris Mi <chrism@...lanox.com>
To:     David Ahern <dsahern@...il.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

  2018/1/3 12:25, David Ahern:
> You need a patch description here ...
Done.
>
> 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.
Done.
>
> 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.
Done. There is a little performance pernalty. But I think it is better 
than segfault.
>
> 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