netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v1] flow_dissector: Add support for HSR
@ 2022-02-28 19:58 Kurt Kanzenbach
  2022-03-03  6:44 ` Jakub Kicinski
  2022-03-03  7:00 ` patchwork-bot+netdevbpf
  0 siblings, 2 replies; 6+ messages in thread
From: Kurt Kanzenbach @ 2022-02-28 19:58 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski
  Cc: Gustavo A. R. Silva, Alexander Lobakin, Vladimir Oltean,
	Eric Dumazet, Paul Blakey, Yoshiki Komachi, zhang kai,
	Juhee Kang, Andreas Oetken, George McCollister,
	Sebastian Andrzej Siewior, netdev, Kurt Kanzenbach,
	Anthony Harivel

Network drivers such as igb or igc call eth_get_headlen() to determine the
header length for their to be constructed skbs in receive path.

When running HSR on top of these drivers, it results in triggering BUG_ON() in
skb_pull(). The reason is the skb headlen is not sufficient for HSR to work
correctly. skb_pull() notices that.

For instance, eth_get_headlen() returns 14 bytes for TCP traffic over HSR which
is not correct. The problem is, the flow dissection code does not take HSR into
account. Therefore, add support for it.

Reported-by: Anthony Harivel <anthony.harivel@linutronix.de>
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 include/linux/if_hsr.h    | 16 ++++++++++++++++
 net/core/flow_dissector.c | 17 +++++++++++++++++
 net/hsr/hsr_main.h        | 16 ----------------
 3 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
index 38bbc537d4e4..408539d5ea5f 100644
--- a/include/linux/if_hsr.h
+++ b/include/linux/if_hsr.h
@@ -9,6 +9,22 @@ enum hsr_version {
 	PRP_V1,
 };
 
+/* HSR Tag.
+ * As defined in IEC-62439-3:2010, the HSR tag is really { ethertype = 0x88FB,
+ * path, LSDU_size, sequence Nr }. But we let eth_header() create { h_dest,
+ * h_source, h_proto = 0x88FB }, and add { path, LSDU_size, sequence Nr,
+ * encapsulated protocol } instead.
+ *
+ * Field names as defined in the IEC:2010 standard for HSR.
+ */
+struct hsr_tag {
+	__be16		path_and_LSDU_size;
+	__be16		sequence_nr;
+	__be16		encap_proto;
+} __packed;
+
+#define HSR_HLEN	6
+
 #if IS_ENABLED(CONFIG_HSR)
 extern bool is_hsr_master(struct net_device *dev);
 extern int hsr_get_version(struct net_device *dev, enum hsr_version *ver);
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 15833e1d6ea1..34441a32e3be 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -22,6 +22,7 @@
 #include <linux/ppp_defs.h>
 #include <linux/stddef.h>
 #include <linux/if_ether.h>
+#include <linux/if_hsr.h>
 #include <linux/mpls.h>
 #include <linux/tcp.h>
 #include <linux/ptp_classify.h>
@@ -1282,6 +1283,22 @@ bool __skb_flow_dissect(const struct net *net,
 		break;
 	}
 
+	case htons(ETH_P_HSR): {
+		struct hsr_tag *hdr, _hdr;
+
+		hdr = __skb_header_pointer(skb, nhoff, sizeof(_hdr), data, hlen,
+					   &_hdr);
+		if (!hdr) {
+			fdret = FLOW_DISSECT_RET_OUT_BAD;
+			break;
+		}
+
+		proto = hdr->encap_proto;
+		nhoff += HSR_HLEN;
+		fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
+		break;
+	}
+
 	default:
 		fdret = FLOW_DISSECT_RET_OUT_BAD;
 		break;
diff --git a/net/hsr/hsr_main.h b/net/hsr/hsr_main.h
index ca556bda3467..b158ba409f9a 100644
--- a/net/hsr/hsr_main.h
+++ b/net/hsr/hsr_main.h
@@ -45,22 +45,6 @@
 /* PRP V1 life redundancy box MAC address */
 #define PRP_TLV_REDBOX_MAC		   30
 
-/* HSR Tag.
- * As defined in IEC-62439-3:2010, the HSR tag is really { ethertype = 0x88FB,
- * path, LSDU_size, sequence Nr }. But we let eth_header() create { h_dest,
- * h_source, h_proto = 0x88FB }, and add { path, LSDU_size, sequence Nr,
- * encapsulated protocol } instead.
- *
- * Field names as defined in the IEC:2010 standard for HSR.
- */
-struct hsr_tag {
-	__be16		path_and_LSDU_size;
-	__be16		sequence_nr;
-	__be16		encap_proto;
-} __packed;
-
-#define HSR_HLEN	6
-
 #define HSR_V1_SUP_LSDUSIZE		52
 
 #define HSR_HSIZE_SHIFT	8
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v1] flow_dissector: Add support for HSR
  2022-02-28 19:58 [PATCH net-next v1] flow_dissector: Add support for HSR Kurt Kanzenbach
@ 2022-03-03  6:44 ` Jakub Kicinski
  2022-03-03  8:08   ` Kurt Kanzenbach
  2022-03-03  7:00 ` patchwork-bot+netdevbpf
  1 sibling, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2022-03-03  6:44 UTC (permalink / raw)
  To: Kurt Kanzenbach
  Cc: David S. Miller, Gustavo A. R. Silva, Alexander Lobakin,
	Vladimir Oltean, Eric Dumazet, Paul Blakey, Yoshiki Komachi,
	zhang kai, Juhee Kang, Andreas Oetken, George McCollister,
	Sebastian Andrzej Siewior, netdev, Anthony Harivel

On Mon, 28 Feb 2022 20:58:56 +0100 Kurt Kanzenbach wrote:
> Network drivers such as igb or igc call eth_get_headlen() to determine the
> header length for their to be constructed skbs in receive path.
> 
> When running HSR on top of these drivers, it results in triggering BUG_ON() in
> skb_pull(). The reason is the skb headlen is not sufficient for HSR to work
> correctly. skb_pull() notices that.

Should that also be fixed? BUG_ON() seems pretty drastic.

> For instance, eth_get_headlen() returns 14 bytes for TCP traffic over HSR which
> is not correct. The problem is, the flow dissection code does not take HSR into
> account. Therefore, add support for it.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v1] flow_dissector: Add support for HSR
  2022-02-28 19:58 [PATCH net-next v1] flow_dissector: Add support for HSR Kurt Kanzenbach
  2022-03-03  6:44 ` Jakub Kicinski
@ 2022-03-03  7:00 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-03-03  7:00 UTC (permalink / raw)
  To: Kurt Kanzenbach
  Cc: davem, kuba, gustavoars, alobakin, vladimir.oltean, edumazet,
	paulb, komachi.yoshiki, zhangkaiheb, claudiajkang, ennoerlangen,
	george.mccollister, bigeasy, netdev, anthony.harivel

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 28 Feb 2022 20:58:56 +0100 you wrote:
> Network drivers such as igb or igc call eth_get_headlen() to determine the
> header length for their to be constructed skbs in receive path.
> 
> When running HSR on top of these drivers, it results in triggering BUG_ON() in
> skb_pull(). The reason is the skb headlen is not sufficient for HSR to work
> correctly. skb_pull() notices that.
> 
> [...]

Here is the summary with links:
  - [net-next,v1] flow_dissector: Add support for HSR
    https://git.kernel.org/netdev/net-next/c/bf08824a0f47

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v1] flow_dissector: Add support for HSR
  2022-03-03  6:44 ` Jakub Kicinski
@ 2022-03-03  8:08   ` Kurt Kanzenbach
  2022-03-03 15:31     ` Jakub Kicinski
  0 siblings, 1 reply; 6+ messages in thread
From: Kurt Kanzenbach @ 2022-03-03  8:08 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: David S. Miller, Gustavo A. R. Silva, Alexander Lobakin,
	Vladimir Oltean, Eric Dumazet, Paul Blakey, Yoshiki Komachi,
	zhang kai, Juhee Kang, Andreas Oetken, George McCollister,
	Sebastian Andrzej Siewior, netdev, Anthony Harivel

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

Hi Jakub,

On Wed Mar 02 2022, Jakub Kicinski wrote:
> On Mon, 28 Feb 2022 20:58:56 +0100 Kurt Kanzenbach wrote:
>> Network drivers such as igb or igc call eth_get_headlen() to determine the
>> header length for their to be constructed skbs in receive path.
>> 
>> When running HSR on top of these drivers, it results in triggering BUG_ON() in
>> skb_pull(). The reason is the skb headlen is not sufficient for HSR to work
>> correctly. skb_pull() notices that.
>
> Should that also be fixed? BUG_ON() seems pretty drastic.

It's this statement here:

 https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/skbuff.h#n2483

I tried to look up, why is this a BUG_ON() in Thomas' history tree
[1]. Couldn't find an explanation. It's been introduced by this commit:

|commit 1a0153507ffae9cf3350e76c12d441788c0191e1 (HEAD)
|Author: Linus Torvalds <torvalds@athlon.transmeta.com>
|Date:   Mon Feb 4 18:11:38 2002 -0800
|
|    v2.4.3.2 -> v2.4.3.3
|    
|      - Hui-Fen Hsu: sis900 driver update
|      - NIIBE Yutaka: Super-H update
|      - Alan Cox: more resyncs (ARM down, but more to go)
|      - David Miller: network zerocopy, Sparc sync, qlogic,FC fix, etc.
|      - David Miller/me: get rid of various drivers hacks to do mmap
|      alignment behind the back of the VM layer. Create a real
|      protocol for it.

It seems like BUG/BUG_ON() is the error handling practice in case of
unavailable memory. Even though most functions such as skb_push() or
skb_put() use asserts or skb_over_panic() which also result in BUG() at
the end.

Thanks,
Kurt

[1] - https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v1] flow_dissector: Add support for HSR
  2022-03-03  8:08   ` Kurt Kanzenbach
@ 2022-03-03 15:31     ` Jakub Kicinski
  2022-03-03 15:48       ` Kurt Kanzenbach
  0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2022-03-03 15:31 UTC (permalink / raw)
  To: Kurt Kanzenbach
  Cc: David S. Miller, Gustavo A. R. Silva, Alexander Lobakin,
	Vladimir Oltean, Eric Dumazet, Paul Blakey, Yoshiki Komachi,
	zhang kai, Juhee Kang, Andreas Oetken, George McCollister,
	Sebastian Andrzej Siewior, netdev, Anthony Harivel

On Thu, 03 Mar 2022 09:08:35 +0100 Kurt Kanzenbach wrote:
> It's this statement here:
> 
>  https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/skbuff.h#n2483
> 
> I tried to look up, why is this a BUG_ON() in Thomas' history tree
> [1]. Couldn't find an explanation. It's been introduced by this commit:

I meant fix the caller to discard a frame if it doesn't have enough
data - call pskb_may_pull() first, then skb_pull().

> |commit 1a0153507ffae9cf3350e76c12d441788c0191e1 (HEAD)
> |Author: Linus Torvalds <torvalds@athlon.transmeta.com>
> |Date:   Mon Feb 4 18:11:38 2002 -0800
> |
> |    v2.4.3.2 -> v2.4.3.3
> |    
> |      - Hui-Fen Hsu: sis900 driver update
> |      - NIIBE Yutaka: Super-H update
> |      - Alan Cox: more resyncs (ARM down, but more to go)
> |      - David Miller: network zerocopy, Sparc sync, qlogic,FC fix, etc.
> |      - David Miller/me: get rid of various drivers hacks to do mmap
> |      alignment behind the back of the VM layer. Create a real
> |      protocol for it.
> 
> It seems like BUG/BUG_ON() is the error handling practice in case of
> unavailable memory. Even though most functions such as skb_push() or
> skb_put() use asserts or skb_over_panic() which also result in BUG() at
> the end.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v1] flow_dissector: Add support for HSR
  2022-03-03 15:31     ` Jakub Kicinski
@ 2022-03-03 15:48       ` Kurt Kanzenbach
  0 siblings, 0 replies; 6+ messages in thread
From: Kurt Kanzenbach @ 2022-03-03 15:48 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: David S. Miller, Gustavo A. R. Silva, Alexander Lobakin,
	Vladimir Oltean, Eric Dumazet, Paul Blakey, Yoshiki Komachi,
	zhang kai, Juhee Kang, Andreas Oetken, George McCollister,
	Sebastian Andrzej Siewior, netdev, Anthony Harivel

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

On Thu Mar 03 2022, Jakub Kicinski wrote:
> On Thu, 03 Mar 2022 09:08:35 +0100 Kurt Kanzenbach wrote:
>> It's this statement here:
>> 
>>  https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/include/linux/skbuff.h#n2483
>> 
>> I tried to look up, why is this a BUG_ON() in Thomas' history tree
>> [1]. Couldn't find an explanation. It's been introduced by this commit:
>
> I meant fix the caller to discard a frame if it doesn't have enough
> data - call pskb_may_pull() first, then skb_pull().

I see. Yes, the caller (hsr_forward_skb) needs fixing too.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-03-03 15:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-28 19:58 [PATCH net-next v1] flow_dissector: Add support for HSR Kurt Kanzenbach
2022-03-03  6:44 ` Jakub Kicinski
2022-03-03  8:08   ` Kurt Kanzenbach
2022-03-03 15:31     ` Jakub Kicinski
2022-03-03 15:48       ` Kurt Kanzenbach
2022-03-03  7:00 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).