All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC] flow_dissector: Add FLOW_DISSECTOR_F_FLOWER
@ 2017-09-29 19:13 Tom Herbert
  2017-10-02 14:49 ` Jiri Pirko
  0 siblings, 1 reply; 3+ messages in thread
From: Tom Herbert @ 2017-09-29 19:13 UTC (permalink / raw)
  To: davem; +Cc: hannes, netdev, rohit, Tom Herbert

This patch is RFC and would be applied after "flow_dissector:
Protocol specific flow dissector offload"

In order to maitain uAPI in flower, the FLOW_DISSECTOR_F_FLOWER flag
is added to indicate to flow_dissector that the caller is flower.
As new funtionality is addes to flow_dissector that would break
the flower uAPI, the code can be wrapped in "if (!(flags &
FLOW_DISSECTOR_F_FLOWER)).

In this patch the conditional is use around protocol specific
dissection (e.g. DPI into VXLAN) as well as the code that
enforces a depth of parsing to prevent DPI. The latter was a
recent patch that would introduce a parsing limit to flower that
did not exist before (i.e. would break uAPI).

Signed-off-by: Tom Herbert <tom@quantonium.net>
---
 include/net/flow_dissector.h |  1 +
 net/core/flow_dissector.c    | 17 +++++++++++------
 net/sched/cls_flow.c         |  3 ++-
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index ad75bbfd1c9c..ca315107d147 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -214,6 +214,7 @@ enum flow_dissector_key_id {
 #define FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL	BIT(2)
 #define FLOW_DISSECTOR_F_STOP_AT_ENCAP		BIT(3)
 #define FLOW_DISSECTOR_F_STOP_AT_L4		BIT(4)
+#define FLOW_DISSECTOR_F_FLOWER			BIT(5)
 
 struct flow_dissector_key {
 	enum flow_dissector_key_id key_id;
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 84b8eb1f6664..a4d5843ec34e 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -383,8 +383,11 @@ __skb_flow_dissect_ipv6(const struct sk_buff *skb,
  */
 #define MAX_FLOW_DISSECT_HDRS	15
 
-static bool skb_flow_dissect_allowed(int *num_hdrs)
+static bool skb_flow_dissect_allowed(int *num_hdrs, unsigned int flags)
 {
+	if (flags & FLOW_DISSECTOR_F_FLOWER)
+		return true;
+
 	++*num_hdrs;
 
 	return (*num_hdrs <= MAX_FLOW_DISSECT_HDRS);
@@ -722,7 +725,8 @@ bool __skb_flow_dissect(struct sk_buff *skb,
 		break;
 
 	default:
-		fdret = flow_dissect_by_type(skb, proto, key_control,
+		if (!(flags & FLOW_DISSECTOR_F_FLOWER))
+			fdret = flow_dissect_by_type(skb, proto, key_control,
 					     flow_dissector,
 					     target_container,
 					     data, &proto, &ip_proto, &nhoff,
@@ -735,7 +739,7 @@ bool __skb_flow_dissect(struct sk_buff *skb,
 	case FLOW_DISSECT_RET_OUT_GOOD:
 		goto out_good;
 	case FLOW_DISSECT_RET_PROTO_AGAIN:
-		if (skb_flow_dissect_allowed(&num_hdrs))
+		if (skb_flow_dissect_allowed(&num_hdrs, flags))
 			goto proto_again;
 		goto out_good;
 	case FLOW_DISSECT_RET_CONTINUE:
@@ -843,7 +847,8 @@ bool __skb_flow_dissect(struct sk_buff *skb,
 		break;
 
 	default:
-		fdret = flow_dissect_by_type_proto(skb, proto,
+		if (!(flags & FLOW_DISSECTOR_F_FLOWER))
+			fdret = flow_dissect_by_type_proto(skb, proto,
 						ip_proto, key_control,
 						flow_dissector,
 						target_container,
@@ -872,11 +877,11 @@ bool __skb_flow_dissect(struct sk_buff *skb,
 	/* Process result of IP proto processing */
 	switch (fdret) {
 	case FLOW_DISSECT_RET_PROTO_AGAIN:
-		if (skb_flow_dissect_allowed(&num_hdrs))
+		if (skb_flow_dissect_allowed(&num_hdrs, flags))
 			goto proto_again;
 		break;
 	case FLOW_DISSECT_RET_IPPROTO_AGAIN:
-		if (skb_flow_dissect_allowed(&num_hdrs))
+		if (skb_flow_dissect_allowed(&num_hdrs, flags))
 			goto ip_proto_again;
 		break;
 	case FLOW_DISSECT_RET_OUT_GOOD:
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 2a3a60ec5b86..ad4ac503697e 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -315,7 +315,8 @@ static int flow_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 
 		keymask = f->keymask;
 		if (keymask & FLOW_KEYS_NEEDED)
-			skb_flow_dissect_flow_keys(skb, &flow_keys, 0);
+			skb_flow_dissect_flow_keys(skb, &flow_keys,
+						   FLOW_DISSECTOR_F_FLOWER);
 
 		for (n = 0; n < f->nkeys; n++) {
 			key = ffs(keymask) - 1;
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH RFC] flow_dissector: Add FLOW_DISSECTOR_F_FLOWER
  2017-09-29 19:13 [PATCH RFC] flow_dissector: Add FLOW_DISSECTOR_F_FLOWER Tom Herbert
@ 2017-10-02 14:49 ` Jiri Pirko
  2017-10-02 16:05   ` Tom Herbert
  0 siblings, 1 reply; 3+ messages in thread
From: Jiri Pirko @ 2017-10-02 14:49 UTC (permalink / raw)
  To: Tom Herbert; +Cc: davem, hannes, netdev, rohit

Fri, Sep 29, 2017 at 09:13:42PM CEST, tom@quantonium.net wrote:
>This patch is RFC and would be applied after "flow_dissector:
>Protocol specific flow dissector offload"
>
>In order to maitain uAPI in flower, the FLOW_DISSECTOR_F_FLOWER flag
>is added to indicate to flow_dissector that the caller is flower.
>As new funtionality is addes to flow_dissector that would break
>the flower uAPI, the code can be wrapped in "if (!(flags &
>FLOW_DISSECTOR_F_FLOWER)).
>
>In this patch the conditional is use around protocol specific
>dissection (e.g. DPI into VXLAN) as well as the code that
>enforces a depth of parsing to prevent DPI. The latter was a
>recent patch that would introduce a parsing limit to flower that
>did not exist before (i.e. would break uAPI).
>
>Signed-off-by: Tom Herbert <tom@quantonium.net>
>---
> include/net/flow_dissector.h |  1 +
> net/core/flow_dissector.c    | 17 +++++++++++------
> net/sched/cls_flow.c         |  3 ++-
> 3 files changed, 14 insertions(+), 7 deletions(-)
>
>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>index ad75bbfd1c9c..ca315107d147 100644
>--- a/include/net/flow_dissector.h
>+++ b/include/net/flow_dissector.h
>@@ -214,6 +214,7 @@ enum flow_dissector_key_id {
> #define FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL	BIT(2)
> #define FLOW_DISSECTOR_F_STOP_AT_ENCAP		BIT(3)
> #define FLOW_DISSECTOR_F_STOP_AT_L4		BIT(4)
>+#define FLOW_DISSECTOR_F_FLOWER			BIT(5)

I don't like flow_dissector to have any user-specific bits. Note that
the same dissection may be used not only from flower, but from other
code as well (OVS). Flow dissector should not care who the caller is.

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH RFC] flow_dissector: Add FLOW_DISSECTOR_F_FLOWER
  2017-10-02 14:49 ` Jiri Pirko
@ 2017-10-02 16:05   ` Tom Herbert
  0 siblings, 0 replies; 3+ messages in thread
From: Tom Herbert @ 2017-10-02 16:05 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: Tom Herbert, David S. Miller, Hannes Frederic Sowa,
	Linux Kernel Network Developers, Rohit Seth

On Mon, Oct 2, 2017 at 7:49 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Fri, Sep 29, 2017 at 09:13:42PM CEST, tom@quantonium.net wrote:
>>This patch is RFC and would be applied after "flow_dissector:
>>Protocol specific flow dissector offload"
>>
>>In order to maitain uAPI in flower, the FLOW_DISSECTOR_F_FLOWER flag
>>is added to indicate to flow_dissector that the caller is flower.
>>As new funtionality is addes to flow_dissector that would break
>>the flower uAPI, the code can be wrapped in "if (!(flags &
>>FLOW_DISSECTOR_F_FLOWER)).
>>
>>In this patch the conditional is use around protocol specific
>>dissection (e.g. DPI into VXLAN) as well as the code that
>>enforces a depth of parsing to prevent DPI. The latter was a
>>recent patch that would introduce a parsing limit to flower that
>>did not exist before (i.e. would break uAPI).
>>
>>Signed-off-by: Tom Herbert <tom@quantonium.net>
>>---
>> include/net/flow_dissector.h |  1 +
>> net/core/flow_dissector.c    | 17 +++++++++++------
>> net/sched/cls_flow.c         |  3 ++-
>> 3 files changed, 14 insertions(+), 7 deletions(-)
>>
>>diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
>>index ad75bbfd1c9c..ca315107d147 100644
>>--- a/include/net/flow_dissector.h
>>+++ b/include/net/flow_dissector.h
>>@@ -214,6 +214,7 @@ enum flow_dissector_key_id {
>> #define FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL   BIT(2)
>> #define FLOW_DISSECTOR_F_STOP_AT_ENCAP                BIT(3)
>> #define FLOW_DISSECTOR_F_STOP_AT_L4           BIT(4)
>>+#define FLOW_DISSECTOR_F_FLOWER                       BIT(5)
>
> I don't like flow_dissector to have any user-specific bits. Note that
> the same dissection may be used not only from flower, but from other
> code as well (OVS). Flow dissector should not care who the caller is.

I agree with that, but unfortunately that's now how it works in
reality. As pointed out flower has assumed flow_dissector semantics as
its uAPI, so we can't change flow dissector with out considering this
one specific caller even if all other use cases of flow dissector
don't care.

If you don't like this approach, then please suggest an alternative
that will achieve the same effect.

Thanks,
Tom

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-10-02 16:05 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-29 19:13 [PATCH RFC] flow_dissector: Add FLOW_DISSECTOR_F_FLOWER Tom Herbert
2017-10-02 14:49 ` Jiri Pirko
2017-10-02 16:05   ` Tom Herbert

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.