Re: [PATCH mptcp-next v7 8/8] DO-NOT-MERGE: mptcp: mp_fail test

[Davide Caratti <dcaratti@redhat.com>]

On Wed, Oct 20, 2021 at 04:49:35PM +0200, Matthieu Baerts wrote:
> Hi Davide,
> 
> On 20/10/2021 13:33, Davide Caratti wrote:
> > hi,
> > 
> > On Wed, Oct 20, 2021 at 12:40 PM Matthieu Baerts
> > <matthieu.baerts@tessares.net> wrote:
> >>
> > [...]
> > 
> >>>
> >>> I had a script that used TC flower and gact + pedit + csum actions to flip the 'PSH'
> >>> bit in the TCP header, I promised myself to change it to corrupt the DSS option.
> > 
> > FTR, that script was:
> > 
> > # convert PSH,ACK into ACK
> > # tc filter add dev vetha egress protocol ip prio 1000 flower ip_proto
> > tcp tcp_flags 0x18/0xff action pedit ex munge tcp flags set 0x10 pipe
> > csum tcp reclassify
> > # convert ACK into PSH,ACK, 1 each 10 packets
> > # tc filter add dev vetha egress protocol ip prio 1001 flower ip_proto
> > tcp tcp_flags 0x10/0xff action pass random determ pipe 10 pedit ex
> > munge tcp flags add 0x8 pipe csum tcp
> 
> Thank you for sharing that.
> 
> I managed to change the content of a TCP packet using:
> 
>   $ tc -n <ns> qdisc add dev <iface> root handle 1: htb
> 
>   $ tc -n <ns> filter add dev <iface> parent 1: protocol ip prio 1000
> flower ip_proto tcp action pedit munge offset 148 u32 invert pipe csum tcp
> 
> It looks like nothing happen for packets smaller than 148 bytes.
> 
> Note that I had to add these to the config file:
> 

> CONFIG_NET_SCH_HTB=m

I assume you are using HTB because it's a classful qdisc and it's used to
attach the flower classifier. You don't need it, just use the clsact
qdisc (CONFIG_NET_CLS_ACT=y + CONFIG_NET_SCH_INGRESS=y), it will allow
you inserting filters+actions even if no root qdisc is in place.

# tc qdisc add dev eth0 clsact
# tc filter add dev eth0 egress matchall action simple sdata "hello"

> 
> CONFIG_NET_ACT_CSUM=m
> 
> CONFIG_NET_ACT_GACT=m
> 
> CONFIG_GACT_PROB=y
> 
> CONFIG_NET_ACT_PEDIT=m
> 
> CONFIG_NET_CLS_FLOWER=m
> 
> 
> @Geliang: do you think you could use this command?
> 
> >>> The problem with this approach is, we can't assume that the DSS option has a fixed
> >>> offset from the IP header. So, the test might start corrupting other options that
> >>> are not MPTCP.
> >>
> >> If we corrupt any data in TCP Options or data and update the TCP
> >> Checksum, that's enough to corrupt the DSS. For example, we could flip
> >> the last bit of the TCP payload (if tcp.len > 0). Would it be possible
> >> to do this by adapting your script?
> > 
> > it's easier if it's the first bit, then...
> >
[...] 
 
> >> And using simple solutions like "tc netem corrupt" will be detectable by
> >> TCP csum.
> > 
> > this can be easily workarounded using the 'csum' TC action after the
> > netem dequeue.
> 
> Good point! So we would need to create a tree with first "netem" then
> "csum" action?

the problem is, netem acts *after* TC actions. But probably in your
setup you can overcome this by making netem corrupt when xmitting, and
install the TCP csum  at  the receiver ingress (also here clsact is
helpful):

# ip link add name vetha type veth peer name vethb
# tc qdisc add dev vetha root netem <...>
# tc qdisc a dev vethb clsact
# tc filter add dev vethb ingress protocol ip flower ip_proto tcp action csum tcp

> 
> The only thing is that netem will introduce errors at random positions,
> maybe in IP/TCP headers but it is maybe not an issue. If the TC filter
> command is enough, it might help to produce more controlled tests.

maybe TC flower + action is even more accurate then netem: because it leaves
you the freedom to "corrupt" only packets after the 3WHS (or only the 3WHS,
depending on the match arguments of flower).
You can obtain the same randomization as netem by using TC "act_gact" random
feature ("CONFIG_GACT_PROB=y")

 # tc filter add dev vetha egress protocol ip prio 1001 flower \
  > ip_proto tcp \
  > tcp_flags 0x10/0xff \ <-- only the ACK BIT is set 
  > action pass random determ pipe 10 \    <-- 1 packet every 10 goes to pedit, others go out unmodified
  > pedit munge offset 148 u32 invert pipe csum tcp <-- corrupt the payload and recompute checksum

 
> Indeed it looks like it is doable but if we could not add new
> dependences, I think that would be easier for us :)

Whatever works, it's good also for me :)

-- 
davide