netdev - Re: [Patch net] sched, cls: check if we could overwrite actions when changing a filter

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]

Date:	Fri, 18 Apr 2014 10:26:33 -0400
From:	Jamal Hadi Salim <jhs@...atatu.com>
To:	Cong Wang <xiyou.wangcong@...il.com>
CC:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Cong Wang <cwang@...pensource.com>
Subject: Re: [Patch net] sched, cls: check if we could overwrite actions when
 changing a filter

On 04/17/14 16:48, Cong Wang wrote:

> Actions attached to a filter are at the end of the graph, they should
> be able to be added/removed together.

I will try to be clear: you are changing policy which is a higher
level semantic than any one of filter or actions. Consider:

a) A simple policy in a linear graph
if (icmp) --> A-->B-->C-->D-->E

With the following individual change operations:
i)change to delete C
ii)change to add G after B
iii) add F as branch from D
iv) add H after E and maintain linear status
v) after #iv make the graph at node D selectively branch between E/H

Ok, let provide a slightly complex graph, sorry it is hard to do ascii
diagram for this, so i will illustrate programmatically:

if (icmp) {
     mark with tag 11 //mark index 7
     if (rate < 10kbs) { //policer index 1
        return
     } else {
        mark with tag 12  //mark index 3
        if (rate2 < 20kbs) {//policer index 2
           return
       } else {
           mirror to dummy0 //mirred index 8
       }
    }
}

What does adding/deleting even mean in this case?

>
> This is a workaround, not a fix. What if I have multiple threads
> trying to append an action at the same time? This workaround
> can't guarantee the correctness.
>

Sorry Cong, your approach is NOT a fix. There is _no way_ you can
achieve correctness without a two phase operation. Talk to the
netfilter guys about such experiences (in 2.x kernels?).
The multiple threads issue is taken care of by locking,
you just want to reduce the amount of lock time needed; that is
what that (proven) 2 phase approach is doing.
I called it workaround because you dont need to make any changes
in the kernel. There is a tiny window of inconsistency possible in
presence of multiple contenders but you already get that with things
like RCU (grace period) that you added.

> My case is a perfectly valid use,

I am not questioning your use case - just it shouldnt come as a hack at
the expense of all other use cases (of which the most important
are not replacing/changing a graph but creating and deleting one).

>we have to fix it, maybe in another way.

If you really really really insist i has to be done in the kernel;
then i would hope youd do it in some form of pseudo transacational
approach as i described is done today in user space.
I hope avoidance of a fat approach like 2PC as well is in order.
Again as i said I am extremely leary of optimizing for such
corner case since you can achieve all that in user space.

cheers,
jamal
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html