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:	Sat, 19 Apr 2014 07:10:59 -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/18/14 13:18, Cong Wang wrote:

> In this case, all the statements inside if (icmp) {} are actions, right?
> Sorry, I still fail to see why not allowing to change them *together*?
>

Yes, thats what i am saying as well. Maybe we were agreeing all along.

> IOW, what's wrong with changing if (icmp) { A } to if (icmp) { B } ?
> where A and B could be any complex combination of actions.
> RTNL lock guarantees this is transactional.
>

RTNL is one dimension. The other is the datapath processing.
You need to make sure that packets still flow correctly during the
change over.
My suggestion was you add the new rule first with a lower priority
so that it is never used as long as the current one is in place.
You then do a delete of the old one. RCU grace period passes
where current packets are processed then the new rule takes effect.
You dont have to follow that suggestion as long as you achieve the
goal.

> Users are responsible to ensure the logic of A or B is correct, not
> the kernel. Kernel should allow even a wrong combination,
> since there is no way to check the correctness in kernel.
>

I'd be happy with that.

> I never mean to only add or remove one of them inside, although
> my specific case is just for appending, my patch should allow to
> overwrite all the actions together.
>

Well - then go nuts and put out a patch.
Replace _all or none_ is a reasonable approach.

> It is not a corner case, it is a very basic functionality we need:
>
> We mirror icmp packets to every vethX device, when one of them
> is gone, we just remove the action; when a new one comes up,
> we append an action. So simple...
>

This is where my problem was Cong - you have a simple use case and i was
hoping that you dont base that for a generic solution. If you only
have one action, then no problem in deleting/adding it. But if you
have a group of actions then you delete/add the whole group.

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