[PATCH 3/3] [NET_DOC] Document some simple rules for actions

[PATCH 3/3] [NET_DOC] Document some simple rules for actions

[jamal]

[-- Attachment #1: Type: text/plain, Size: 31 bytes --]

Against net-2.6

cheers,
jamal

[-- Attachment #2: 0003-NET_DOC-Document-some-simple-rules-for-actions.txt --]
[-- Type: application/mbox, Size: 2093 bytes --]

Re: [PATCH 3/3] [NET_DOC] Document some simple rules for actions

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Sun, 21 Oct 2007 15:22:17 -0400

> Against net-2.6

Applied.

[PATCH 3/3] [NET_DOC] Document some simple rules for actions

[jamal]

[-- Attachment #1: Type: text/plain, Size: 15 bytes --]

cheers,
jamal


[-- Attachment #2: 0003-NET_DOC-Document-some-simple-rules-for-actions --]
[-- Type: text/plain, Size: 1966 bytes --]

[NET_DOC] Document some simple rules for actions

This patch adds documentation on what is expected of an
action which branches away from the action-graph or when it
needs to trample on actins. It also describes what is expected of
users of such actions.

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
---
 Documentation/networking/tc-actions-env-rules.txt |   29 +++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/networking/tc-actions-env-rules.txt b/Documentation/networking/tc-actions-env-rules.txt
new file mode 100644
index 0000000..c684f48
--- /dev/null
+++ b/Documentation/networking/tc-actions-env-rules.txt
@@ -0,0 +1,29 @@
+
+The "enviromental" rules for authors of any new tc actions are:   
+
+1) If you stealeth or borroweth any packet thou shalt be branching 
+from the righteous path and thou shalt cloneth. 
+
+For example if your action queues a packet to be processed later
+or intentionaly branches by redirecting a packet then you need to 
+clone the packet. 
+There are certain fields in the skb tc_verd that need to be reset so we
+avoid loops etc. A few are generic enough so much so that skb_act_clone() 
+resets them for you. So invoke skb_act_clone() rather than skb_clone()
+
+2) If you munge any packet thou shalt call skb_expand in the case
+someone else is referencing the skb. After that you "own" the skb. 
+You must also tell us if it is ok to munge the packet (TC_OK2MUNGE), 
+this way any action downstream can stomp on the packet.
+
+3) dropping packets you dont own is a nono. You simply return
+TC_ACT_SHOT to the caller and they will drop it.
+
+The "enviromental" rules for callers of actions (qdiscs etc) are:
+
+*) thou art responsible for freeing anything returned as being
+TC_ACT_SHOT/STOLEN/QUEUED. If none of TC_ACT_SHOT/STOLEN/QUEUED is
+returned then all is great and you dont need to do anything.
+
+Post on netdev if something is unclear.
+
-- 
1.4.4.1.gaed4

Re: [PATCH 3/3] [NET_DOC] Document some simple rules for actions

[Herbert Xu]

On Tue, Oct 16, 2007 at 08:33:31AM -0400, jamal wrote:
>
> +2) If you munge any packet thou shalt call skb_expand in the case
> +someone else is referencing the skb. After that you "own" the skb. 
> +You must also tell us if it is ok to munge the packet (TC_OK2MUNGE), 
> +this way any action downstream can stomp on the packet.

Thanks for the documentation!

But I still think the distinction between skb_clone and
skb_copy_header is wrong.

When you munge a packet, it's still going to go back up
the stack and be processed along the same path.  Therefore
you should be calling pskb_expand_head.

In other words, this does not explain why skb_copy/pskb_copy
and skb_copy_expand should differ from skb_clone.  As far as
I can see all these functions should behave in the same manner
with respect to tc_verd.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 3/3] [NET_DOC] Document some simple rules for actions

[jamal]

On Tue, 2007-16-10 at 20:51 +0800, Herbert Xu wrote:

> But I still think the distinction between skb_clone and
> skb_copy_header is wrong.
> 
> When you munge a packet, it's still going to go back up
> the stack and be processed along the same path.  Therefore
> you should be calling pskb_expand_head.

Dang, just noticed i said skb_expand() in the doc - is that the 
issue? I will resend that one patch.
We do call pskb_expand_head() - example look at act_pedit.c

> In other words, this does not explain why skb_copy/pskb_copy
> and skb_copy_expand should differ from skb_clone.  As far as
> I can see all these functions should behave in the same manner
> with respect to tc_verd


We need to avoid actions from infinetely looping (eg when they ask for
being reclassified) in a directed graph. That loop ttl count  must be
reset to zero if we branch, because that is a new directed path. This is
always true and therefore that field must always be reset. 
I have a feeling i am going on a tangent if that doesnt respond to your
comment above.

cheers,
jamal