conntrack enhancement

conntrack enhancement

[Jack Ma]

Hi there,

I am planing to add new user option to allow connmark to be shifted to enable more advanced routing options.

Currently, it might be something like:

Conntrack parameters and options:
 
-sl, --shift-left bits                      shift mark by n bit to the left
-sr, --shift-right bits                    shift mark by n bit to the right.

We run out of nfmark (skb->mark) in our systems due to increasing number of routes we are supporting.

One common user-case:

Using connmark to direct traffic via ip rule onto different route tables.

But the the first packet of this flow needs to be per-inspected by the IP-tables first to be marked with an ID (Route number ID in our case.).

If we can have "SHIFT" operation working in conntrack, we would be able to support much more numbers of 'ID'.

I wounder if this "SHIFT" idea can be considered to be accepted by upstream ?

Thanks,
Jack 

Re: conntrack enhancement

[Florian Westphal]

Jack Ma <Jack.Ma@alliedtelesis.co.nz> wrote:
> Hi there,
> 
> I am planing to add new user option to allow connmark to be shifted to enable more advanced routing options.
> 
> Currently, it might be something like:
> 
> Conntrack parameters and options:
>  
> -sl, --shift-left bits                      shift mark by n bit to the left
> -sr, --shift-right bits                    shift mark by n bit to the right.
> 
> We run out of nfmark (skb->mark) in our systems due to increasing number of routes we are supporting.

I suspect this is for -j CONNMARK  --restore-mark / --save-mark ?

What would this do?

Something like

skb->mark = ct->mark >> $lshift; ?
ct->mark = skb->mark >> $lshift; ?

I don't really understand how this is supposed to work.
Could you elaborate a bit?

Thanks.

Re: conntrack enhancement

[Jack Ma]

(Re-send this again including CC)

Hi Florian,

I suspect this is for -j CONNMARK  --restore-mark / --save-mark ?

>> Yes, current thought is to shift bits when mark is restored.

Something like

skb->mark = ct->mark >> $lshift; ?

>> Yes, we shift ct marks.

I don't really understand how this is supposed to work.
Could you elaborate a bit?

>> It's common to run out of fwmark, skb->mark first. This suggestion basically attempts to 'grab' certain bits from ct->mark,
      so we can restore the desired bits into skb->mark. Once skb is marked correctly, we can easily achieve more versatile
      routing per skb. However, this solution is not going to be scalable in the future. Currently, we still have enough bits in ct->mark
      left, but way less bits left in skb->mark (which have been occupied by other features thats connectionless)...


Thanks,

Jack

Re: conntrack enhancement

[Jack Ma]

Hi Florian,

Any comments? Please let me know if anything is unclear to you.

Thanks,
Jack

Re: conntrack enhancement

[Jack Ma]

Hi Florian,


Lets start with iptables setting

iptables -t mangle

Chain EXAMPLE_MAIN

pkts bytes target     prot opt in     out     source               destination 
3709K  204M MARK       all  --  any    any     anywhere             anywhere             MARK and 0xfffff
37  6952 CONNMARK   all  --  any    any     anywhere             anywhere             CONNMARK restore mask 0xfff00000
0     0 MARK       udp  --  any    any     anywhere             anywhere             match .... and MARK xset 0xabc00000/0xfff00000         
    5   308 CONNMARK   all  --  any    any     anywhere             anywhere             CONNMARK save mask 0xfff00000


For the first packet of 'a' flow we mark the packet with fwmark = ct->mark.

ip rule

then direct traffic using such fwmark.

250:    from all fwmark 0xabc lookup TABLE

However, we also implemented firewall feature, packet inspection feature.... etc to use fwmark at the same time.

which makes such 32 bit far more occupied than ct->mark (32)..

Hopefully this can help!

Please let me know if this confuses you..

Thanks,
Jack

shift by n bits while performing '--restore-mark'

[Jack Ma]

Hi Florian,

Our current condition is:

1) only 0xfff00000 (three F available in skb->mark), but 0xfffff000 (five F available in ct->mark)

We wish to copy either 0xfff00000 or 0x00fff000 from ct->mark into skb->mark,


What about '-j CONNMARK  --restore-mark --mask 0xfffff000 << 8 ( left shift 2 F)'

This will result in skb->mark = ct->mark << 8

if ct->mark = 0xabcde000, now skb->mark is changed to:  skb->mark = 0xcde00000.

Does this make sense :) ?


Regards,
Jack


________________________________________
From: Florian Westphal <fw@strlen.de>
Sent: Thursday, January 25, 2018 7:22 PM
To: Jack Ma
Subject: Re: conntrack enhancement

Jack Ma <Jack.Ma@alliedtelesis.co.nz> wrote:
> Hi Florian,
>
> Any comments? Please let me know if anything is unclear to you.

It would be nice if you could show a pseudo-ruleset that uses
this proposed feature, save and restore rule should be enough.

Just so I can see why existing mask support isn't sufficient for your
use case.

Thanks,
Florian

Re: shift by n bits while performing '--restore-mark'

[Florian Westphal]

Jack Ma <Jack.Ma@alliedtelesis.co.nz> wrote:
> Our current condition is:
> 
> 1) only 0xfff00000 (three F available in skb->mark), but 0xfffff000 (five F available in ct->mark)
> 
> We wish to copy either 0xfff00000 or 0x00fff000 from ct->mark into skb->mark,
> 
> 
> What about '-j CONNMARK  --restore-mark --mask 0xfffff000 << 8 ( left shift 2 F)'
> 
> This will result in skb->mark = ct->mark << 8
> 
> if ct->mark = 0xabcde000, now skb->mark is changed to:  skb->mark = 0xcde00000.
> 
> Does this make sense :) ?

Yes it does.

AFAICS with nftables you could already do this but I can understand if
you need to use iptables for this.

So feel free to send a patch from xt_connmark.

Thanks for explaining this.

Re: shift by n bits while performing '--restore-mark'

[Jack Ma]

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

Hi Florian,

I attached two 'draft' patches in this email :)

Thanks,
Jack

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: libxt_CONNMARK.patch --]
[-- Type: text/x-patch; name="libxt_CONNMARK.patch", Size: 9246 bytes --]

From 6d811e63c9c777ed4287bc4547134c99e939b49d Mon Sep 17 00:00:00 2001
From: Jack Ma <jack.ma@alliedtelesis.co.nz>
Date: Mon, 12 Feb 2018 13:41:29 +1300
Subject: [PATCH] libxt_CONNMARK: Support bit-shifting for --restore,set and
 save-mark

Added bit-shifting operations for --restore & set & save-mark.

Signed-off-by: Jack Ma <jack.ma@alliedtelesis.co.nz>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 extensions/libxt_CONNMARK.c           | 176 ++++++++++++++++++++++++++--------
 include/linux/netfilter/xt_connmark.h |   2 +-
 2 files changed, 139 insertions(+), 39 deletions(-)

diff --git a/extensions/libxt_CONNMARK.c b/extensions/libxt_CONNMARK.c
index f60be583..dbd02351 100644
--- a/extensions/libxt_CONNMARK.c
+++ b/extensions/libxt_CONNMARK.c
@@ -28,32 +28,43 @@
 struct xt_connmark_target_info {
 	unsigned long mark;
 	unsigned long mask;
+	uint8_t shift_dir;
+	uint8_t shift_bits;
 	uint8_t mode;
 };
 
 enum {
+	D_SHIFT_LEFT = 0,
+	D_SHIFT_RIGHT,
+};
+
+enum {
 	O_SET_MARK = 0,
 	O_SAVE_MARK,
 	O_RESTORE_MARK,
 	O_AND_MARK,
 	O_OR_MARK,
 	O_XOR_MARK,
+	O_LEFT_SHIFT_MARK,
+	O_RIGHT_SHIFT_MARK,
 	O_SET_XMARK,
 	O_CTMASK,
 	O_NFMASK,
 	O_MASK,
-	F_SET_MARK     = 1 << O_SET_MARK,
-	F_SAVE_MARK    = 1 << O_SAVE_MARK,
-	F_RESTORE_MARK = 1 << O_RESTORE_MARK,
-	F_AND_MARK     = 1 << O_AND_MARK,
-	F_OR_MARK      = 1 << O_OR_MARK,
-	F_XOR_MARK     = 1 << O_XOR_MARK,
-	F_SET_XMARK    = 1 << O_SET_XMARK,
-	F_CTMASK       = 1 << O_CTMASK,
-	F_NFMASK       = 1 << O_NFMASK,
-	F_MASK         = 1 << O_MASK,
-	F_OP_ANY       = F_SET_MARK | F_SAVE_MARK | F_RESTORE_MARK |
-	                 F_AND_MARK | F_OR_MARK | F_XOR_MARK | F_SET_XMARK,
+	F_SET_MARK         = 1 << O_SET_MARK,
+	F_SAVE_MARK        = 1 << O_SAVE_MARK,
+	F_RESTORE_MARK     = 1 << O_RESTORE_MARK,
+	F_AND_MARK         = 1 << O_AND_MARK,
+	F_OR_MARK          = 1 << O_OR_MARK,
+	F_XOR_MARK         = 1 << O_XOR_MARK,
+	F_LEFT_SHIFT_MARK  = 1 << O_LEFT_SHIFT_MARK,
+	F_RIGHT_SHIFT_MARK = 1 << O_RIGHT_SHIFT_MARK,
+	F_SET_XMARK        = 1 << O_SET_XMARK,
+	F_CTMASK           = 1 << O_CTMASK,
+	F_NFMASK           = 1 << O_NFMASK,
+	F_MASK             = 1 << O_MASK,
+	F_OP_ANY           = F_SET_MARK | F_SAVE_MARK | F_RESTORE_MARK |
+	                     F_AND_MARK | F_OR_MARK | F_XOR_MARK | F_SET_XMARK,
 };
 
 static void CONNMARK_help(void)
@@ -74,6 +85,8 @@ static const struct xt_option_entry CONNMARK_opts[] = {
 	{.name = "restore-mark", .id = O_RESTORE_MARK, .type = XTTYPE_NONE,
 	 .excl = F_OP_ANY},
 	{.name = "mask", .id = O_MASK, .type = XTTYPE_UINT32},
+	{.name = "left-shift-mark", .id = O_LEFT_SHIFT_MARK, .type = XTTYPE_UINT8},
+	{.name = "right-shift-mark", .id = O_RIGHT_SHIFT_MARK, .type = XTTYPE_UINT8},
 	XTOPT_TABLEEND,
 };
 #undef s
@@ -94,6 +107,8 @@ static const struct xt_option_entry connmark_tg_opts[] = {
 	 .excl = F_OP_ANY},
 	{.name = "restore-mark", .id = O_RESTORE_MARK, .type = XTTYPE_NONE,
 	 .excl = F_OP_ANY},
+	{.name = "left-shift-mark", .id = O_LEFT_SHIFT_MARK, .type = XTTYPE_UINT8},
+	{.name = "right-shift-mark", .id = O_RIGHT_SHIFT_MARK, .type = XTTYPE_UINT8},
 	{.name = "ctmask", .id = O_CTMASK, .type = XTTYPE_UINT32,
 	 .excl = F_MASK, .flags = XTOPT_PUT, XTOPT_POINTER(s, ctmask)},
 	{.name = "nfmask", .id = O_NFMASK, .type = XTTYPE_UINT32,
@@ -119,6 +134,8 @@ static void connmark_tg_help(void)
 "  --and-mark value              Binary AND the ctmark with bits\n"
 "  --or-mark value               Binary OR  the ctmark with bits\n"
 "  --xor-mark value              Binary XOR the ctmark with bits\n"
+"  --left-shift-mark value       Left shift the ctmark with bits\n"
+"  --right-shift-mark value      Right shift the ctmark with bits\n"
 );
 }
 
@@ -154,6 +171,16 @@ static void CONNMARK_parse(struct xt_option_call *cb)
 	case O_MASK:
 		markinfo->mask = cb->val.u32;
 		break;
+	case O_LEFT_SHIFT_MARK:
+		markinfo->mode = XT_CONNMARK_RESTORE;
+		markinfo->shift_dir = D_SHIFT_LEFT;
+		markinfo->shift_bits = cb->val.u8;
+		break;
+	case O_RIGHT_SHIFT_MARK:
+		markinfo->mode = XT_CONNMARK_RESTORE;
+		markinfo->shift_dir = D_SHIFT_RIGHT;
+		markinfo->shift_bits = cb->val.u8;
+  break;
 	}
 }
 
@@ -197,6 +224,14 @@ static void connmark_tg_parse(struct xt_option_call *cb)
 	case O_MASK:
 		info->nfmask = info->ctmask = cb->val.u32;
 		break;
+	case O_LEFT_SHIFT_MARK:
+		info->shift_dir = D_SHIFT_LEFT;
+		info->shift_bits = cb->val.u8;
+		break;
+	case O_RIGHT_SHIFT_MARK:
+		info->shift_dir = D_SHIFT_RIGHT;
+		info->shift_bits = cb->val.u8;
+  break;
 	}
 }
 
@@ -253,36 +288,101 @@ connmark_tg_print(const void *ip, const struct xt_entry_target *target,
 
 	switch (info->mode) {
 	case XT_CONNMARK_SET:
-		if (info->ctmark == 0)
-			printf(" CONNMARK and 0x%x",
-			       (unsigned int)(uint32_t)~info->ctmask);
-		else if (info->ctmark == info->ctmask)
-			printf(" CONNMARK or 0x%x", info->ctmark);
-		else if (info->ctmask == 0)
-			printf(" CONNMARK xor 0x%x", info->ctmark);
-		else if (info->ctmask == 0xFFFFFFFFU)
-			printf(" CONNMARK set 0x%x", info->ctmark);
-		else
-			printf(" CONNMARK xset 0x%x/0x%x",
-			       info->ctmark, info->ctmask);
+		if (info->ctmark == 0) {
+			if (info->shift_bits)
+				printf(" CONNMARK and 0x%x %s %d",
+					(unsigned int)(uint32_t)~info->ctmask,
+					info->shift_dir == D_SHIFT_LEFT ? "<<" : ">>",
+					info->shift_bits);
+			else
+				printf(" CONNMARK and 0x%x",
+					(unsigned int)(uint32_t)~info->ctmask);
+		}
+		else if (info->ctmark == info->ctmask) {
+			if (info->shift_bits)
+				printf(" CONNMARK or 0x%x %s %d", info->ctmark,
+					info->shift_dir == D_SHIFT_LEFT ? "<<" : ">>",
+					info->shift_bits);
+			else
+				printf(" CONNMARK or 0x%x", info->ctmark);
+		}
+		else if (info->ctmask == 0) {
+			if (info->shift_bits)
+				printf(" CONNMARK xor 0x%x %s %d", info->ctmark,
+					info->shift_dir == D_SHIFT_LEFT ? "<<" : ">>",
+					info->shift_bits);
+			else
+				printf(" CONNMARK xor 0x%x", info->ctmark);
+		}
+		else if (info->ctmask == 0xFFFFFFFFU) {
+			if (info->shift_bits)
+				printf(" CONNMARK set 0x%x %s %d", info->ctmark,
+					info->shift_dir == D_SHIFT_LEFT ? "<<" : ">>",
+					info->shift_bits);
+			else
+				printf(" CONNMARK set 0x%x", info->ctmark);
+		}
+		else {
+			if (info->shift_bits)
+				printf(" CONNMARK xset 0x%x/0x%x %s %d",
+					info->ctmark, info->ctmask, info->shift_dir ==
+					D_SHIFT_LEFT ? "<<" : ">>", info->shift_bits);
+			else
+				printf(" CONNMARK xset 0x%x/0x%x",
+						info->ctmark, info->ctmask);
+		}
 		break;
 	case XT_CONNMARK_SAVE:
-		if (info->nfmask == UINT32_MAX && info->ctmask == UINT32_MAX)
-			printf(" CONNMARK save");
-		else if (info->nfmask == info->ctmask)
-			printf(" CONNMARK save mask 0x%x", info->nfmask);
-		else
-			printf(" CONNMARK save nfmask 0x%x ctmask ~0x%x",
-			       info->nfmask, info->ctmask);
+		if (info->nfmask == UINT32_MAX && info->ctmask == UINT32_MAX) {
+			if (info->shift_bits)
+				printf(" CONNMARK save %s %d", info->shift_dir ==
+					D_SHIFT_LEFT ? "<<" : ">>", info->shift_bits);
+			else
+				printf(" CONNMARK save");
+		}
+		else if (info->nfmask == info->ctmask) {
+			if (info->shift_bits)
+				printf(" CONNMARK save mask 0x%x %s %d", info->nfmask,
+					info->shift_dir == D_SHIFT_LEFT ? "<<" : ">>",
+					info->shift_bits);
+			else
+				printf(" CONNMARK save mask 0x%x", info->nfmask);
+		}
+		else {
+			if (info->shift_bits)
+				printf(" CONNMARK save nfmask 0x%x ctmask ~0x%x %s %d",
+					info->nfmask, info->ctmask, info->shift_dir ==
+					D_SHIFT_LEFT ? "<<" : ">>", info->shift_bits);
+			else
+				printf(" CONNMARK save nfmask 0x%x ctmask ~0x%x",
+					info->nfmask, info->ctmask);
+		}
 		break;
 	case XT_CONNMARK_RESTORE:
-		if (info->ctmask == UINT32_MAX && info->nfmask == UINT32_MAX)
-			printf(" CONNMARK restore");
-		else if (info->ctmask == info->nfmask)
-			printf(" CONNMARK restore mask 0x%x", info->ctmask);
-		else
-			printf(" CONNMARK restore ctmask 0x%x nfmask ~0x%x",
-			       info->ctmask, info->nfmask);
+		if (info->ctmask == UINT32_MAX && info->nfmask == UINT32_MAX) {
+			if (info->shift_bits)
+				printf(" CONNMARK restore %s %d", info->shift_dir ==
+					D_SHIFT_LEFT ? "<<" : ">>", info->shift_bits);
+			else
+				printf(" CONNMARK restore");
+		}
+		else if (info->ctmask == info->nfmask) {
+			if (info->shift_bits)
+				printf(" CONNMARK restore mask 0x%x %s %d", info->ctmask,
+					info->shift_dir == D_SHIFT_LEFT ? "<<" : ">>",
+					info->shift_bits);
+			else
+				printf(" CONNMARK restore mask 0x%x", info->ctmask);
+		}
+		else {
+			if (info->shift_bits)
+				printf(" CONNMARK restore ctmask 0x%x nfmask ~0x%x %s %d",
+					info->ctmask, info->nfmask, info->shift_dir ==
+					D_SHIFT_LEFT ? "<<" : ">>", info->shift_bits);
+			else
+				printf(" CONNMARK restore ctmask 0x%x nfmask ~0x%x",
+					info->ctmask, info->nfmask);
+		}
 		break;
 
 	default:
diff --git a/include/linux/netfilter/xt_connmark.h b/include/linux/netfilter/xt_connmark.h
index efc17a83..2010a40c 100644
--- a/include/linux/netfilter/xt_connmark.h
+++ b/include/linux/netfilter/xt_connmark.h
@@ -20,7 +20,7 @@ enum {
 
 struct xt_connmark_tginfo1 {
 	__u32 ctmark, ctmask, nfmask;
-	__u8 mode;
+	__u8 shift_dir, shift_bits, mode;
 };
 
 struct xt_connmark_mtinfo1 {
-- 
2.13.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: xt_conntrack.patch --]
[-- Type: text/x-patch; name="xt_conntrack.patch", Size: 2441 bytes --]

From b6ff58635bb42cd4c14e03ebfccb9236fd0d5001 Mon Sep 17 00:00:00 2001
From: Jack Ma <jack.ma@alliedtelesis.co.nz>
Date: Mon, 12 Feb 2018 11:13:08 +1300
Subject: [PATCH] xt_conntrack: Support bit-shifting for CONNMARK & MARK.

Bit-shifting operations are added to accompany with --restore,set,
save-mark + --mask.

Signed-off-by: Jack Ma <jack.ma@alliedtelesis.co.nz>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/uapi/linux/netfilter/xt_connmark.h |  7 ++++++-
 net/netfilter/xt_connmark.c                | 16 ++++++++++++----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/netfilter/xt_connmark.h b/include/uapi/linux/netfilter/xt_connmark.h
index 408a9654f05c..28834812124f 100644
--- a/include/uapi/linux/netfilter/xt_connmark.h
+++ b/include/uapi/linux/netfilter/xt_connmark.h
@@ -19,9 +19,14 @@ enum {
 	XT_CONNMARK_RESTORE
 };
 
+enum {
+	D_SHIFT_LEFT = 0,
+	D_SHIFT_RIGHT,
+};
+
 struct xt_connmark_tginfo1 {
 	__u32 ctmark, ctmask, nfmask;
-	__u8 mode;
+	__u8 shift_dir, shift_bits, mode;
 };
 
 struct xt_connmark_mtinfo1 {
diff --git a/net/netfilter/xt_connmark.c b/net/netfilter/xt_connmark.c
index ec377cc6a369..31cb0acb6208 100644
--- a/net/netfilter/xt_connmark.c
+++ b/net/netfilter/xt_connmark.c
@@ -51,7 +51,10 @@ connmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
 	case XT_CONNMARK_SET:
 		newmark = (ct->mark & ~info->ctmask) ^ info->ctmark;
 		if (ct->mark != newmark) {
-			ct->mark = newmark;
+			if (info->shift_dir == D_SHIFT_RIGHT)
+				ct->mark = newmark >> info->shift_bits;
+			else
+				ct->mark = newmark << info->shift_bits;
 			nf_conntrack_event_cache(IPCT_MARK, ct);
 		}
 		break;
@@ -59,17 +62,22 @@ connmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
 		newmark = (ct->mark & ~info->ctmask) ^
 		          (skb->mark & info->nfmask);
 		if (ct->mark != newmark) {
-			ct->mark = newmark;
+			if (info->shift_dir == D_SHIFT_RIGHT)
+				ct->mark = newmark >> info->shift_bits;
+			else
+				ct->mark = newmark << info->shift_bits;
 			nf_conntrack_event_cache(IPCT_MARK, ct);
 		}
 		break;
 	case XT_CONNMARK_RESTORE:
 		newmark = (skb->mark & ~info->nfmask) ^
 		          (ct->mark & info->ctmask);
-		skb->mark = newmark;
+		if (info->shift_dir == D_SHIFT_RIGHT)
+			skb->mark = newmark >> info->shift_bits;
+		else
+			skb->mark = newmark << info->shift_bits;
 		break;
 	}
-
 	return XT_CONTINUE;
 }
 
-- 
2.13.0