tc ipt action

Hello,

i not sure this is correct list, please advise if not.

i'm trying to use ipt action, and got a problem:

#tc filter add dev eth0 parent ffff: protocol ip u32 match u32 0 0 action ipt -j CONNMARK --restore-mark action mirred egress redirect dev ifb0 -> bad action type ipt

from strace:
open("/usr/lib/tc//m_gact.so", O_RDONLY) = -1 ENOENT (No such file or directory)
write(2, "bad action type ipt\n", 20bad action type ipt

well. i'm trying to use xt:
#tc filter add dev eth0 parent ffff: protocol ip u32 match u32 0 0 action xt -j CONNMARK --restore-mark action mirred egress redirect dev ifb0
xt: unrecognized option '--restore-mark'

from strace:
open("/lib/xtables/libxt_CONNMARK.so", O_RDONLY) = 4
read(4, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0\200\6\0\0004\0\0\0"...,
512) = 512
fstat64(4, {st_mode=S_IFREG|0644, st_size=9756, ...}) = 0 mmap2(NULL, 12548, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 4, 0) = 0xf76f3000
mmap2(0xf76f5000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 4, 0x1) = 0xf76f5000

close(4)                                = 0
mprotect(0xf76f5000, 4096, PROT_READ)   = 0
socket(PF_INET, SOCK_RAW, IPPROTO_RAW)  = 4
fcntl64(4, F_SETFD, FD_CLOEXEC)         = 0

lstat64("/proc/net/ip_tables_names", {st_mode=S_IFREG|0440, st_size=0, ...}) = 0
statfs64("/proc/net/ip_tables_names", 84, {f_type="PROC_SUPER_MAGIC", f_bsize=4096, f_blocks=0, f_bfree=0, f_bavail=0, f_files=0, f_ffree=0, f_fsid={0, 0}, f_namelen=255, f_frsize=4096}) = 0 getsockopt(4, SOL_IP, 0x43 /* IP_??? */, "CONNMARK\0\367\f\300\0\0\0po\367l8p\367\364/p\367:}\302\1", [30]) = 0
close(4) = 0
write(2, "xt: unrecognized option '--resto"..., 41xt: unrecognized option '--restore-mark'

so... i make something wrong or this is a bug ?

ps: 3.6.8 kernel 64 bit kernel with 32 bit userspace, iproute 20121001 from debian-experimental,
module act_ipt is loaded.
pps: please, cc me in reply.

Yury,

This appears to be an ABI breakage on iptables/netfilter side. I will look at it (and hopefully fix it) over the weekend.

cheers,
jamal

Quoted 55 lines · Yury Stankevich

Hello,

i not sure this is correct list, please advise if not.

i'm trying to use ipt action, and got a problem:

#tc filter add dev eth0 parent ffff: protocol ip u32 match u32 0 0 action ipt -j CONNMARK --restore-mark action mirred egress redirect dev ifb0
-> bad action type ipt

from strace:
open("/usr/lib/tc//m_gact.so", O_RDONLY) = -1 ENOENT (No such file or directory)
write(2, "bad action type ipt\n", 20bad action type ipt

well. i'm trying to use xt:
#tc filter add dev eth0 parent ffff: protocol ip u32 match u32 0 0 action xt -j CONNMARK --restore-mark action mirred egress redirect dev ifb0
xt: unrecognized option '--restore-mark'

from strace:
open("/lib/xtables/libxt_CONNMARK.so", O_RDONLY) = 4
read(4, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0\3\0\1\0\0\0\200\6\0\0004\0\0\0"...,
512) = 512
fstat64(4, {st_mode=S_IFREG|0644, st_size=9756, ...}) = 0 mmap2(NULL, 12548, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 4, 0) = 0xf76f3000
mmap2(0xf76f5000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 4, 0x1) = 0xf76f5000

close(4)                                = 0
mprotect(0xf76f5000, 4096, PROT_READ)   = 0
socket(PF_INET, SOCK_RAW, IPPROTO_RAW)  = 4
fcntl64(4, F_SETFD, FD_CLOEXEC)         = 0

lstat64("/proc/net/ip_tables_names", {st_mode=S_IFREG|0440, st_size=0, ...}) = 0
statfs64("/proc/net/ip_tables_names", 84, {f_type="PROC_SUPER_MAGIC", f_bsize=4096, f_blocks=0, f_bfree=0, f_bavail=0, f_files=0, f_ffree=0, f_fsid={0, 0}, f_namelen=255, f_frsize=4096}) = 0 getsockopt(4, SOL_IP, 0x43 /* IP_??? */, "CONNMARK\0\367\f\300\0\0\0po\367l8p\367\364/p\367:}\302\1", [30]) = 0
close(4) = 0
write(2, "xt: unrecognized option '--resto"..., 41xt: unrecognized option '--restore-mark'

so... i make something wrong or this is a bug ?

ps: 3.6.8 kernel 64 bit kernel with 32 bit userspace, iproute 20121001 from debian-experimental,
module act_ipt is loaded.
pps: please, cc me in reply.

Example, the following should work for
tc filter add dev eth0 parent ffff: protocol ip u32 match u32 0 0 action ipt -j CONNMARK \
action mirred egress redirect dev ifb0

Pablo, Hasan Chowdhury tells me this broke after iptable 1.4.10 Hasan also sent me a small patch to fake "xt" instead of "ipt" - but i think there's more than meets the eye here; some interface we are using to talk to xtables on user space seems to have changed.

Quoted 5 lines · Jamal Hadi Salim

Example, the following should work for
tc filter add dev eth0 parent ffff: protocol ip u32 match u32 0 0 action ipt -j CONNMARK \
action mirred egress redirect dev ifb0

If I try that command (substituting ipt->xt and eth0->dummy0, ifb0->dummy1), all I get is the dreaded "Invalid argument". So the kernel rejected the command, which could indicate that userspace construction might have been ok.

# tc filter add dev dummy0 parent ffff: protocol ip u32 match u32 0 0 \
action xt -j CONNMARK action mirred egress redirect dev dummy1

tablename: mangle hook: NF_IP_PRE_ROUTING

       target:  CONNMARK and 0x0 index 0

RTNETLINK answers: Invalid argument
We have an error talking to the kernel

Quoted 4 lines

Pablo, Hasan Chowdhury tells me this broke after iptable 1.4.10 Hasan also sent me a small patch to fake "xt" instead of "ipt" - but i think there's more than meets the eye here; some interface we are using to talk to xtables on user space seems to have changed.

What was the last combination that worked?

Commit v3.7.0~10 caused the variable new PKG_CONFIG variable never to be present at the time of calling make, leading to tc/m_xt.so not linked with -lxtables (result from pkg-config xtables --libs), that in turn leading to

tc: symbol lookup error: /usr/lib64/tc//m_xt.so: undefined symbol:
xtables_init_all

Fixing that.

Signed-off-by Jan Engelhardt <[email protected]>

I can confirm it builds fine for me now if i take out the hack I had and use this patch.

Quoted 2 lines · Jamal Hadi Salim

I can confirm it builds fine for me now if i take out the hack I had and use this patch.

Sorry, I take what i said back and went back to explicitly adding -l xtables. The problem is still the intepretation of tc/Makefile. Here's the compile output.
----
gcc -Wall -Wstrict-prototypes -O2 -I../include -DRESOLVE_HOSTNAMES

On Sunday 2012-12-16 18:03, Jamal Hadi Salim wrote:

I saw the same during make, _but_, on running ldd tc/m_xt.so, I got a libxtables.so entry, so I thought I was fine.

configure +0 -1

@@ -4,7 +4,6 @@
 INCLUDE=${1:-"$PWD/include"}
 
 : ${PKG_CONFIG:=pkg-config}
 : ${CC=gcc}
 
-echo "PKG_CONFIG:=${PKG_CONFIG}" >>Config
 

# Make a temp directory in build tree.
TMPDIR=$(mktemp -d config.XXXXXX)

Commit v3.7.0~10 caused the variable new PKG_CONFIG variable never to be present at the time of calling make, leading to tc/m_xt.so not linked with -lxtables (result from pkg-config xtables --libs), that in turn leading to

tc: symbol lookup error: /usr/lib64/tc//m_xt.so: undefined symbol:
xtables_init_all

Fixing that.

Signed-off-by Jan Engelhardt <[email protected]>

---
 configure |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

configure +0 -1

index 9912114..573ee55 100755
@@ -4,7 +4,6 @@
 INCLUDE=${1:-"$PWD/include"}
 : ${PKG_CONFIG:=pkg-config}
 : ${CC=gcc}
-echo "PKG_CONFIG:=${PKG_CONFIG}" >>Config
 
 # Make a temp directory in build tree.
 TMPDIR=$(mktemp -d config.XXXXXX)

Quoted 23 lines · On Sun, 16 Dec 2012 01:32:48 +0100 Jan Engelhardt wrote:

Commit v3.7.0~10 caused the variable new PKG_CONFIG variable never to be present at the time of calling make, leading to tc/m_xt.so not linked with -lxtables (result from pkg-config xtables --libs), that in turn leading to

tc: symbol lookup error: /usr/lib64/tc//m_xt.so: undefined symbol:
xtables_init_all

Fixing that.

configure +0 -1

@@ -4,7 +4,6 @@
 INCLUDE=${1:-"$PWD/include"}
 : ${PKG_CONFIG:=pkg-config}
 : ${CC=gcc}
-echo "PKG_CONFIG:=${PKG_CONFIG}" >>Config
 
 # Make a temp directory in build tree.
 TMPDIR=$(mktemp -d config.XXXXXX)

@@ -224,6 +223,7 @@ rm -f $TMPDIR/ipsettest.c $TMPDIR/ipsettest
 }
 

configure rm -f $TMPDIR/ipsettest.c $TMPDIR/ipsettest · line 228

Quoted 3 lines

configure +1 -0

 echo "# Generated config based on" $INCLUDE >Config
+echo "PKG_CONFIG:=${PKG_CONFIG}" >>Config
 echo "TC schedulers"

Ok, manually did the diff (conflicted with other previous changes).

this patch is no longer necessary one you merged my:
configure: move toolchain init to a function

it's actually undesirable to apply this after that since it makes the configure script less clear again ...

sorry if my commit message wasn't obvious.
-mike

If I try that command (substituting ipt->xt and eth0->dummy0, ifb0->dummy1), all I get is the dreaded "Invalid argument". So the kernel rejected the command, which could indicate that userspace construction might have been ok.

# tc filter add dev dummy0 parent ffff: protocol ip u32 match u32 0 0 \
action xt -j CONNMARK action mirred egress redirect dev dummy1

tablename: mangle hook: NF_IP_PRE_ROUTING

         target:  CONNMARK and 0x0 index 0

RTNETLINK answers: Invalid argument
We have an error talking to the kernel

What was the last combination that worked?

[Hasan**] I thought "xt" is a supported action kind for iproute2-3.6.0. Besides with a default compilation it compiled m_xt.c (not m_ipt.c ) linked with shared object m_xt.so

the workaround exits in the patch for file m_action (see the changes
there ) as netlink reply from kernel for this tc u32 action xt command
comes as .kind = "ipt" instead of xt (assumed act_ipt.c in kernle
is not aware of new xt extensions .)

Hasan,

I can confirm that xtables_options_xfrm() works.
Just suspicious of that call, "xfrm" seems too specific to xfrm subsystem but generic enough.

"xfrm" is one of these pictogram-based abbreviations like "xing" (the thing they paint on roads/signs), apparently standing for "trans"form and "cross"ing, respectively, though 'x' is ambiguous and open to a lot of other interpretations.

There is an "intermediate solution" from Hasan which doesnt require the kernel change. It changes the kernel endpoint to "ipt". I am conflicted because it is a quick hack while otoh forcing people to upgrade kernel is a usability issue.

Quoted 6 lines · Jamal Hadi Salim

There is an "intermediate solution" from Hasan which doesnt require the kernel change. It changes the kernel endpoint to "ipt". I am conflicted because it is a quick hack while otoh forcing people to upgrade kernel is a usability issue.

Attached. Author is Hasan - I didnt sign it because i am looking for feedback and i find it distasteful but it solves the problem. This is needed until we have a proper fix in the kernel propagated. Once that kernel change is ubiquitous this change is noise and a maintanance pain. I am making it hard to even turn it on (i.e someone knowledgeable will have to compile with CONFIG_XT_HACK)

cheers,
jamal

Maybe xtables should have stable API/ABI and use shim routines there?

Juts noticed , the attached patch does not have the modification for
m_xt.c , without it ,I guess this patch is not going to work.

Hi Jamal,

Thanks for all the help and the information. I will keep tune myself so when the proper path from kernel side will show up I will integrate it into my system to test it.

To be applied pending more testing.

Attached. Sorry, I thought I had sent this out over the weekend. I have done basic testing with a single mark and sending pings to update stats which can then displayed for the mark.

diffstat xt-p1

Kconfig  |   15 ++
Makefile |    1 
act_xt.c |  324 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 339 insertions(+), 1 deletion(-)

Humm... that's a huge patch for what seems to be equal to act_ipt.c Let's do a cross-diff:

Hi Jamal,
I will test it once I get some opportunity , but think I like to know even before any testing

1. What will be the new procedure to compile iproute2 after the patch apply (any new library or any configuration that needs to be adjusted )

2. tc filter add dev eth0 protocol ip parent 1: prio 3 u32 match ip src 192.168.0.0/16 <http://192.168.0.0/16> flowid 1:1 action xt -j MARK --set-mark 3

is this still a valid command ?

Hasan/Yury, if you test this please use the latest iproute2 with only the first patch I posted (originally from Hasan). Hasan please use that patch not your version - if theres anything wrong we can find out sooner before the patch becomes final.

19.12.2012 15:56, Jamal Hadi Salim пишет:

Quoted 4 lines

Hasan/Yury, if you test this please use the latest iproute2 with only the first patch I posted (originally from Hasan). Hasan please use that patch not your version - if theres anything wrong we can find out sooner before the patch becomes final.

Hello,
3.7.1 kernel with 3.7.0 iproute,
patch-xt, xt-p1 + linkage fix was applyed
command successfully performed, but actually doesn't work.

command:
tc filter add dev $dev parent ffff: protocol ip u32 match u32 0 0 \
action xt -j CONNMARK --restore-mark \
action mirred egress redirect dev ifb0
then i use filter:

tc filter add dev ifb0 protocol ip parent 1: prio 2 handle 0xa fw flowid 1:102

iptables line:
iptable -t mangle -A POSTROUTING -p tcp --dport 80 -m connmark --mark 0 -m connbytes --connbytes 204800: --connbytes-dir both --connbytes-mode bytes -j CONNMARK --set-mark 0xa

once i run a test to download 300K file,
from iptables counters i can see that rule in POSTROUTING is triggered, but from tc -s qdisc show dev ifb0 i see that no packets was sent to 1:102 flow.

btw,
tc -p -s filter show dev ifb0 parent 1:
do not show stats (rule hit 416 success 0) for this (filter protocol ip pref 2 fw handle 0xa classid 1:102) rule.

Could be your setup. I didnt do a lot of testing but from my notes (running different kernel at the moment):

#try to point to everything (no iptables setup)
tc filter add dev eth0 parent ffff: protocol ip u32 match u32 0 0 flowid 23:23 action xt -j CONNMARK --restore-mark
#let it run for a 1 sec then display with
tc -s filter show dev eth0 parent ffff:

----
filter protocol ip pref 49152 u32
filter protocol ip pref 49152 u32 fh 800: ht divisor 1 filter protocol ip pref 49152 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid 23:23
match 00000000/00000000 at 0
action order 1: tablename: mangle hook: NF_IP_PRE_ROUTING target CONNMARK restore
index 1 ref 1 bind 1 installed 3 sec used 1 sec
Action statistics:
Sent 280 bytes 4 pkt (dropped 0, overlimits 0 requeues 0) backlog 0b 0p requeues 0
----

cheers,
jamal

Quoted 37 lines · Yury Stankevich

19.12.2012 15:56, Jamal Hadi Salim пишет:

Quoted 4 lines

Hasan/Yury, if you test this please use the latest iproute2 with only the first patch I posted (originally from Hasan). Hasan please use that patch not your version - if theres anything wrong we can find out sooner before the patch becomes final.

Hello,
3.7.1 kernel with 3.7.0 iproute,
patch-xt, xt-p1 + linkage fix was applyed
command successfully performed, but actually doesn't work.

command:
tc filter add dev $dev parent ffff: protocol ip u32 match u32 0 0 \
action xt -j CONNMARK --restore-mark \
action mirred egress redirect dev ifb0
then i use filter:

tc filter add dev ifb0 protocol ip parent 1: prio 2 handle 0xa fw flowid 1:102

iptables line:
iptable -t mangle -A POSTROUTING -p tcp --dport 80 -m connmark --mark 0 -m connbytes --connbytes 204800: --connbytes-dir both --connbytes-mode bytes -j CONNMARK --set-mark 0xa

once i run a test to download 300K file,
from iptables counters i can see that rule in POSTROUTING is triggered, but from tc -s qdisc show dev ifb0 i see that no packets was sent to 1:102 flow.

btw,
tc -p -s filter show dev ifb0 parent 1:
do not show stats (rule hit 416 success 0) for this (filter protocol ip pref 2 fw handle 0xa classid 1:102) rule.

interesting,

#tc -s filter show dev usb0 parent ffff:

Quoted 3 lines

interesting,

#tc -s filter show dev usb0 parent ffff:

Given you are adding this on ingress - the settings you have will happen before pre-routing hook.
If you did things at egress - the setting will take effect after post-routing. So take a closer look at those details they look like your source of issues..

sure,
i use it ingress,
so, i need to use tc xt action
to get mark on the packet, before filter on ifb will run.

prerouting rule, in turn, used to test if mark was actually restored.

in practice:
1. prerouting rule - is not fired. so, no packets with mark was seen.
2. filter on ifb - do not pass traffic to flow configured. looks like CONNMARK --restore is not really called.

Quoted 5 lines

sure,
i use it ingress,
so, i need to use tc xt action
to get mark on the packet, before filter on ifb will run.

Ok. So does ifb see it?

Quoted 1 line

prerouting rule, in turn, used to test if mark was actually restored.

No experience with connmark, but - in order to restore something has to store it, correct?

Quoted 5 lines

in practice:
1. prerouting rule - is not fired. so, no packets with mark was seen.
2. filter on ifb - do not pass traffic to flow configured. looks like CONNMARK --restore is not really called.

My suspicion is that it is not set to begin with...

cheers,
jamal

well.
let me describe whole picture i want to achieve

Having said that, what you are doing sounds so useful that we need to make it work ;-> But it appears like we need a brand new action for it, something like GetMarkFromConntrack. Jan, I am assuming (on ingress only) we need to call "something" to give us the nfct then grab the skb->mark from nfct.

On egress,
I am assuming the skb->mark is already set if connmark is to be used... Am i correct?

Looking up CT before ingress would mean the entire "raw" table needs to be moved before ingress. But with classic ip_tables, calling a table requires a lot of setup (basically ip_rcv).

All new skbs (i.e. those that did not loop due to IPsec, for example) received through __netif_receive_skb should start out with skb->mark=0, which is why CONNMARK --restore-mark is needed to copy skb->mark=ct->mark.

From the description Jan provided in his response, I dont think this used to work at all. Are you saying it worked before?

Having said that, what you are doing sounds so useful that we need to make it work ;-> But it appears like we need a brand new action for it, something like GetMarkFromConntrack.

22.12.2012 17:19, Jamal Hadi Salim пишет:

Quoted 2 lines

From the description Jan provided in his response, I dont think this used to work at all. Are you saying it worked before?

no.
i'm trying if this can work, alas. it can't.

i'm trying if this can work, alas. it can't.

Quoted 1 line · Yury Stankevich

i'm trying if this can work, alas. it can't.

Now i want it to work ;-> So dont give up yet.

cheers,
jamal

Some good news Yury.
I am told Felix Fietkau <[email protected]> (on CC) actually already solved this issue and it is a feature in openwrt. I cant find the code.

Felix - Yury is trying to retrieve skb->mark fields from netfilter connmark. My understanding is you have written such an action. Can you please point us to it - and any reason you havent submitted this for inclusion in kernel proper?

After I added it as an experiment, I got distracted with other projects again and forgot about submitting it. Take a look at the code - if the approach is reasonable, I'll submit this thing for inclusion soon.

net/sched/act_connmark.c new file +9 -0

+static int tcf_connmark(struct sk_buff *skb, const struct tc_action *a,
+		       struct tcf_result *res)
+{
+	struct nf_conn *c;
+	enum ip_conntrack_info ctinfo;
+	int proto;
+	int r;
+
+	if (skb->protocol == htons(ETH_P_IP)) {

net/sched/act_connmark.c new file +4 -0

+	r = nf_conntrack_in(dev_net(skb->dev), proto, NF_INET_PRE_ROUTING, skb);
+	if (r != NF_ACCEPT)
+		goto out;
+

Quoted 11 lines · Jamal Hadi Salim

Some good news Yury.
I am told Felix Fietkau <[email protected]> (on CC) actually already solved this issue and it is a feature in openwrt. I cant find the code.

Felix - Yury is trying to retrieve skb->mark fields from netfilter connmark. My understanding is you have written such an action. Can you please point us to it - and any reason you havent submitted this for inclusion in kernel proper?

After I added it as an experiment, I got distracted with other projects again and forgot about submitting it. Take a look at the code - if the approach is reasonable, I'll submit this thing for inclusion soon.

- Felix

net/sched/act_connmark.c new file +55 -0

@@ -0,0 +1,137 @@
+/*
+ * Copyright (c) 2011 Felix Fietkau <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
+ * Place - Suite 330, Boston, MA 02111-1307 USA.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/skbuff.h>
+#include <linux/rtnetlink.h>
+#include <linux/pkt_cls.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <net/netlink.h>
+#include <net/pkt_sched.h>
+#include <net/act_api.h>
+
+#include <net/netfilter/nf_conntrack.h>
+#include <net/netfilter/nf_conntrack_core.h>
+
+#define TCA_ACT_CONNMARK	20
+
+#define CONNMARK_TAB_MASK     3
+static struct tcf_common *tcf_connmark_ht[CONNMARK_TAB_MASK + 1];
+static u32 connmark_idx_gen;
+static DEFINE_RWLOCK(connmark_lock);
+
+static struct tcf_hashinfo connmark_hash_info = {
+	.htab	=	tcf_connmark_ht,
+	.hmask	=	CONNMARK_TAB_MASK,
+	.lock	=	&connmark_lock,
+};
+
+static int tcf_connmark(struct sk_buff *skb, const struct tc_action *a,
+		       struct tcf_result *res)
+{
+	struct nf_conn *c;
+	enum ip_conntrack_info ctinfo;
+	int proto;
+	int r;
+
+	if (skb->protocol == htons(ETH_P_IP)) {
+		if (skb->len < sizeof(struct iphdr))

conntrack needs to see defragmented packets, you have to call nf_defrag_ipv4 / _ipv6 respectively before that.

This also changes the semantics of the raw table in iptables since it will now see packet with conntrack already attached. So this would also break -j CT --notrack.

This needs more thinking. I can appreciate the value of calling conntrack from different points of the packet traversal, but there are a couple of thing we have to resolve before allowing that.

Quoted 4 lines · Pablo Neira Ayuso

conntrack needs to see defragmented packets, you have to call nf_defrag_ipv4 / _ipv6 respectively before that.

This should not be too hard to do - although my thinking says this should be a separate action.

Quoted 3 lines

This also changes the semantics of the raw table in iptables since it will now see packet with conntrack already attached. So this would also break -j CT --notrack.

Is there a flag we can check which says a flow is not to be tracked? Doesnt nf_conntrack_in() fail if --no track is set?

Quoted 3 lines

This needs more thinking. I can appreciate the value of calling conntrack from different points of the packet traversal, but there are a couple of thing we have to resolve before allowing that.

There is user need for this Pablo - as you can see from what Felix deployed it seems to be used a lot more wider audience dependency. What do we need to do to get this to work properly?

Hi Jamal,

Quoted 9 lines · Jamal Hadi Salim

Quoted 4 lines · Pablo Neira Ayuso

conntrack needs to see defragmented packets, you have to call nf_defrag_ipv4 / _ipv6 respectively before that.

This should not be too hard to do - although my thinking says this should be a separate action.

Quoted 6 lines

Quoted 3 lines

This also changes the semantics of the raw table in iptables since it will now see packet with conntrack already attached. So this would also break -j CT --notrack.

Is there a flag we can check which says a flow is not to be tracked? Doesnt nf_conntrack_in() fail if --no track is set?

The notrack dummy conntrack (consider it a flag) is attached in prerouting raw table. By attaching conntracks at ingress, the notrack flag will be ignored. Note that this also breaks conntrack templates via -j CT, that allows us to set custom conntrack timeouts, zones and helpers at prerouting raw.

Basically, ct templates are attached via -j CT, this template is munched by nf_conntrack_in, which adds the corresponding ct features based on the template information.

Quoted 4 lines · Yury Stankevich

i use it ingress,
so, i need to use tc xt action
to get mark on the packet, before filter on ifb will run. prerouting rule, in turn, used to test if mark was actually restored.

No experience with connmark, but - in order to restore something has to store it, correct?

The bigger problem here, if I see __netif_receive_skb right, is that when ingress rules run, skb->nfct is still unset, thereby the CONNMARK action is a no-op.

On Fri, 2012-12-21 at 15:35 +0100, Jan Engelhardt wrote:

This reminds me this might be the reason we have skb_reset_transport_header(skb);
in __netif_receive_skb(), while its not very logical.

(Yes, sorry for being off topic, but I am referring to
http://www.spinics.net/lists/netdev/msg214662.html )

Quoted 7 lines · Eric Dumazet

On Fri, 2012-12-21 at 15:35 +0100, Jan Engelhardt wrote:

This reminds me this might be the reason we have skb_reset_transport_header(skb);
in __netif_receive_skb(), while its not very logical.

You seem to have nailed the egress part finally. That has been a constant battle. At one point the standard answer was "turn off TSO" ;->

Quoted 2 lines

(Yes, sorry for being off topic, but I am referring to
http://www.spinics.net/lists/netdev/msg214662.html )

I think the skb_reset_transport_header() when Acme made a major overhaul to replace direct pointer access. For this reason i think your second option seems preferable.

Note that network stacks usually reset the transport header anyway, after pulling the network header, so this change only allows a followup patch to have more precise qdisc pkt_len computation for GSO packets at ingress side.

[PATCH net-next] net: introduce skb_transport_header_was_set()

We have skb_mac_header_was_set() helper to tell if mac_header was set on a skb. We would like the same for transport_header.

__netif_receive_skb() doesn't reset the transport header if already set by GRO layer.

Note that network stacks usually reset the transport header anyway, after pulling the network header, so this change only allows a followup patch to have more precise qdisc pkt_len computation for GSO packets at ingress side.

Signed-off-by Eric Dumazet <[email protected]>

Yury,

I took a brief look and run some quick tests on ubuntu 12.04. I am going to be lazy and try and involve the netfilter folks.
It seems that if you left out the args to CONNMARK (includes other targets like MARK etc) you will succeed - but you get default values.

Example, the following should work for
tc filter add dev eth0 parent ffff: protocol ip u32 match u32 0 0 action ipt -j CONNMARK \
action mirred egress redirect dev ifb0

Here is what the output looks like when you dont pass the parameters.

-------

j@ubuntu:~$ sudo tc filter show dev eth0 parent ffff:
filter protocol ip pref 1 u32
filter protocol ip pref 1 u32 fh 800: ht divisor 1
filter protocol ip pref 1 u32 fh 800::800 order 2048 key ht 800 bkt
0 flowid 1:15
  match 0a000015/ffffffff at 12
	action order 1: tablename: mangle  hook: NF_IP_PRE_ROUTING
	target  MARK and 0xffffffff
	index 2 ref 1 bind 1

filter protocol ip pref 49149 u32
filter protocol ip pref 49149 u32 fh 804: ht divisor 1 filter protocol ip pref 49149 u32 fh 804::800 order 2048 key ht 804 bkt 0 flowid 1:12
match 00000000/00000000 at 0
action order 33: tablename: mangle hook: NF_IP_PRE_ROUTING target CONNMARK and 0x0
index 123 ref 1 bind 1
----------------

Pablo, Hasan Chowdhury tells me this broke after iptable 1.4.10 Hasan also sent me a small patch to fake "xt" instead of "ipt" - but i think there's more than meets the eye here; some interface we are using to talk to xtables on user space seems to have changed.

Quoted 5 lines · Jamal Hadi Salim

Example, the following should work for
tc filter add dev eth0 parent ffff: protocol ip u32 match u32 0 0 action ipt -j CONNMARK \
action mirred egress redirect dev ifb0

commit 7299fa4b615d7f7ee12cde444266f6b31f667f9f
libxt_CONNMARK: use guided option parser

BTW, I think it would be good if we find the way to check for libxtables current version (see iptables/configure.ac), so you can know that we broke binary compatibility again.

For the C level, there is XTABLES_VERSION_CODE.

#if XTABLES_VERSION_CODE >= 6
	if (m != NULL && m->x6_parse != NULL)
		m->x6_parse(...)
#else
	else if (m != NULL && m->parse != NULL)
		m->parse(...)
	...

We can also export this through pkgconfig, similar to how downstream users are to discover the plugin dir (pkg-config xtables --variable libdir).

Quoted 4 lines · Jan Engelhardt

For the C level, there is XTABLES_VERSION_CODE.

Quoted 8 lines

#if XTABLES_VERSION_CODE >= 6
	if (m != NULL && m->x6_parse != NULL)
		m->x6_parse(...)
#else
	else if (m != NULL && m->parse != NULL)
		m->parse(...)
	...

I think you are suggesting this to be done in tc. That would make it easier to fix.
IMO, it is easier to keep backward compat if you left the old APIs around for a period of time

and maybe log a warning that they
will be deprecated over a period of time (sort of like kernel approach to changing APIs).

BTW: another interface that seems to have changed that we need is m->final_check().

As you can see, the old ->parse() that goes back to libxtables.so.0 still remains. It's just that... only 5 out of 99 plugins still come with an old parse function.

old parse has not entered any deprecation stage yet, since there are still plugins out there (both the 5 and external ones) that make use of it. Right now, both parse and x6_parse are valid.

Yes, all those with an x6_ prefix are new.
Mh, I already dream of plans reducing m_xt to something that does not require libxtables.so anymore.

Quoted 4 lines

old parse has not entered any deprecation stage yet, since there are still plugins out there (both the 5 and external ones) that make use of it. Right now, both parse and x6_parse are valid.

True - but we are getting broken because we are using a call that only 5 or so users provide. It would have saved us time if we got the
a good log message instead of checking for if !m->parse()

Quoted 3 lines

Yes, all those with an x6_ prefix are new.
Mh, I already dream of plans reducing m_xt to something that does not require libxtables.so anymore.

That would be nice - but someone is going to have to link to libxtables, no?

A certainly safe bet would be to write, at the top of tc/m_xt.c,

#if XTABLES_VERSION_CODE > 9
# error Someone call the guy who changed iptables and \
make him fix it^W^W^W^W say you need help.
#endif

The following is a rough, it-compiles, untested never-run, draft of a module. The vision here is that userspace only ever sends a chain name to the kernel. The git tree/branch for it is

git://git.inai.de/linux xt2-pktsched

commit 42c559c148cbbc22bf2cc29f2ad08bc330891838

Quoted 2 lines

git://git.inai.de/linux xt2-pktsched
commit 42c559c148cbbc22bf2cc29f2ad08bc330891838

I'll look at it later - very slow connection at the moment so cloning will take a while.

If you have a preexisting clone of any linux tree, you can utilize git remote add ... to only grab the deltas.

Quoted 2 lines · Jan Engelhardt

If you have a preexisting clone of any linux tree, you can utilize git remote add ... to only grab the deltas.

It downloaded eventually. So looking at this quickly, basic
question: is xtables2 different API wise from what we do today in act_ipt?

Second: Are chain names unique system wide?

On Monday 2012-12-17 13:58, Jamal Hadi Salim wrote:

AFAICS, (one instance of) act_ipt today directly invokes (exactly one instance of) a target.

With act_xt2 as drafted, it instead invokes a chain, which would

1. leave the construction of the target data and calling it to the subsystems they conceptually belong to - the packet filter
2. lets you do matches, jumps and all that.

Good thing you ask. Chain names are unique within a netns, and this act_xtables.c draft looks at the packet to get to know its netns, so that seems fine.

However, your question also leads to looking at whether TC Actions themselves are sufficiently netns-ified, and it seems this is _not_ the case. Am I right in the observation that variables like "tcf_ipt_ht" are in fact global rather tha per-netns?

Quoted 6 lines · Jan Engelhardt

With act_xt2 as drafted, it instead invokes a chain, which would

1. leave the construction of the target data and calling it to the subsystems they conceptually belong to - the packet filter
2. lets you do matches, jumps and all that.

I like #2. For #1 as long as it doesnt deviate from desire to have one or more instances of targets, we should be fine.

Quoted 3 lines

Good thing you ask. Chain names are unique within a netns, and this act_xtables.c draft looks at the packet to get to know its netns, so that seems fine.

My motivation for that question:
Is it possible to ignore the hook and tablename and just use the chain name?

Chains can store multiple targets, so no loss.

1. table

First, I think some targets need to relax their restrictions, such as with xt_DSCP.

Then, only a handful of extensions remain: CT, <all NATs>, TPROXY and REJECT. Would anyone want to call these from act_ipt? I doubt it. :)

2. hooks

Extensions with hook limit: <NAT>, TPROXY, REJECT, CLASSIFY. Again, I don't quite see the value of attempting to NAT from act_ipt. CLASSIFY {c|sh?}ould be relaxed, unless I am missing something.

The binary interface was broken in 1.4.11 with the guided option parser:

You need a patch to use the new interface to stay in sync with current iptables libraries. I'll make it for tc and send it to you.

BTW, I think it would be good if we find the way to check for libxtables current version (see iptables/configure.ac), so you can know that we broke binary compatibility again.