Skip to contentSkip to conversation

skbedit: allow the user to specify bitmask for mark

PATCH Applied Antonio Quartulli 3 participants 5 messages Jul 23, 2014 – Oct 27, 2016

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Antonio Quartulli

author
#0 lore raw

PATCH v3 1/1 skbedit: allow the user to specify bitmask for mark

From: Antonio Quartulli <[email protected]>

The user may want to use only some bits of the skb mark in his skbedit rules because the remaining part might be used by something else.

Introduce the "mask" parameter to the skbedit actor in order to implement such functionality.

When the mask is specified, only those bits selected by the latter are altered really changed by the actor, while the rest is left untouched.

Signed-off-by Antonio Quartulli <[email protected]>
Signed-off-by Jamal Hadi Salim <[email protected]>

---

Changes from v2:
- remove useless comments
- use nla_put_u32() and fix typ0

Changes from v1:
- use '&=' tcf_skbedit() to clean the mark
- extend tcf_skbedit_dump() in order to send the mask as well

 include/net/tc_act/tc_skbedit.h        |  1 +
 include/uapi/linux/tc_act/tc_skbedit.h |  2 ++
 net/sched/act_skbedit.c                | 21 ++++++++++++++++++---
 3 files changed, 21 insertions(+), 3 deletions(-)
include/net/tc_act/tc_skbedit.h +1 -0
index 0df9a0d..a385c68 100644
@@ -26,6 +26,7 @@ struct tcf_skbedit {
 	u32			flags;
 	u32     		priority;
 	u32     		mark;
+	u32			mask;
 	u16			queue_mapping;
 	/* XXX: 16-bit pad here? */
 };
include/uapi/linux/tc_act/tc_skbedit.h +2 -0
index 7a2e910..c7c8c5f 100644
@@ -27,6 +27,7 @@
 #define SKBEDIT_F_PRIORITY		0x1
 #define SKBEDIT_F_QUEUE_MAPPING		0x2
 #define SKBEDIT_F_MARK			0x4
+#define SKBEDIT_F_MASK			0x8
 
 struct tc_skbedit {
 	tc_gen;

@@ -39,6 +40,7 @@ enum {
 	TCA_SKBEDIT_PRIORITY,
 	TCA_SKBEDIT_QUEUE_MAPPING,
 	TCA_SKBEDIT_MARK,
+	TCA_SKBEDIT_MASK,
 	__TCA_SKBEDIT_MAX
 };
 #define TCA_SKBEDIT_MAX (__TCA_SKBEDIT_MAX - 1)
net/sched/act_skbedit.c +18 -3
index fcfeeaf..4409bdb 100644
@@ -43,8 +43,10 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
 	if (d->flags & SKBEDIT_F_QUEUE_MAPPING &&
 	    skb->dev->real_num_tx_queues > d->queue_mapping)
 		skb_set_queue_mapping(skb, d->queue_mapping);
-	if (d->flags & SKBEDIT_F_MARK)
-		skb->mark = d->mark;
+	if (d->flags & SKBEDIT_F_MARK) {
+		skb->mark &= ~d->mask;
+		skb->mark |= d->mark & d->mask;
+	}
 
 	spin_unlock(&d->tcf_lock);
 	return d->tcf_action;

@@ -55,6 +57,7 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
 	[TCA_SKBEDIT_PRIORITY]		= { .len = sizeof(u32) },
 	[TCA_SKBEDIT_QUEUE_MAPPING]	= { .len = sizeof(u16) },
 	[TCA_SKBEDIT_MARK]		= { .len = sizeof(u32) },
+	[TCA_SKBEDIT_MASK]		= { .len = sizeof(u32) },
 };
 
 static int tcf_skbedit_init(struct net *net, struct nlattr *nla,

@@ -64,7 +67,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	struct nlattr *tb[TCA_SKBEDIT_MAX + 1];
 	struct tc_skbedit *parm;
 	struct tcf_skbedit *d;
-	u32 flags = 0, *priority = NULL, *mark = NULL;
+	u32 flags = 0, *priority = NULL, *mark = NULL, *mask = NULL;
 	u16 *queue_mapping = NULL;
 	int ret = 0, err;
 

@@ -93,6 +96,11 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 		mark = nla_data(tb[TCA_SKBEDIT_MARK]);
 	}
 
+	if (tb[TCA_SKBEDIT_MASK] != NULL) {
+		flags |= SKBEDIT_F_MASK;
+		mask = nla_data(tb[TCA_SKBEDIT_MASK]);
+	}
+
 	if (!flags)
 		return -EINVAL;
 

@@ -123,6 +131,10 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 		d->queue_mapping = *queue_mapping;
 	if (flags & SKBEDIT_F_MARK)
 		d->mark = *mark;
+	/* default behaviour is to use all the bits */
+	d->mask = 0xffffffff;
+	if (flags & SKBEDIT_F_MASK)
+		d->mask = *mask;
 
 	d->tcf_action = parm->action;
 

@@ -160,6 +172,9 @@ static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a,
 	    nla_put(skb, TCA_SKBEDIT_MARK, sizeof(d->mark),
 		    &d->mark))
 		goto nla_put_failure;
+	if ((d->flags & SKBEDIT_F_MASK) &&
+	    nla_put_u32(skb, TCA_SKBEDIT_MASK, d->mask);
+		goto nla_put_failure;
 	t.install = jiffies_to_clock_t(jiffies - d->tcf_tm.install);
 	t.lastuse = jiffies_to_clock_t(jiffies - d->tcf_tm.lastuse);
 	t.expires = jiffies_to_clock_t(d->tcf_tm.expires);
Signature · 1 line
1.8.5.5
maintainer Applied
#1 lore raw

From: Antonio Quartulli <[email protected]>
Date: Wed, 23 Jul 2014 21:13:35 +0200

Quoted 9 lines

From: Antonio Quartulli <[email protected]>

The user may want to use only some bits of the skb mark in his skbedit rules because the remaining part might be used by something else.

Introduce the "mask" parameter to the skbedit actor in order to implement such functionality.

Quoted 6 lines

When the mask is specified, only those bits selected by the latter are altered really changed by the actor, while the rest is left untouched.

Signed-off-by Antonio Quartulli <[email protected]>
Signed-off-by Jamal Hadi Salim <[email protected]>

Applied, thank you.

maintainer
#2 lore raw

From: David Miller <[email protected]>
Date: Thu, 24 Jul 2014 18:03:23 -0700 (PDT)

Quoted 12 lines

From: Antonio Quartulli <[email protected]>
Date: Wed, 23 Jul 2014 21:13:35 +0200

Quoted 9 lines

From: Antonio Quartulli <[email protected]>

The user may want to use only some bits of the skb mark in his skbedit rules because the remaining part might be used by something else.

Introduce the "mask" parameter to the skbedit actor in order to implement such functionality.

Quoted 8 lines
Quoted 6 lines

When the mask is specified, only those bits selected by the latter are altered really changed by the actor, while the rest is left untouched.

Signed-off-by Antonio Quartulli <[email protected]>
Signed-off-by Jamal Hadi Salim <[email protected]>

Applied, thank you.

Actually reverted, it doesn't even build:

+ if ((d->flags & SKBEDIT_F_MASK) &&
+ nla_put_u32(skb, TCA_SKBEDIT_MASK, d->mask);

How did you test this?

You didn't even compile this code.

#3 lore raw

PATCH v4 1/1 skbedit: allow the user to specify bitmask for mark

The user may want to use only some bits of the skb mark in his skbedit rules because the remaining part might be used by something else.

Introduce the "mask" parameter to the skbedit actor in order to implement such functionality.

When the mask is specified, only those bits selected by the latter are altered really changed by the actor, while the rest is left untouched.

Signed-off-by Antonio Quartulli <[email protected]>
Signed-off-by Jamal Hadi Salim <[email protected]>

---

This patch has been sleeping for a while although it was basically ready for being merged. I hope it can still be merged.

Checkpatch is now complaining about this:

CHECK: Comparison to NULL could be written "tb[TCA_SKBEDIT_MASK]" #112: FILE: net/sched/act_skbedit.c:114:
+ if (tb[TCA_SKBEDIT_MASK] != NULL) {

However the surrounding code does not use this codestyle. Please, let me know if I should rearrange this line.

Thanks!

Changes from v3:
- rebase on top of net-next
- fix syntax error in if statement

Changes from v2:
- remove useless comments
- use nla_put_u32() and fix typ0

Changes from v1:
- use '&=' tcf_skbedit() to clean the mark
- extend tcf_skbedit_dump() in order to send the mask as well

 include/net/tc_act/tc_skbedit.h        |  1 +
 include/uapi/linux/tc_act/tc_skbedit.h |  2 ++
 net/sched/act_skbedit.c                | 21 ++++++++++++++++++---
 3 files changed, 21 insertions(+), 3 deletions(-)
include/net/tc_act/tc_skbedit.h +1 -0
index 5767e9dbcf92..19cd3d345804 100644
@@ -27,6 +27,7 @@ struct tcf_skbedit {
 	u32		flags;
 	u32		priority;
 	u32		mark;
+	u32		mask;
 	u16		queue_mapping;
 	u16		ptype;
 };
include/uapi/linux/tc_act/tc_skbedit.h +2 -0
index a4d00c608d8f..2884425738ce 100644
@@ -28,6 +28,7 @@
 #define SKBEDIT_F_QUEUE_MAPPING		0x2
 #define SKBEDIT_F_MARK			0x4
 #define SKBEDIT_F_PTYPE			0x8
+#define SKBEDIT_F_MASK			0x10
 
 struct tc_skbedit {
 	tc_gen;

@@ -42,6 +43,7 @@ enum {
 	TCA_SKBEDIT_MARK,
 	TCA_SKBEDIT_PAD,
 	TCA_SKBEDIT_PTYPE,
+	TCA_SKBEDIT_MASK,
 	__TCA_SKBEDIT_MAX
 };
 #define TCA_SKBEDIT_MAX (__TCA_SKBEDIT_MAX - 1)
net/sched/act_skbedit.c +18 -3
index a133dcb82132..024f3a3afeff 100644
@@ -46,8 +46,10 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
 	if (d->flags & SKBEDIT_F_QUEUE_MAPPING &&
 	    skb->dev->real_num_tx_queues > d->queue_mapping)
 		skb_set_queue_mapping(skb, d->queue_mapping);
-	if (d->flags & SKBEDIT_F_MARK)
-		skb->mark = d->mark;
+	if (d->flags & SKBEDIT_F_MARK) {
+		skb->mark &= ~d->mask;
+		skb->mark |= d->mark & d->mask;
+	}
 	if (d->flags & SKBEDIT_F_PTYPE)
 		skb->pkt_type = d->ptype;
 

@@ -61,6 +63,7 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
 	[TCA_SKBEDIT_QUEUE_MAPPING]	= { .len = sizeof(u16) },
 	[TCA_SKBEDIT_MARK]		= { .len = sizeof(u32) },
 	[TCA_SKBEDIT_PTYPE]		= { .len = sizeof(u16) },
+	[TCA_SKBEDIT_MASK]		= { .len = sizeof(u32) },
 };
 
 static int tcf_skbedit_init(struct net *net, struct nlattr *nla,

@@ -71,7 +74,7 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	struct nlattr *tb[TCA_SKBEDIT_MAX + 1];
 	struct tc_skbedit *parm;
 	struct tcf_skbedit *d;
-	u32 flags = 0, *priority = NULL, *mark = NULL;
+	u32 flags = 0, *priority = NULL, *mark = NULL, *mask = NULL;
 	u16 *queue_mapping = NULL, *ptype = NULL;
 	bool exists = false;
 	int ret = 0, err;

@@ -108,6 +111,11 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 		mark = nla_data(tb[TCA_SKBEDIT_MARK]);
 	}
 
+	if (tb[TCA_SKBEDIT_MASK] != NULL) {
+		flags |= SKBEDIT_F_MASK;
+		mask = nla_data(tb[TCA_SKBEDIT_MASK]);
+	}
+
 	parm = nla_data(tb[TCA_SKBEDIT_PARMS]);
 
 	exists = tcf_hash_check(tn, parm->index, a, bind);

@@ -145,6 +153,10 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 		d->mark = *mark;
 	if (flags & SKBEDIT_F_PTYPE)
 		d->ptype = *ptype;
+	/* default behaviour is to use all the bits */
+	d->mask = 0xffffffff;
+	if (flags & SKBEDIT_F_MASK)
+		d->mask = *mask;
 
 	d->tcf_action = parm->action;
 

@@ -182,6 +194,9 @@ static int tcf_skbedit_dump(struct sk_buff *skb, struct tc_action *a,
 	if ((d->flags & SKBEDIT_F_PTYPE) &&
 	    nla_put_u16(skb, TCA_SKBEDIT_PTYPE, d->ptype))
 		goto nla_put_failure;
+	if ((d->flags & SKBEDIT_F_MASK) &&
+	    nla_put_u32(skb, TCA_SKBEDIT_MASK, d->mask))
+		goto nla_put_failure;
 
 	tcf_tm_dump(&t, &d->tcf_tm);
 	if (nla_put_64bit(skb, TCA_SKBEDIT_TM, sizeof(t), &t, TCA_SKBEDIT_PAD))
Signature · 1 line
2.10.1
maintainer Applied
#4 lore raw

From: Antonio Quartulli <[email protected]>
Date: Mon, 24 Oct 2016 20:32:57 +0800

Quoted 7 lines

The user may want to use only some bits of the skb mark in his skbedit rules because the remaining part might be used by something else.

Introduce the "mask" parameter to the skbedit actor in order to implement such functionality.

Quoted 6 lines

When the mask is specified, only those bits selected by the latter are altered really changed by the actor, while the rest is left untouched.

Signed-off-by Antonio Quartulli <[email protected]>
Signed-off-by Jamal Hadi Salim <[email protected]>

Applied, thanks.