Re: Question about an assignment in handle_ing()

[jamal <hadi@cyberus.ca>]

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

On Wed, 2010-05-26 at 07:38 -0400, jamal wrote:
> On Wed, 2010-05-26 at 09:13 +1000, Herbert Xu wrote:
> 
> > If it did happen like you said then it would be a serious bug
> > in our stack as everything else (including the TCP stack) relies
> > on this.
> 
> It could have been a bug. Note this was not a simple test, so there
> may be other factors involved. If you or Jiri are willing to run the
> test i will construct a scenario which will test this out. It will need
> a compile of the kernel and a small check in pedit to see if we see
> cloned skbs when we run the two tcpdumps (and to make sure the tcpdumps
> see the correct bytes). Otherwise i will get to it by weekend.

I have constructed a test case (attached) and my fear is unfortunately
still there;-< What am i doing wrong?

The packet path is:
-->eth0-->tcpdump eth0-->pedit-->mirror to dummy0-->tcpdump dummy0

I expect pedit to see a cloned packet. It doesnt. The check is in
tcf_pedit(), just before "if (!(skb->tc_verd & TC_OK2MUNGE))"
added: 
printk("pedit: skb-%p is %s\n",skb,skb_cloned(skb)?"cloned":"!cloned");

Is pf packet not cloning etc? Sorry, I dont have much time today
to dig into the code - but i figure youd know the answer.

> > But how can the caller make that decision when you return exactly
> > the same value in the error case as the normal case?
> 
> Ok - i see your point Herbert ;-> 
> it makes sense to have pedit have an error action code like some of the
> others actions which defaults to a drop.
> I will do a proper patch sometime this weekend.

I will get it done this week.

cheers,
jamal

[-- Attachment #2: jiri-q-test --]
[-- Type: text/plain, Size: 1855 bytes --]


machine running script is 10.0.0.111 receiving on eth0.
We are pinging from 10.0.0.26 to 10.0.0.111.
On 10.0.0.111:
1)Edit the packet when it comes in to change src/dst mac addresses
2)Mirror copy to dummy0 

mirror to dummy0 is useful for debugging (ifconfig shows you stats and you
can run tcpdump to log the copies as we do)

run tcpdump before #1 and after #1 - this way we see the original
packet at eth0 and the modified packet at dummy0.

-------------- start script on 10.0.0.111 -----
tc qdisc del dev eth0 ingress     
tc qdisc add dev eth0 ingress     
ifconfig dummy0 up
tc filter add dev eth0 parent ffff: protocol ip prio 10 u32 \
match ip protocol 1 0xff flowid 1:2 \
action pedit \
munge offset -12 u32 set 0x00010100 \
munge offset -8 u32 set 0x0aaf0100 \
munge offset -4 u32 set 0x00080800 pipe \
action mirred egress mirror dev dummy0
------

To validate you did this right, dumping should look as follows:
----
filter protocol ip pref 10 u32 
filter protocol ip pref 10 u32 fh 800: ht divisor 1 
filter protocol ip pref 10 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 1:2 
  match 00010000/00ff0000 at 8
	action order 1:  pedit action pipe keys 3
 	 index 1 ref 1 bind 1
	 key #0  at -12: val 00010100 mask 00000000
	 key #1  at -8: val 0aaf0100 mask 00000000
	 key #2  at -4: val 00080800 mask 00000000
 
	action order 2: mirred (Egress Mirror to device dummy0) pipe
 	index 1 ref 1 bind 1
-----

tcpdump on dummy0 (showing modified macs):
 
0a:af:01:00:00:08 > 52:54:00:01:01:00, ethertype IPv4 (0x0800), length 98: 10.0.0.26 > 10.0.0.111: ICMP echo request, id 5981, seq 1, length 64
	0x0000:  4500 0054 0000 4000 4001 2621 0a00 001a
	0x0010:  0a00 006f 0800 a951 175d 0001 d3c8 fa4b
	0x0020:  0000 0000 9d68 0d00 0000 0000 1011 1213
	0x0030:  1415 1617 1819 1a1b 1c1d 1e1f 2021 2223
	0x0040:  2425 2627 2829 2a2b 2c2d 2e2f