[RFC PATCH net-next v3 0/4] ice: PPPoE offload support

[RFC PATCH net-next v3 0/4] ice: PPPoE offload support

[Marcin Szycik]

Add support for dissecting PPPoE and PPP-specific fields in flow dissector:
PPPoE session id and PPP protocol type. Add support for those fields in
tc-flower and support offloading PPPoE. Finally, add support for hardware
offload of PPPoE packets in switchdev mode in ice driver.

Example filter:
tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
    1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR

Changes in iproute2 are required to use the new fields (will be submitted
soon).

ICE COMMS DDP package is required to create a filter in ice.

Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
will add this feature.

[0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com

v3: revert byte order changes in is_ppp_proto_supported from previous
    version, add kernel-doc for is_ppp_proto_supported, add more CC
v2: cosmetic changes

Marcin Szycik (1):
  ice: Add support for PPPoE hardware offload

Wojciech Drewek (3):
  flow_dissector: Add PPPoE dissectors
  net/sched: flower: Add PPPoE filter
  flow_offload: Introduce flow_match_pppoe

 drivers/net/ethernet/intel/ice/ice.h          |   1 +
 .../net/ethernet/intel/ice/ice_flex_pipe.c    |   5 +-
 .../ethernet/intel/ice/ice_protocol_type.h    |  11 ++
 drivers/net/ethernet/intel/ice/ice_switch.c   | 165 ++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_tc_lib.c   |  95 ++++++++--
 drivers/net/ethernet/intel/ice/ice_tc_lib.h   |   8 +
 include/net/flow_dissector.h                  |  11 ++
 include/net/flow_offload.h                    |   6 +
 include/uapi/linux/pkt_cls.h                  |   3 +
 net/core/flow_dissector.c                     |  55 +++++-
 net/core/flow_offload.c                       |   7 +
 net/sched/cls_flower.c                        |  46 +++++
 12 files changed, 396 insertions(+), 17 deletions(-)

-- 
2.35.1

[Intel-wired-lan] [RFC PATCH net-next v3 0/4] ice: PPPoE offload support

[Marcin Szycik]

Add support for dissecting PPPoE and PPP-specific fields in flow dissector:
PPPoE session id and PPP protocol type. Add support for those fields in
tc-flower and support offloading PPPoE. Finally, add support for hardware
offload of PPPoE packets in switchdev mode in ice driver.

Example filter:
tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
    1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR

Changes in iproute2 are required to use the new fields (will be submitted
soon).

ICE COMMS DDP package is required to create a filter in ice.

Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
will add this feature.

[0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com

v3: revert byte order changes in is_ppp_proto_supported from previous
    version, add kernel-doc for is_ppp_proto_supported, add more CC
v2: cosmetic changes

Marcin Szycik (1):
  ice: Add support for PPPoE hardware offload

Wojciech Drewek (3):
  flow_dissector: Add PPPoE dissectors
  net/sched: flower: Add PPPoE filter
  flow_offload: Introduce flow_match_pppoe

 drivers/net/ethernet/intel/ice/ice.h          |   1 +
 .../net/ethernet/intel/ice/ice_flex_pipe.c    |   5 +-
 .../ethernet/intel/ice/ice_protocol_type.h    |  11 ++
 drivers/net/ethernet/intel/ice/ice_switch.c   | 165 ++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_tc_lib.c   |  95 ++++++++--
 drivers/net/ethernet/intel/ice/ice_tc_lib.h   |   8 +
 include/net/flow_dissector.h                  |  11 ++
 include/net/flow_offload.h                    |   6 +
 include/uapi/linux/pkt_cls.h                  |   3 +
 net/core/flow_dissector.c                     |  55 +++++-
 net/core/flow_offload.c                       |   7 +
 net/sched/cls_flower.c                        |  46 +++++
 12 files changed, 396 insertions(+), 17 deletions(-)

-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors

[Marcin Szycik]

From: Wojciech Drewek <wojciech.drewek@intel.com>

Allow to dissect PPPoE specific fields which are:
- session ID (16 bits)
- ppp protocol (16 bits)

The goal is to make the following TC command possible:

  # tc filter add dev ens6f0 ingress prio 1 protocol ppp_ses \
      flower \
        pppoe_sid 12 \
        ppp_proto ip \
      action drop

Note that only PPPoE Session is supported.

Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
v3: revert byte order changes in is_ppp_proto_supported from previous 
    version, add kernel-doc for is_ppp_proto_supported
v2: use ntohs instead of htons in is_ppp_proto_supported

 include/net/flow_dissector.h | 11 ++++++++
 net/core/flow_dissector.c    | 55 ++++++++++++++++++++++++++++++++----
 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index a4c6057c7097..8ff40c7c3f1c 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -261,6 +261,16 @@ struct flow_dissector_key_num_of_vlans {
 	u8 num_of_vlans;
 };
 
+/**
+ * struct flow_dissector_key_pppoe:
+ * @session_id: pppoe session id
+ * @ppp_proto: ppp protocol
+ */
+struct flow_dissector_key_pppoe {
+	u16 session_id;
+	__be16 ppp_proto;
+};
+
 enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
@@ -291,6 +301,7 @@ enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_CT, /* struct flow_dissector_key_ct */
 	FLOW_DISSECTOR_KEY_HASH, /* struct flow_dissector_key_hash */
 	FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */
+	FLOW_DISSECTOR_KEY_PPPOE,  /* struct flow_dissector_key_pppoe */
 
 	FLOW_DISSECTOR_KEY_MAX,
 };
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 6aee04f75e3e..42393af477a2 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -895,6 +895,39 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
 	return result == BPF_OK;
 }
 
+/**
+ * is_ppp_proto_supported - checks if inner PPP protocol should be dissected
+ * @proto: protocol type (PPP proto field)
+ */
+static bool is_ppp_proto_supported(__be16 proto)
+{
+	switch (proto) {
+	case htons(PPP_AT):
+	case htons(PPP_IPX):
+	case htons(PPP_VJC_COMP):
+	case htons(PPP_VJC_UNCOMP):
+	case htons(PPP_MP):
+	case htons(PPP_COMPFRAG):
+	case htons(PPP_COMP):
+	case htons(PPP_MPLS_UC):
+	case htons(PPP_MPLS_MC):
+	case htons(PPP_IPCP):
+	case htons(PPP_ATCP):
+	case htons(PPP_IPXCP):
+	case htons(PPP_IPV6CP):
+	case htons(PPP_CCPFRAG):
+	case htons(PPP_MPLSCP):
+	case htons(PPP_LCP):
+	case htons(PPP_PAP):
+	case htons(PPP_LQR):
+	case htons(PPP_CHAP):
+	case htons(PPP_CBCP):
+		return true;
+	default:
+		return false;
+	}
+}
+
 /**
  * __skb_flow_dissect - extract the flow_keys struct and return it
  * @net: associated network namespace, derived from @skb if NULL
@@ -1221,19 +1254,29 @@ bool __skb_flow_dissect(const struct net *net,
 		}
 
 		nhoff += PPPOE_SES_HLEN;
-		switch (hdr->proto) {
-		case htons(PPP_IP):
+		if (hdr->proto == htons(PPP_IP)) {
 			proto = htons(ETH_P_IP);
 			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
-			break;
-		case htons(PPP_IPV6):
+		} else if (hdr->proto == htons(PPP_IPV6)) {
 			proto = htons(ETH_P_IPV6);
 			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
-			break;
-		default:
+		} else if (is_ppp_proto_supported(hdr->proto)) {
+			fdret = FLOW_DISSECT_RET_OUT_GOOD;
+		} else {
 			fdret = FLOW_DISSECT_RET_OUT_BAD;
 			break;
 		}
+
+		if (dissector_uses_key(flow_dissector,
+				       FLOW_DISSECTOR_KEY_PPPOE)) {
+			struct flow_dissector_key_pppoe *key_pppoe;
+
+			key_pppoe = skb_flow_dissector_target(flow_dissector,
+							      FLOW_DISSECTOR_KEY_PPPOE,
+							      target_container);
+			key_pppoe->session_id = ntohs(hdr->hdr.sid);
+			key_pppoe->ppp_proto = hdr->proto;
+		}
 		break;
 	}
 	case htons(ETH_P_TIPC): {
-- 
2.35.1

[Intel-wired-lan] [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors

[Marcin Szycik]

From: Wojciech Drewek <wojciech.drewek@intel.com>

Allow to dissect PPPoE specific fields which are:
- session ID (16 bits)
- ppp protocol (16 bits)

The goal is to make the following TC command possible:

  # tc filter add dev ens6f0 ingress prio 1 protocol ppp_ses \
      flower \
        pppoe_sid 12 \
        ppp_proto ip \
      action drop

Note that only PPPoE Session is supported.

Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
v3: revert byte order changes in is_ppp_proto_supported from previous 
    version, add kernel-doc for is_ppp_proto_supported
v2: use ntohs instead of htons in is_ppp_proto_supported

 include/net/flow_dissector.h | 11 ++++++++
 net/core/flow_dissector.c    | 55 ++++++++++++++++++++++++++++++++----
 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index a4c6057c7097..8ff40c7c3f1c 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -261,6 +261,16 @@ struct flow_dissector_key_num_of_vlans {
 	u8 num_of_vlans;
 };
 
+/**
+ * struct flow_dissector_key_pppoe:
+ * @session_id: pppoe session id
+ * @ppp_proto: ppp protocol
+ */
+struct flow_dissector_key_pppoe {
+	u16 session_id;
+	__be16 ppp_proto;
+};
+
 enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
 	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
@@ -291,6 +301,7 @@ enum flow_dissector_key_id {
 	FLOW_DISSECTOR_KEY_CT, /* struct flow_dissector_key_ct */
 	FLOW_DISSECTOR_KEY_HASH, /* struct flow_dissector_key_hash */
 	FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */
+	FLOW_DISSECTOR_KEY_PPPOE,  /* struct flow_dissector_key_pppoe */
 
 	FLOW_DISSECTOR_KEY_MAX,
 };
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index 6aee04f75e3e..42393af477a2 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -895,6 +895,39 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
 	return result == BPF_OK;
 }
 
+/**
+ * is_ppp_proto_supported - checks if inner PPP protocol should be dissected
+ * @proto: protocol type (PPP proto field)
+ */
+static bool is_ppp_proto_supported(__be16 proto)
+{
+	switch (proto) {
+	case htons(PPP_AT):
+	case htons(PPP_IPX):
+	case htons(PPP_VJC_COMP):
+	case htons(PPP_VJC_UNCOMP):
+	case htons(PPP_MP):
+	case htons(PPP_COMPFRAG):
+	case htons(PPP_COMP):
+	case htons(PPP_MPLS_UC):
+	case htons(PPP_MPLS_MC):
+	case htons(PPP_IPCP):
+	case htons(PPP_ATCP):
+	case htons(PPP_IPXCP):
+	case htons(PPP_IPV6CP):
+	case htons(PPP_CCPFRAG):
+	case htons(PPP_MPLSCP):
+	case htons(PPP_LCP):
+	case htons(PPP_PAP):
+	case htons(PPP_LQR):
+	case htons(PPP_CHAP):
+	case htons(PPP_CBCP):
+		return true;
+	default:
+		return false;
+	}
+}
+
 /**
  * __skb_flow_dissect - extract the flow_keys struct and return it
  * @net: associated network namespace, derived from @skb if NULL
@@ -1221,19 +1254,29 @@ bool __skb_flow_dissect(const struct net *net,
 		}
 
 		nhoff += PPPOE_SES_HLEN;
-		switch (hdr->proto) {
-		case htons(PPP_IP):
+		if (hdr->proto == htons(PPP_IP)) {
 			proto = htons(ETH_P_IP);
 			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
-			break;
-		case htons(PPP_IPV6):
+		} else if (hdr->proto == htons(PPP_IPV6)) {
 			proto = htons(ETH_P_IPV6);
 			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
-			break;
-		default:
+		} else if (is_ppp_proto_supported(hdr->proto)) {
+			fdret = FLOW_DISSECT_RET_OUT_GOOD;
+		} else {
 			fdret = FLOW_DISSECT_RET_OUT_BAD;
 			break;
 		}
+
+		if (dissector_uses_key(flow_dissector,
+				       FLOW_DISSECTOR_KEY_PPPOE)) {
+			struct flow_dissector_key_pppoe *key_pppoe;
+
+			key_pppoe = skb_flow_dissector_target(flow_dissector,
+							      FLOW_DISSECTOR_KEY_PPPOE,
+							      target_container);
+			key_pppoe->session_id = ntohs(hdr->hdr.sid);
+			key_pppoe->ppp_proto = hdr->proto;
+		}
 		break;
 	}
 	case htons(ETH_P_TIPC): {
-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[RFC PATCH net-next v3 2/4] net/sched: flower: Add PPPoE filter

[Marcin Szycik]

From: Wojciech Drewek <wojciech.drewek@intel.com>

Add support for PPPoE specific fields for tc-flower.
Those fields can be provided only when protocol was set
to ETH_P_PPP_SES. Defines, dump, load and set are being done here.

Overwrite basic.n_proto only in case of PPP_IP and PPP_IPV6,
otherwise leave it as ETH_P_PPP_SES.

Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
 include/uapi/linux/pkt_cls.h |  3 +++
 net/sched/cls_flower.c       | 46 ++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 9a2ee1e39fad..a67dcd8294c9 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -589,6 +589,9 @@ enum {
 
 	TCA_FLOWER_KEY_NUM_OF_VLANS,    /* u8 */
 
+	TCA_FLOWER_KEY_PPPOE_SID,	/* u16 */
+	TCA_FLOWER_KEY_PPP_PROTO,	/* be16 */
+
 	__TCA_FLOWER_MAX,
 };
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index dcca70144dff..f6a0bb87f869 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -16,6 +16,7 @@
 #include <linux/in6.h>
 #include <linux/ip.h>
 #include <linux/mpls.h>
+#include <linux/ppp_defs.h>
 
 #include <net/sch_generic.h>
 #include <net/pkt_cls.h>
@@ -73,6 +74,7 @@ struct fl_flow_key {
 	struct flow_dissector_key_ct ct;
 	struct flow_dissector_key_hash hash;
 	struct flow_dissector_key_num_of_vlans num_of_vlans;
+	struct flow_dissector_key_pppoe pppoe;
 } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */
 
 struct fl_flow_mask_range {
@@ -714,6 +716,8 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_HASH]		= { .type = NLA_U32 },
 	[TCA_FLOWER_KEY_HASH_MASK]	= { .type = NLA_U32 },
 	[TCA_FLOWER_KEY_NUM_OF_VLANS]	= { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_PPPOE_SID]	= { .type = NLA_U16 },
+	[TCA_FLOWER_KEY_PPP_PROTO]	= { .type = NLA_U16 },
 
 };
 
@@ -1041,6 +1045,32 @@ static void fl_set_key_vlan(struct nlattr **tb,
 	}
 }
 
+static void fl_set_key_pppoe(struct nlattr **tb,
+			     struct flow_dissector_key_pppoe *key_val,
+			     struct flow_dissector_key_pppoe *key_mask,
+			     struct fl_flow_key *key,
+			     struct fl_flow_key *mask)
+{
+	if (tb[TCA_FLOWER_KEY_PPPOE_SID]) {
+		key_val->session_id =
+			nla_get_u16(tb[TCA_FLOWER_KEY_PPPOE_SID]);
+		key_mask->session_id = 0xffff;
+	}
+	if (tb[TCA_FLOWER_KEY_PPP_PROTO]) {
+		key_val->ppp_proto =
+			nla_get_be16(tb[TCA_FLOWER_KEY_PPP_PROTO]);
+		key_mask->ppp_proto = cpu_to_be16(~0);
+
+		if (key_val->ppp_proto == htons(PPP_IP)) {
+			key->basic.n_proto = htons(ETH_P_IP);
+			mask->basic.n_proto = cpu_to_be16(~0);
+		} else if (key_val->ppp_proto == htons(PPP_IPV6)) {
+			key->basic.n_proto = htons(ETH_P_IPV6);
+			mask->basic.n_proto = cpu_to_be16(~0);
+		}
+	}
+}
+
 static void fl_set_key_flag(u32 flower_key, u32 flower_mask,
 			    u32 *dissector_key, u32 *dissector_mask,
 			    u32 flower_flag_bit, u32 dissector_flag_bit)
@@ -1651,6 +1681,9 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 		}
 	}
 
+	if (key->basic.n_proto == htons(ETH_P_PPP_SES))
+		fl_set_key_pppoe(tb, &key->pppoe, &mask->pppoe, key, mask);
+
 	if (key->basic.n_proto == htons(ETH_P_IP) ||
 	    key->basic.n_proto == htons(ETH_P_IPV6)) {
 		fl_set_key_val(tb, &key->basic.ip_proto, TCA_FLOWER_KEY_IP_PROTO,
@@ -1923,6 +1956,8 @@ static void fl_init_dissector(struct flow_dissector *dissector,
 			     FLOW_DISSECTOR_KEY_HASH, hash);
 	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
 			     FLOW_DISSECTOR_KEY_NUM_OF_VLANS, num_of_vlans);
+	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
+			     FLOW_DISSECTOR_KEY_PPPOE, pppoe);
 
 	skb_flow_dissector_init(dissector, keys, cnt);
 }
@@ -3051,6 +3086,17 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net,
 	    fl_dump_key_ip(skb, false, &key->ip, &mask->ip)))
 		goto nla_put_failure;
 
+	if (mask->pppoe.session_id) {
+		if (nla_put_u16(skb, TCA_FLOWER_KEY_PPPOE_SID,
+				key->pppoe.session_id))
+			goto nla_put_failure;
+	}
+	if (mask->basic.n_proto && mask->pppoe.ppp_proto) {
+		if (nla_put_be16(skb, TCA_FLOWER_KEY_PPP_PROTO,
+				 key->pppoe.ppp_proto))
+			goto nla_put_failure;
+	}
+
 	if (key->control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS &&
 	    (fl_dump_key_val(skb, &key->ipv4.src, TCA_FLOWER_KEY_IPV4_SRC,
 			     &mask->ipv4.src, TCA_FLOWER_KEY_IPV4_SRC_MASK,
-- 
2.35.1

[Intel-wired-lan] [RFC PATCH net-next v3 2/4] net/sched: flower: Add PPPoE filter

[Marcin Szycik]

From: Wojciech Drewek <wojciech.drewek@intel.com>

Add support for PPPoE specific fields for tc-flower.
Those fields can be provided only when protocol was set
to ETH_P_PPP_SES. Defines, dump, load and set are being done here.

Overwrite basic.n_proto only in case of PPP_IP and PPP_IPV6,
otherwise leave it as ETH_P_PPP_SES.

Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
 include/uapi/linux/pkt_cls.h |  3 +++
 net/sched/cls_flower.c       | 46 ++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index 9a2ee1e39fad..a67dcd8294c9 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -589,6 +589,9 @@ enum {
 
 	TCA_FLOWER_KEY_NUM_OF_VLANS,    /* u8 */
 
+	TCA_FLOWER_KEY_PPPOE_SID,	/* u16 */
+	TCA_FLOWER_KEY_PPP_PROTO,	/* be16 */
+
 	__TCA_FLOWER_MAX,
 };
 
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index dcca70144dff..f6a0bb87f869 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -16,6 +16,7 @@
 #include <linux/in6.h>
 #include <linux/ip.h>
 #include <linux/mpls.h>
+#include <linux/ppp_defs.h>
 
 #include <net/sch_generic.h>
 #include <net/pkt_cls.h>
@@ -73,6 +74,7 @@ struct fl_flow_key {
 	struct flow_dissector_key_ct ct;
 	struct flow_dissector_key_hash hash;
 	struct flow_dissector_key_num_of_vlans num_of_vlans;
+	struct flow_dissector_key_pppoe pppoe;
 } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */
 
 struct fl_flow_mask_range {
@@ -714,6 +716,8 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
 	[TCA_FLOWER_KEY_HASH]		= { .type = NLA_U32 },
 	[TCA_FLOWER_KEY_HASH_MASK]	= { .type = NLA_U32 },
 	[TCA_FLOWER_KEY_NUM_OF_VLANS]	= { .type = NLA_U8 },
+	[TCA_FLOWER_KEY_PPPOE_SID]	= { .type = NLA_U16 },
+	[TCA_FLOWER_KEY_PPP_PROTO]	= { .type = NLA_U16 },
 
 };
 
@@ -1041,6 +1045,32 @@ static void fl_set_key_vlan(struct nlattr **tb,
 	}
 }
 
+static void fl_set_key_pppoe(struct nlattr **tb,
+			     struct flow_dissector_key_pppoe *key_val,
+			     struct flow_dissector_key_pppoe *key_mask,
+			     struct fl_flow_key *key,
+			     struct fl_flow_key *mask)
+{
+	if (tb[TCA_FLOWER_KEY_PPPOE_SID]) {
+		key_val->session_id =
+			nla_get_u16(tb[TCA_FLOWER_KEY_PPPOE_SID]);
+		key_mask->session_id = 0xffff;
+	}
+	if (tb[TCA_FLOWER_KEY_PPP_PROTO]) {
+		key_val->ppp_proto =
+			nla_get_be16(tb[TCA_FLOWER_KEY_PPP_PROTO]);
+		key_mask->ppp_proto = cpu_to_be16(~0);
+
+		if (key_val->ppp_proto == htons(PPP_IP)) {
+			key->basic.n_proto = htons(ETH_P_IP);
+			mask->basic.n_proto = cpu_to_be16(~0);
+		} else if (key_val->ppp_proto == htons(PPP_IPV6)) {
+			key->basic.n_proto = htons(ETH_P_IPV6);
+			mask->basic.n_proto = cpu_to_be16(~0);
+		}
+	}
+}
+
 static void fl_set_key_flag(u32 flower_key, u32 flower_mask,
 			    u32 *dissector_key, u32 *dissector_mask,
 			    u32 flower_flag_bit, u32 dissector_flag_bit)
@@ -1651,6 +1681,9 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
 		}
 	}
 
+	if (key->basic.n_proto == htons(ETH_P_PPP_SES))
+		fl_set_key_pppoe(tb, &key->pppoe, &mask->pppoe, key, mask);
+
 	if (key->basic.n_proto == htons(ETH_P_IP) ||
 	    key->basic.n_proto == htons(ETH_P_IPV6)) {
 		fl_set_key_val(tb, &key->basic.ip_proto, TCA_FLOWER_KEY_IP_PROTO,
@@ -1923,6 +1956,8 @@ static void fl_init_dissector(struct flow_dissector *dissector,
 			     FLOW_DISSECTOR_KEY_HASH, hash);
 	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
 			     FLOW_DISSECTOR_KEY_NUM_OF_VLANS, num_of_vlans);
+	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
+			     FLOW_DISSECTOR_KEY_PPPOE, pppoe);
 
 	skb_flow_dissector_init(dissector, keys, cnt);
 }
@@ -3051,6 +3086,17 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net,
 	    fl_dump_key_ip(skb, false, &key->ip, &mask->ip)))
 		goto nla_put_failure;
 
+	if (mask->pppoe.session_id) {
+		if (nla_put_u16(skb, TCA_FLOWER_KEY_PPPOE_SID,
+				key->pppoe.session_id))
+			goto nla_put_failure;
+	}
+	if (mask->basic.n_proto && mask->pppoe.ppp_proto) {
+		if (nla_put_be16(skb, TCA_FLOWER_KEY_PPP_PROTO,
+				 key->pppoe.ppp_proto))
+			goto nla_put_failure;
+	}
+
 	if (key->control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS &&
 	    (fl_dump_key_val(skb, &key->ipv4.src, TCA_FLOWER_KEY_IPV4_SRC,
 			     &mask->ipv4.src, TCA_FLOWER_KEY_IPV4_SRC_MASK,
-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[RFC PATCH net-next v3 3/4] flow_offload: Introduce flow_match_pppoe

[Marcin Szycik]

From: Wojciech Drewek <wojciech.drewek@intel.com>

Allow to offload PPPoE filters by adding flow_rule_match_pppoe.
Drivers can extract PPPoE specific fields from now on.

Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
 include/net/flow_offload.h | 6 ++++++
 net/core/flow_offload.c    | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 6484095a8c01..50283a30b97b 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -72,6 +72,10 @@ struct flow_match_ct {
 	struct flow_dissector_key_ct *key, *mask;
 };
 
+struct flow_match_pppoe {
+	struct flow_dissector_key_pppoe *key, *mask;
+};
+
 struct flow_rule;
 
 void flow_rule_match_meta(const struct flow_rule *rule,
@@ -116,6 +120,8 @@ void flow_rule_match_enc_opts(const struct flow_rule *rule,
 			      struct flow_match_enc_opts *out);
 void flow_rule_match_ct(const struct flow_rule *rule,
 			struct flow_match_ct *out);
+void flow_rule_match_pppoe(const struct flow_rule *rule,
+			   struct flow_match_pppoe *out);
 
 enum flow_action_id {
 	FLOW_ACTION_ACCEPT		= 0,
diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 929f6379a279..d13d293e3de2 100644
--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c
@@ -223,6 +223,13 @@ void flow_rule_match_ct(const struct flow_rule *rule,
 }
 EXPORT_SYMBOL(flow_rule_match_ct);
 
+void flow_rule_match_pppoe(const struct flow_rule *rule,
+			   struct flow_match_pppoe *out)
+{
+	FLOW_DISSECTOR_MATCH(rule, FLOW_DISSECTOR_KEY_PPPOE, out);
+}
+EXPORT_SYMBOL(flow_rule_match_pppoe);
+
 struct flow_block_cb *flow_block_cb_alloc(flow_setup_cb_t *cb,
 					  void *cb_ident, void *cb_priv,
 					  void (*release)(void *cb_priv))
-- 
2.35.1

[Intel-wired-lan] [RFC PATCH net-next v3 3/4] flow_offload: Introduce flow_match_pppoe

[Marcin Szycik]

From: Wojciech Drewek <wojciech.drewek@intel.com>

Allow to offload PPPoE filters by adding flow_rule_match_pppoe.
Drivers can extract PPPoE specific fields from now on.

Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
 include/net/flow_offload.h | 6 ++++++
 net/core/flow_offload.c    | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 6484095a8c01..50283a30b97b 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -72,6 +72,10 @@ struct flow_match_ct {
 	struct flow_dissector_key_ct *key, *mask;
 };
 
+struct flow_match_pppoe {
+	struct flow_dissector_key_pppoe *key, *mask;
+};
+
 struct flow_rule;
 
 void flow_rule_match_meta(const struct flow_rule *rule,
@@ -116,6 +120,8 @@ void flow_rule_match_enc_opts(const struct flow_rule *rule,
 			      struct flow_match_enc_opts *out);
 void flow_rule_match_ct(const struct flow_rule *rule,
 			struct flow_match_ct *out);
+void flow_rule_match_pppoe(const struct flow_rule *rule,
+			   struct flow_match_pppoe *out);
 
 enum flow_action_id {
 	FLOW_ACTION_ACCEPT		= 0,
diff --git a/net/core/flow_offload.c b/net/core/flow_offload.c
index 929f6379a279..d13d293e3de2 100644
--- a/net/core/flow_offload.c
+++ b/net/core/flow_offload.c
@@ -223,6 +223,13 @@ void flow_rule_match_ct(const struct flow_rule *rule,
 }
 EXPORT_SYMBOL(flow_rule_match_ct);
 
+void flow_rule_match_pppoe(const struct flow_rule *rule,
+			   struct flow_match_pppoe *out)
+{
+	FLOW_DISSECTOR_MATCH(rule, FLOW_DISSECTOR_KEY_PPPOE, out);
+}
+EXPORT_SYMBOL(flow_rule_match_pppoe);
+
 struct flow_block_cb *flow_block_cb_alloc(flow_setup_cb_t *cb,
 					  void *cb_ident, void *cb_priv,
 					  void (*release)(void *cb_priv))
-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[RFC PATCH net-next v3 4/4] ice: Add support for PPPoE hardware offload

[Marcin Szycik]

Add support for creating PPPoE filters in switchdev mode. Add support
for parsing PPPoE and PPP-specific tc options: pppoe_sid and ppp_proto.

Example filter:
tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
    1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR

Changes in iproute2 are required to use the new fields.

ICE COMMS DDP package is required to create a filter as it contains PPPoE
profiles. Added a warning message when loaded DDP package does not contain
required profiles.

Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
will add this feature.

[0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com

Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
---
v2: wrap one long line

 drivers/net/ethernet/intel/ice/ice.h          |   1 +
 .../net/ethernet/intel/ice/ice_flex_pipe.c    |   5 +-
 .../ethernet/intel/ice/ice_protocol_type.h    |  11 ++
 drivers/net/ethernet/intel/ice/ice_switch.c   | 165 ++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_tc_lib.c   |  95 ++++++++--
 drivers/net/ethernet/intel/ice/ice_tc_lib.h   |   8 +
 6 files changed, 274 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 60453b3b8d23..0e48a930f004 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -52,6 +52,7 @@
 #include <net/udp_tunnel.h>
 #include <net/vxlan.h>
 #include <net/gtp.h>
+#include <linux/ppp_defs.h>
 #include "ice_devids.h"
 #include "ice_type.h"
 #include "ice_txrx.h"
diff --git a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
index c73cdab44f70..14fdd73a83be 100644
--- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
+++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
@@ -1964,8 +1964,11 @@ ice_get_sw_fv_list(struct ice_hw *hw, struct ice_prot_lkup_ext *lkups,
 			}
 		}
 	} while (fv);
-	if (list_empty(fv_list))
+	if (list_empty(fv_list)) {
+		dev_warn(ice_hw_to_dev(hw), "Required profiles not found in currently loaded DDP package");
 		return -EIO;
+	}
+
 	return 0;
 
 err:
diff --git a/drivers/net/ethernet/intel/ice/ice_protocol_type.h b/drivers/net/ethernet/intel/ice/ice_protocol_type.h
index 3f64300b0e14..4b7471a25551 100644
--- a/drivers/net/ethernet/intel/ice/ice_protocol_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_protocol_type.h
@@ -43,6 +43,7 @@ enum ice_protocol_type {
 	ICE_NVGRE,
 	ICE_GTP,
 	ICE_GTP_NO_PAY,
+	ICE_PPPOE,
 	ICE_VXLAN_GPE,
 	ICE_SCTP_IL,
 	ICE_PROTOCOL_LAST
@@ -107,6 +108,7 @@ enum ice_prot_id {
 #define ICE_TCP_IL_HW		49
 #define ICE_UDP_ILOS_HW		53
 #define ICE_GRE_OF_HW		64
+#define ICE_PPPOE_HW		103
 
 #define ICE_UDP_OF_HW	52 /* UDP Tunnels */
 #define ICE_META_DATA_ID_HW 255 /* this is used for tunnel type */
@@ -200,6 +202,14 @@ struct ice_udp_gtp_hdr {
 	u8 rsvrd;
 };
 
+struct ice_pppoe_hdr {
+	u8 rsrvd_ver_type;
+	u8 rsrvd_code;
+	__be16 session_id;
+	__be16 length;
+	__be16 ppp_prot_id; /* control and data only */
+};
+
 struct ice_nvgre_hdr {
 	__be16 flags;
 	__be16 protocol;
@@ -217,6 +227,7 @@ union ice_prot_hdr {
 	struct ice_udp_tnl_hdr tnl_hdr;
 	struct ice_nvgre_hdr nvgre_hdr;
 	struct ice_udp_gtp_hdr gtp_hdr;
+	struct ice_pppoe_hdr pppoe_hdr;
 };
 
 /* This is mapping table entry that maps every word within a given protocol
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 8d8f3eec79ee..99af56264f8a 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -41,6 +41,7 @@ enum {
 	ICE_PKT_INNER_TCP	= BIT(7),
 	ICE_PKT_INNER_UDP	= BIT(8),
 	ICE_PKT_GTP_NOPAY	= BIT(9),
+	ICE_PKT_PPPOE		= BIT(10),
 };
 
 struct ice_dummy_pkt_offsets {
@@ -1233,6 +1234,154 @@ ICE_DECLARE_PKT_TEMPLATE(ipv6_gtp) = {
 	0x00, 0x00,
 };
 
+ICE_DECLARE_PKT_OFFSETS(pppoe_ipv4_tcp) = {
+	{ ICE_MAC_OFOS,		0 },
+	{ ICE_ETYPE_OL,		12 },
+	{ ICE_PPPOE,		14 },
+	{ ICE_IPV4_OFOS,	22 },
+	{ ICE_TCP_IL,		42 },
+	{ ICE_PROTOCOL_LAST,	0 },
+};
+
+ICE_DECLARE_PKT_TEMPLATE(pppoe_ipv4_tcp) = {
+	0x00, 0x00, 0x00, 0x00, /* ICE_MAC_OFOS 0 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x88, 0x64,		/* ICE_ETYPE_OL 12 */
+
+	0x11, 0x00, 0x00, 0x00, /* ICE_PPPOE 14 */
+	0x00, 0x16,
+
+	0x00, 0x21,		/* PPP Link Layer 20 */
+
+	0x45, 0x00, 0x00, 0x28, /* ICE_IPV4_OFOS 22 */
+	0x00, 0x01, 0x00, 0x00,
+	0x00, 0x06, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00, 0x00, 0x00, /* ICE_TCP_IL 42 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x50, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00,		/* 2 bytes for 4 bytes alignment */
+};
+
+ICE_DECLARE_PKT_OFFSETS(pppoe_ipv4_udp) = {
+	{ ICE_MAC_OFOS,		0 },
+	{ ICE_ETYPE_OL,		12 },
+	{ ICE_PPPOE,		14 },
+	{ ICE_IPV4_OFOS,	22 },
+	{ ICE_UDP_ILOS,		42 },
+	{ ICE_PROTOCOL_LAST,	0 },
+};
+
+ICE_DECLARE_PKT_TEMPLATE(pppoe_ipv4_udp) = {
+	0x00, 0x00, 0x00, 0x00, /* ICE_MAC_OFOS 0 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x88, 0x64,		/* ICE_ETYPE_OL 12 */
+
+	0x11, 0x00, 0x00, 0x00, /* ICE_PPPOE 14 */
+	0x00, 0x16,
+
+	0x00, 0x21,		/* PPP Link Layer 20 */
+
+	0x45, 0x00, 0x00, 0x1c, /* ICE_IPV4_OFOS 22 */
+	0x00, 0x01, 0x00, 0x00,
+	0x00, 0x11, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00, 0x00, 0x00, /* ICE_UDP_ILOS 42 */
+	0x00, 0x08, 0x00, 0x00,
+
+	0x00, 0x00,		/* 2 bytes for 4 bytes alignment */
+};
+
+ICE_DECLARE_PKT_OFFSETS(pppoe_ipv6_tcp) = {
+	{ ICE_MAC_OFOS,		0 },
+	{ ICE_ETYPE_OL,		12 },
+	{ ICE_PPPOE,		14 },
+	{ ICE_IPV6_OFOS,	22 },
+	{ ICE_TCP_IL,		62 },
+	{ ICE_PROTOCOL_LAST,	0 },
+};
+
+ICE_DECLARE_PKT_TEMPLATE(pppoe_ipv6_tcp) = {
+	0x00, 0x00, 0x00, 0x00, /* ICE_MAC_OFOS 0 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x88, 0x64,		/* ICE_ETYPE_OL 12 */
+
+	0x11, 0x00, 0x00, 0x00, /* ICE_PPPOE 14 */
+	0x00, 0x2a,
+
+	0x00, 0x57,		/* PPP Link Layer 20 */
+
+	0x60, 0x00, 0x00, 0x00, /* ICE_IPV6_OFOS 22 */
+	0x00, 0x14, 0x06, 0x00, /* Next header is TCP */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00, 0x00, 0x00, /* ICE_TCP_IL 62 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x50, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00,		/* 2 bytes for 4 bytes alignment */
+};
+
+ICE_DECLARE_PKT_OFFSETS(pppoe_ipv6_udp) = {
+	{ ICE_MAC_OFOS,		0 },
+	{ ICE_ETYPE_OL,		12 },
+	{ ICE_PPPOE,		14 },
+	{ ICE_IPV6_OFOS,	22 },
+	{ ICE_UDP_ILOS,		62 },
+	{ ICE_PROTOCOL_LAST,	0 },
+};
+
+ICE_DECLARE_PKT_TEMPLATE(pppoe_ipv6_udp) = {
+	0x00, 0x00, 0x00, 0x00, /* ICE_MAC_OFOS 0 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x88, 0x64,		/* ICE_ETYPE_OL 12 */
+
+	0x11, 0x00, 0x00, 0x00, /* ICE_PPPOE 14 */
+	0x00, 0x2a,
+
+	0x00, 0x57,		/* PPP Link Layer 20 */
+
+	0x60, 0x00, 0x00, 0x00, /* ICE_IPV6_OFOS 22 */
+	0x00, 0x08, 0x11, 0x00, /* Next header UDP*/
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00, 0x00, 0x00, /* ICE_UDP_ILOS 62 */
+	0x00, 0x08, 0x00, 0x00,
+
+	0x00, 0x00,		/* 2 bytes for 4 bytes alignment */
+};
+
 static const struct ice_dummy_pkt_profile ice_dummy_pkt_profiles[] = {
 	ICE_PKT_PROFILE(ipv6_gtp, ICE_PKT_TUN_GTPU | ICE_PKT_OUTER_IPV6 |
 				  ICE_PKT_GTP_NOPAY),
@@ -1259,6 +1408,11 @@ static const struct ice_dummy_pkt_profile ice_dummy_pkt_profiles[] = {
 	ICE_PKT_PROFILE(ipv4_gtpu_ipv4_tcp, ICE_PKT_TUN_GTPU),
 	ICE_PKT_PROFILE(ipv6_gtp, ICE_PKT_TUN_GTPC | ICE_PKT_OUTER_IPV6),
 	ICE_PKT_PROFILE(ipv4_gtpu_ipv4, ICE_PKT_TUN_GTPC),
+	ICE_PKT_PROFILE(pppoe_ipv6_udp, ICE_PKT_PPPOE | ICE_PKT_OUTER_IPV6 |
+					ICE_PKT_INNER_UDP),
+	ICE_PKT_PROFILE(pppoe_ipv6_tcp, ICE_PKT_PPPOE | ICE_PKT_OUTER_IPV6),
+	ICE_PKT_PROFILE(pppoe_ipv4_udp, ICE_PKT_PPPOE | ICE_PKT_INNER_UDP),
+	ICE_PKT_PROFILE(pppoe_ipv4_tcp, ICE_PKT_PPPOE),
 	ICE_PKT_PROFILE(gre_ipv6_tcp, ICE_PKT_TUN_NVGRE | ICE_PKT_INNER_IPV6 |
 				      ICE_PKT_INNER_TCP),
 	ICE_PKT_PROFILE(gre_tcp, ICE_PKT_TUN_NVGRE | ICE_PKT_INNER_TCP),
@@ -4609,6 +4763,7 @@ static const struct ice_prot_ext_tbl_entry ice_prot_ext[ICE_PROTOCOL_LAST] = {
 	{ ICE_NVGRE,		{ 0, 2, 4, 6 } },
 	{ ICE_GTP,		{ 8, 10, 12, 14, 16, 18, 20, 22 } },
 	{ ICE_GTP_NO_PAY,	{ 8, 10, 12, 14 } },
+	{ ICE_PPPOE,		{ 0, 2, 4, 6 } },
 };
 
 static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
@@ -4629,6 +4784,7 @@ static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
 	{ ICE_NVGRE,		ICE_GRE_OF_HW },
 	{ ICE_GTP,		ICE_UDP_OF_HW },
 	{ ICE_GTP_NO_PAY,	ICE_UDP_ILOS_HW },
+	{ ICE_PPPOE,		ICE_PPPOE_HW },
 };
 
 /**
@@ -5615,6 +5771,12 @@ ice_find_dummy_packet(struct ice_adv_lkup_elem *lkups, u16 lkups_cnt,
 			match |= ICE_PKT_INNER_IPV6;
 		else if (lkups[i].type == ICE_GTP_NO_PAY)
 			match |= ICE_PKT_GTP_NOPAY;
+		else if (lkups[i].type == ICE_PPPOE) {
+			match |= ICE_PKT_PPPOE;
+			if (lkups[i].h_u.pppoe_hdr.ppp_prot_id ==
+			    htons(PPP_IPV6))
+				match |= ICE_PKT_OUTER_IPV6;
+		}
 	}
 
 	while (ret->match && (match & ret->match) != ret->match)
@@ -5707,6 +5869,9 @@ ice_fill_adv_dummy_packet(struct ice_adv_lkup_elem *lkups, u16 lkups_cnt,
 		case ICE_GTP:
 			len = sizeof(struct ice_udp_gtp_hdr);
 			break;
+		case ICE_PPPOE:
+			len = sizeof(struct ice_pppoe_hdr);
+			break;
 		default:
 			return -EINVAL;
 		}
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index b803f2ab3cc7..c14483248b73 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -50,6 +50,11 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
 	if (flags & ICE_TC_FLWR_FIELD_VLAN)
 		lkups_cnt++;
 
+	/* are PPPoE options specified? */
+	if (flags & (ICE_TC_FLWR_FIELD_PPPOE_SESSID |
+		     ICE_TC_FLWR_FIELD_PPP_PROTO))
+		lkups_cnt++;
+
 	/* are IPv[4|6] fields specified? */
 	if (flags & (ICE_TC_FLWR_FIELD_DEST_IPV4 | ICE_TC_FLWR_FIELD_SRC_IPV4 |
 		     ICE_TC_FLWR_FIELD_DEST_IPV6 | ICE_TC_FLWR_FIELD_SRC_IPV6))
@@ -317,6 +322,28 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
 		i++;
 	}
 
+	if (flags & (ICE_TC_FLWR_FIELD_PPPOE_SESSID |
+		     ICE_TC_FLWR_FIELD_PPP_PROTO)) {
+		struct ice_pppoe_hdr *vals, *masks;
+
+		vals = &list[i].h_u.pppoe_hdr;
+		masks = &list[i].m_u.pppoe_hdr;
+
+		list[i].type = ICE_PPPOE;
+
+		if (flags & ICE_TC_FLWR_FIELD_PPPOE_SESSID) {
+			vals->session_id = headers->pppoe_hdr.session_id;
+			masks->session_id = cpu_to_be16(0xFFFF);
+		}
+
+		if (flags & ICE_TC_FLWR_FIELD_PPP_PROTO) {
+			vals->ppp_prot_id = headers->pppoe_hdr.ppp_proto;
+			masks->ppp_prot_id = cpu_to_be16(0xFFFF);
+		}
+
+		i++;
+	}
+
 	/* copy L3 (IPv[4|6]: src, dest) address */
 	if (flags & (ICE_TC_FLWR_FIELD_DEST_IPV4 |
 		     ICE_TC_FLWR_FIELD_SRC_IPV4)) {
@@ -660,6 +687,33 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
 	return ret;
 }
 
+/**
+ * ice_tc_set_pppoe - Parse PPPoE fields from TC flower filter
+ * @match: Pointer to flow match structure
+ * @fltr: Pointer to filter structure
+ * @headers: Pointer to outer header fields
+ * @returns Whether ppp_proto field was used
+ */
+static bool
+ice_tc_set_pppoe(struct flow_match_pppoe *match,
+		 struct ice_tc_flower_fltr *fltr,
+		 struct ice_tc_flower_lyr_2_4_hdrs *headers)
+{
+	if (match->mask->session_id) {
+		fltr->flags |= ICE_TC_FLWR_FIELD_PPPOE_SESSID;
+		headers->pppoe_hdr.session_id =
+			cpu_to_be16(match->key->session_id);
+	}
+
+	if (match->mask->ppp_proto) {
+		fltr->flags |= ICE_TC_FLWR_FIELD_PPP_PROTO;
+		headers->pppoe_hdr.ppp_proto = match->key->ppp_proto;
+		return true;
+	}
+
+	return false;
+}
+
 /**
  * ice_tc_set_ipv4 - Parse IPv4 addresses from TC flower filter
  * @match: Pointer to flow match structure
@@ -937,6 +991,7 @@ ice_parse_cls_flower(struct net_device *filter_dev, struct ice_vsi *vsi,
 	u16 n_proto_mask = 0, n_proto_key = 0, addr_type = 0;
 	struct flow_dissector *dissector;
 	struct net_device *tunnel_dev;
+	bool ppp_proto_present = false;
 
 	dissector = rule->match.dissector;
 
@@ -954,7 +1009,8 @@ ice_parse_cls_flower(struct net_device *filter_dev, struct ice_vsi *vsi,
 	      BIT(FLOW_DISSECTOR_KEY_ENC_PORTS) |
 	      BIT(FLOW_DISSECTOR_KEY_ENC_OPTS) |
 	      BIT(FLOW_DISSECTOR_KEY_ENC_IP) |
-	      BIT(FLOW_DISSECTOR_KEY_PORTS))) {
+	      BIT(FLOW_DISSECTOR_KEY_PORTS) |
+	      BIT(FLOW_DISSECTOR_KEY_PPPOE))) {
 		NL_SET_ERR_MSG_MOD(fltr->extack, "Unsupported key used");
 		return -EOPNOTSUPP;
 	}
@@ -986,21 +1042,40 @@ ice_parse_cls_flower(struct net_device *filter_dev, struct ice_vsi *vsi,
 		fltr->tunnel_type = TNL_LAST;
 	}
 
+	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_PPPOE)) {
+		struct flow_match_pppoe match;
+
+		flow_rule_match_pppoe(rule, &match);
+		ppp_proto_present = ice_tc_set_pppoe(&match, fltr, headers);
+	}
+
 	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC)) {
 		struct flow_match_basic match;
 
 		flow_rule_match_basic(rule, &match);
 
-		n_proto_key = ntohs(match.key->n_proto);
-		n_proto_mask = ntohs(match.mask->n_proto);
-
-		if (n_proto_key == ETH_P_ALL || n_proto_key == 0 ||
-		    fltr->tunnel_type == TNL_GTPU ||
-		    fltr->tunnel_type == TNL_GTPC) {
-			n_proto_key = 0;
-			n_proto_mask = 0;
+		if (ppp_proto_present) {
+			/* When ppp_proto field is specified, it also overwrites
+			 * ethertype field with its value. E.g. if tc command is
+			 * ... protocol ppp_ses ... ppp_proto ip ...
+			 * it will result in ppp_proto=ip and n_proto_key=ip.
+			 * n_proto_key needs to be set to the proper value,
+			 * i.e. ppp_ses.
+			 */
+			n_proto_key = ETH_P_PPP_SES;
+			n_proto_mask = 0xFFFF;
 		} else {
-			fltr->flags |= ICE_TC_FLWR_FIELD_ETH_TYPE_ID;
+			n_proto_key = ntohs(match.key->n_proto);
+			n_proto_mask = ntohs(match.mask->n_proto);
+
+			if (n_proto_key == ETH_P_ALL || n_proto_key == 0 ||
+			    fltr->tunnel_type == TNL_GTPU ||
+			    fltr->tunnel_type == TNL_GTPC) {
+				n_proto_key = 0;
+				n_proto_mask = 0;
+			} else {
+				fltr->flags |= ICE_TC_FLWR_FIELD_ETH_TYPE_ID;
+			}
 		}
 
 		headers->l2_key.n_proto = cpu_to_be16(n_proto_key);
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.h b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
index e25e958f4396..efbd9c0a54ec 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
@@ -23,6 +23,8 @@
 #define ICE_TC_FLWR_FIELD_ENC_DST_MAC		BIT(16)
 #define ICE_TC_FLWR_FIELD_ETH_TYPE_ID		BIT(17)
 #define ICE_TC_FLWR_FIELD_ENC_OPTS		BIT(18)
+#define ICE_TC_FLWR_FIELD_PPPOE_SESSID		BIT(19)
+#define ICE_TC_FLWR_FIELD_PPP_PROTO		BIT(20)
 
 #define ICE_TC_FLOWER_MASK_32   0xFFFFFFFF
 
@@ -42,6 +44,11 @@ struct ice_tc_vlan_hdr {
 	u16 vlan_prio; /* Only last 3 bits valid (valid values: 0..7) */
 };
 
+struct ice_tc_pppoe_hdr {
+	__be16 session_id;
+	__be16 ppp_proto;
+};
+
 struct ice_tc_l2_hdr {
 	u8 dst_mac[ETH_ALEN];
 	u8 src_mac[ETH_ALEN];
@@ -81,6 +88,7 @@ struct ice_tc_flower_lyr_2_4_hdrs {
 	struct ice_tc_l2_hdr l2_key;
 	struct ice_tc_l2_hdr l2_mask;
 	struct ice_tc_vlan_hdr vlan_hdr;
+	struct ice_tc_pppoe_hdr pppoe_hdr;
 	/* L3 (IPv4[6]) layer fields with their mask */
 	struct ice_tc_l3_hdr l3_key;
 	struct ice_tc_l3_hdr l3_mask;
-- 
2.35.1

[Intel-wired-lan] [RFC PATCH net-next v3 4/4] ice: Add support for PPPoE hardware offload

[Marcin Szycik]

Add support for creating PPPoE filters in switchdev mode. Add support
for parsing PPPoE and PPP-specific tc options: pppoe_sid and ppp_proto.

Example filter:
tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
    1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR

Changes in iproute2 are required to use the new fields.

ICE COMMS DDP package is required to create a filter as it contains PPPoE
profiles. Added a warning message when loaded DDP package does not contain
required profiles.

Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
will add this feature.

[0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com

Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
---
v2: wrap one long line

 drivers/net/ethernet/intel/ice/ice.h          |   1 +
 .../net/ethernet/intel/ice/ice_flex_pipe.c    |   5 +-
 .../ethernet/intel/ice/ice_protocol_type.h    |  11 ++
 drivers/net/ethernet/intel/ice/ice_switch.c   | 165 ++++++++++++++++++
 drivers/net/ethernet/intel/ice/ice_tc_lib.c   |  95 ++++++++--
 drivers/net/ethernet/intel/ice/ice_tc_lib.h   |   8 +
 6 files changed, 274 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 60453b3b8d23..0e48a930f004 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -52,6 +52,7 @@
 #include <net/udp_tunnel.h>
 #include <net/vxlan.h>
 #include <net/gtp.h>
+#include <linux/ppp_defs.h>
 #include "ice_devids.h"
 #include "ice_type.h"
 #include "ice_txrx.h"
diff --git a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
index c73cdab44f70..14fdd73a83be 100644
--- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
+++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
@@ -1964,8 +1964,11 @@ ice_get_sw_fv_list(struct ice_hw *hw, struct ice_prot_lkup_ext *lkups,
 			}
 		}
 	} while (fv);
-	if (list_empty(fv_list))
+	if (list_empty(fv_list)) {
+		dev_warn(ice_hw_to_dev(hw), "Required profiles not found in currently loaded DDP package");
 		return -EIO;
+	}
+
 	return 0;
 
 err:
diff --git a/drivers/net/ethernet/intel/ice/ice_protocol_type.h b/drivers/net/ethernet/intel/ice/ice_protocol_type.h
index 3f64300b0e14..4b7471a25551 100644
--- a/drivers/net/ethernet/intel/ice/ice_protocol_type.h
+++ b/drivers/net/ethernet/intel/ice/ice_protocol_type.h
@@ -43,6 +43,7 @@ enum ice_protocol_type {
 	ICE_NVGRE,
 	ICE_GTP,
 	ICE_GTP_NO_PAY,
+	ICE_PPPOE,
 	ICE_VXLAN_GPE,
 	ICE_SCTP_IL,
 	ICE_PROTOCOL_LAST
@@ -107,6 +108,7 @@ enum ice_prot_id {
 #define ICE_TCP_IL_HW		49
 #define ICE_UDP_ILOS_HW		53
 #define ICE_GRE_OF_HW		64
+#define ICE_PPPOE_HW		103
 
 #define ICE_UDP_OF_HW	52 /* UDP Tunnels */
 #define ICE_META_DATA_ID_HW 255 /* this is used for tunnel type */
@@ -200,6 +202,14 @@ struct ice_udp_gtp_hdr {
 	u8 rsvrd;
 };
 
+struct ice_pppoe_hdr {
+	u8 rsrvd_ver_type;
+	u8 rsrvd_code;
+	__be16 session_id;
+	__be16 length;
+	__be16 ppp_prot_id; /* control and data only */
+};
+
 struct ice_nvgre_hdr {
 	__be16 flags;
 	__be16 protocol;
@@ -217,6 +227,7 @@ union ice_prot_hdr {
 	struct ice_udp_tnl_hdr tnl_hdr;
 	struct ice_nvgre_hdr nvgre_hdr;
 	struct ice_udp_gtp_hdr gtp_hdr;
+	struct ice_pppoe_hdr pppoe_hdr;
 };
 
 /* This is mapping table entry that maps every word within a given protocol
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index 8d8f3eec79ee..99af56264f8a 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -41,6 +41,7 @@ enum {
 	ICE_PKT_INNER_TCP	= BIT(7),
 	ICE_PKT_INNER_UDP	= BIT(8),
 	ICE_PKT_GTP_NOPAY	= BIT(9),
+	ICE_PKT_PPPOE		= BIT(10),
 };
 
 struct ice_dummy_pkt_offsets {
@@ -1233,6 +1234,154 @@ ICE_DECLARE_PKT_TEMPLATE(ipv6_gtp) = {
 	0x00, 0x00,
 };
 
+ICE_DECLARE_PKT_OFFSETS(pppoe_ipv4_tcp) = {
+	{ ICE_MAC_OFOS,		0 },
+	{ ICE_ETYPE_OL,		12 },
+	{ ICE_PPPOE,		14 },
+	{ ICE_IPV4_OFOS,	22 },
+	{ ICE_TCP_IL,		42 },
+	{ ICE_PROTOCOL_LAST,	0 },
+};
+
+ICE_DECLARE_PKT_TEMPLATE(pppoe_ipv4_tcp) = {
+	0x00, 0x00, 0x00, 0x00, /* ICE_MAC_OFOS 0 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x88, 0x64,		/* ICE_ETYPE_OL 12 */
+
+	0x11, 0x00, 0x00, 0x00, /* ICE_PPPOE 14 */
+	0x00, 0x16,
+
+	0x00, 0x21,		/* PPP Link Layer 20 */
+
+	0x45, 0x00, 0x00, 0x28, /* ICE_IPV4_OFOS 22 */
+	0x00, 0x01, 0x00, 0x00,
+	0x00, 0x06, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00, 0x00, 0x00, /* ICE_TCP_IL 42 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x50, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00,		/* 2 bytes for 4 bytes alignment */
+};
+
+ICE_DECLARE_PKT_OFFSETS(pppoe_ipv4_udp) = {
+	{ ICE_MAC_OFOS,		0 },
+	{ ICE_ETYPE_OL,		12 },
+	{ ICE_PPPOE,		14 },
+	{ ICE_IPV4_OFOS,	22 },
+	{ ICE_UDP_ILOS,		42 },
+	{ ICE_PROTOCOL_LAST,	0 },
+};
+
+ICE_DECLARE_PKT_TEMPLATE(pppoe_ipv4_udp) = {
+	0x00, 0x00, 0x00, 0x00, /* ICE_MAC_OFOS 0 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x88, 0x64,		/* ICE_ETYPE_OL 12 */
+
+	0x11, 0x00, 0x00, 0x00, /* ICE_PPPOE 14 */
+	0x00, 0x16,
+
+	0x00, 0x21,		/* PPP Link Layer 20 */
+
+	0x45, 0x00, 0x00, 0x1c, /* ICE_IPV4_OFOS 22 */
+	0x00, 0x01, 0x00, 0x00,
+	0x00, 0x11, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00, 0x00, 0x00, /* ICE_UDP_ILOS 42 */
+	0x00, 0x08, 0x00, 0x00,
+
+	0x00, 0x00,		/* 2 bytes for 4 bytes alignment */
+};
+
+ICE_DECLARE_PKT_OFFSETS(pppoe_ipv6_tcp) = {
+	{ ICE_MAC_OFOS,		0 },
+	{ ICE_ETYPE_OL,		12 },
+	{ ICE_PPPOE,		14 },
+	{ ICE_IPV6_OFOS,	22 },
+	{ ICE_TCP_IL,		62 },
+	{ ICE_PROTOCOL_LAST,	0 },
+};
+
+ICE_DECLARE_PKT_TEMPLATE(pppoe_ipv6_tcp) = {
+	0x00, 0x00, 0x00, 0x00, /* ICE_MAC_OFOS 0 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x88, 0x64,		/* ICE_ETYPE_OL 12 */
+
+	0x11, 0x00, 0x00, 0x00, /* ICE_PPPOE 14 */
+	0x00, 0x2a,
+
+	0x00, 0x57,		/* PPP Link Layer 20 */
+
+	0x60, 0x00, 0x00, 0x00, /* ICE_IPV6_OFOS 22 */
+	0x00, 0x14, 0x06, 0x00, /* Next header is TCP */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00, 0x00, 0x00, /* ICE_TCP_IL 62 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x50, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00,		/* 2 bytes for 4 bytes alignment */
+};
+
+ICE_DECLARE_PKT_OFFSETS(pppoe_ipv6_udp) = {
+	{ ICE_MAC_OFOS,		0 },
+	{ ICE_ETYPE_OL,		12 },
+	{ ICE_PPPOE,		14 },
+	{ ICE_IPV6_OFOS,	22 },
+	{ ICE_UDP_ILOS,		62 },
+	{ ICE_PROTOCOL_LAST,	0 },
+};
+
+ICE_DECLARE_PKT_TEMPLATE(pppoe_ipv6_udp) = {
+	0x00, 0x00, 0x00, 0x00, /* ICE_MAC_OFOS 0 */
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x88, 0x64,		/* ICE_ETYPE_OL 12 */
+
+	0x11, 0x00, 0x00, 0x00, /* ICE_PPPOE 14 */
+	0x00, 0x2a,
+
+	0x00, 0x57,		/* PPP Link Layer 20 */
+
+	0x60, 0x00, 0x00, 0x00, /* ICE_IPV6_OFOS 22 */
+	0x00, 0x08, 0x11, 0x00, /* Next header UDP*/
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00,
+
+	0x00, 0x00, 0x00, 0x00, /* ICE_UDP_ILOS 62 */
+	0x00, 0x08, 0x00, 0x00,
+
+	0x00, 0x00,		/* 2 bytes for 4 bytes alignment */
+};
+
 static const struct ice_dummy_pkt_profile ice_dummy_pkt_profiles[] = {
 	ICE_PKT_PROFILE(ipv6_gtp, ICE_PKT_TUN_GTPU | ICE_PKT_OUTER_IPV6 |
 				  ICE_PKT_GTP_NOPAY),
@@ -1259,6 +1408,11 @@ static const struct ice_dummy_pkt_profile ice_dummy_pkt_profiles[] = {
 	ICE_PKT_PROFILE(ipv4_gtpu_ipv4_tcp, ICE_PKT_TUN_GTPU),
 	ICE_PKT_PROFILE(ipv6_gtp, ICE_PKT_TUN_GTPC | ICE_PKT_OUTER_IPV6),
 	ICE_PKT_PROFILE(ipv4_gtpu_ipv4, ICE_PKT_TUN_GTPC),
+	ICE_PKT_PROFILE(pppoe_ipv6_udp, ICE_PKT_PPPOE | ICE_PKT_OUTER_IPV6 |
+					ICE_PKT_INNER_UDP),
+	ICE_PKT_PROFILE(pppoe_ipv6_tcp, ICE_PKT_PPPOE | ICE_PKT_OUTER_IPV6),
+	ICE_PKT_PROFILE(pppoe_ipv4_udp, ICE_PKT_PPPOE | ICE_PKT_INNER_UDP),
+	ICE_PKT_PROFILE(pppoe_ipv4_tcp, ICE_PKT_PPPOE),
 	ICE_PKT_PROFILE(gre_ipv6_tcp, ICE_PKT_TUN_NVGRE | ICE_PKT_INNER_IPV6 |
 				      ICE_PKT_INNER_TCP),
 	ICE_PKT_PROFILE(gre_tcp, ICE_PKT_TUN_NVGRE | ICE_PKT_INNER_TCP),
@@ -4609,6 +4763,7 @@ static const struct ice_prot_ext_tbl_entry ice_prot_ext[ICE_PROTOCOL_LAST] = {
 	{ ICE_NVGRE,		{ 0, 2, 4, 6 } },
 	{ ICE_GTP,		{ 8, 10, 12, 14, 16, 18, 20, 22 } },
 	{ ICE_GTP_NO_PAY,	{ 8, 10, 12, 14 } },
+	{ ICE_PPPOE,		{ 0, 2, 4, 6 } },
 };
 
 static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
@@ -4629,6 +4784,7 @@ static struct ice_protocol_entry ice_prot_id_tbl[ICE_PROTOCOL_LAST] = {
 	{ ICE_NVGRE,		ICE_GRE_OF_HW },
 	{ ICE_GTP,		ICE_UDP_OF_HW },
 	{ ICE_GTP_NO_PAY,	ICE_UDP_ILOS_HW },
+	{ ICE_PPPOE,		ICE_PPPOE_HW },
 };
 
 /**
@@ -5615,6 +5771,12 @@ ice_find_dummy_packet(struct ice_adv_lkup_elem *lkups, u16 lkups_cnt,
 			match |= ICE_PKT_INNER_IPV6;
 		else if (lkups[i].type == ICE_GTP_NO_PAY)
 			match |= ICE_PKT_GTP_NOPAY;
+		else if (lkups[i].type == ICE_PPPOE) {
+			match |= ICE_PKT_PPPOE;
+			if (lkups[i].h_u.pppoe_hdr.ppp_prot_id ==
+			    htons(PPP_IPV6))
+				match |= ICE_PKT_OUTER_IPV6;
+		}
 	}
 
 	while (ret->match && (match & ret->match) != ret->match)
@@ -5707,6 +5869,9 @@ ice_fill_adv_dummy_packet(struct ice_adv_lkup_elem *lkups, u16 lkups_cnt,
 		case ICE_GTP:
 			len = sizeof(struct ice_udp_gtp_hdr);
 			break;
+		case ICE_PPPOE:
+			len = sizeof(struct ice_pppoe_hdr);
+			break;
 		default:
 			return -EINVAL;
 		}
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index b803f2ab3cc7..c14483248b73 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -50,6 +50,11 @@ ice_tc_count_lkups(u32 flags, struct ice_tc_flower_lyr_2_4_hdrs *headers,
 	if (flags & ICE_TC_FLWR_FIELD_VLAN)
 		lkups_cnt++;
 
+	/* are PPPoE options specified? */
+	if (flags & (ICE_TC_FLWR_FIELD_PPPOE_SESSID |
+		     ICE_TC_FLWR_FIELD_PPP_PROTO))
+		lkups_cnt++;
+
 	/* are IPv[4|6] fields specified? */
 	if (flags & (ICE_TC_FLWR_FIELD_DEST_IPV4 | ICE_TC_FLWR_FIELD_SRC_IPV4 |
 		     ICE_TC_FLWR_FIELD_DEST_IPV6 | ICE_TC_FLWR_FIELD_SRC_IPV6))
@@ -317,6 +322,28 @@ ice_tc_fill_rules(struct ice_hw *hw, u32 flags,
 		i++;
 	}
 
+	if (flags & (ICE_TC_FLWR_FIELD_PPPOE_SESSID |
+		     ICE_TC_FLWR_FIELD_PPP_PROTO)) {
+		struct ice_pppoe_hdr *vals, *masks;
+
+		vals = &list[i].h_u.pppoe_hdr;
+		masks = &list[i].m_u.pppoe_hdr;
+
+		list[i].type = ICE_PPPOE;
+
+		if (flags & ICE_TC_FLWR_FIELD_PPPOE_SESSID) {
+			vals->session_id = headers->pppoe_hdr.session_id;
+			masks->session_id = cpu_to_be16(0xFFFF);
+		}
+
+		if (flags & ICE_TC_FLWR_FIELD_PPP_PROTO) {
+			vals->ppp_prot_id = headers->pppoe_hdr.ppp_proto;
+			masks->ppp_prot_id = cpu_to_be16(0xFFFF);
+		}
+
+		i++;
+	}
+
 	/* copy L3 (IPv[4|6]: src, dest) address */
 	if (flags & (ICE_TC_FLWR_FIELD_DEST_IPV4 |
 		     ICE_TC_FLWR_FIELD_SRC_IPV4)) {
@@ -660,6 +687,33 @@ ice_add_tc_flower_adv_fltr(struct ice_vsi *vsi,
 	return ret;
 }
 
+/**
+ * ice_tc_set_pppoe - Parse PPPoE fields from TC flower filter
+ * @match: Pointer to flow match structure
+ * @fltr: Pointer to filter structure
+ * @headers: Pointer to outer header fields
+ * @returns Whether ppp_proto field was used
+ */
+static bool
+ice_tc_set_pppoe(struct flow_match_pppoe *match,
+		 struct ice_tc_flower_fltr *fltr,
+		 struct ice_tc_flower_lyr_2_4_hdrs *headers)
+{
+	if (match->mask->session_id) {
+		fltr->flags |= ICE_TC_FLWR_FIELD_PPPOE_SESSID;
+		headers->pppoe_hdr.session_id =
+			cpu_to_be16(match->key->session_id);
+	}
+
+	if (match->mask->ppp_proto) {
+		fltr->flags |= ICE_TC_FLWR_FIELD_PPP_PROTO;
+		headers->pppoe_hdr.ppp_proto = match->key->ppp_proto;
+		return true;
+	}
+
+	return false;
+}
+
 /**
  * ice_tc_set_ipv4 - Parse IPv4 addresses from TC flower filter
  * @match: Pointer to flow match structure
@@ -937,6 +991,7 @@ ice_parse_cls_flower(struct net_device *filter_dev, struct ice_vsi *vsi,
 	u16 n_proto_mask = 0, n_proto_key = 0, addr_type = 0;
 	struct flow_dissector *dissector;
 	struct net_device *tunnel_dev;
+	bool ppp_proto_present = false;
 
 	dissector = rule->match.dissector;
 
@@ -954,7 +1009,8 @@ ice_parse_cls_flower(struct net_device *filter_dev, struct ice_vsi *vsi,
 	      BIT(FLOW_DISSECTOR_KEY_ENC_PORTS) |
 	      BIT(FLOW_DISSECTOR_KEY_ENC_OPTS) |
 	      BIT(FLOW_DISSECTOR_KEY_ENC_IP) |
-	      BIT(FLOW_DISSECTOR_KEY_PORTS))) {
+	      BIT(FLOW_DISSECTOR_KEY_PORTS) |
+	      BIT(FLOW_DISSECTOR_KEY_PPPOE))) {
 		NL_SET_ERR_MSG_MOD(fltr->extack, "Unsupported key used");
 		return -EOPNOTSUPP;
 	}
@@ -986,21 +1042,40 @@ ice_parse_cls_flower(struct net_device *filter_dev, struct ice_vsi *vsi,
 		fltr->tunnel_type = TNL_LAST;
 	}
 
+	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_PPPOE)) {
+		struct flow_match_pppoe match;
+
+		flow_rule_match_pppoe(rule, &match);
+		ppp_proto_present = ice_tc_set_pppoe(&match, fltr, headers);
+	}
+
 	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC)) {
 		struct flow_match_basic match;
 
 		flow_rule_match_basic(rule, &match);
 
-		n_proto_key = ntohs(match.key->n_proto);
-		n_proto_mask = ntohs(match.mask->n_proto);
-
-		if (n_proto_key == ETH_P_ALL || n_proto_key == 0 ||
-		    fltr->tunnel_type == TNL_GTPU ||
-		    fltr->tunnel_type == TNL_GTPC) {
-			n_proto_key = 0;
-			n_proto_mask = 0;
+		if (ppp_proto_present) {
+			/* When ppp_proto field is specified, it also overwrites
+			 * ethertype field with its value. E.g. if tc command is
+			 * ... protocol ppp_ses ... ppp_proto ip ...
+			 * it will result in ppp_proto=ip and n_proto_key=ip.
+			 * n_proto_key needs to be set to the proper value,
+			 * i.e. ppp_ses.
+			 */
+			n_proto_key = ETH_P_PPP_SES;
+			n_proto_mask = 0xFFFF;
 		} else {
-			fltr->flags |= ICE_TC_FLWR_FIELD_ETH_TYPE_ID;
+			n_proto_key = ntohs(match.key->n_proto);
+			n_proto_mask = ntohs(match.mask->n_proto);
+
+			if (n_proto_key == ETH_P_ALL || n_proto_key == 0 ||
+			    fltr->tunnel_type == TNL_GTPU ||
+			    fltr->tunnel_type == TNL_GTPC) {
+				n_proto_key = 0;
+				n_proto_mask = 0;
+			} else {
+				fltr->flags |= ICE_TC_FLWR_FIELD_ETH_TYPE_ID;
+			}
 		}
 
 		headers->l2_key.n_proto = cpu_to_be16(n_proto_key);
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.h b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
index e25e958f4396..efbd9c0a54ec 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.h
@@ -23,6 +23,8 @@
 #define ICE_TC_FLWR_FIELD_ENC_DST_MAC		BIT(16)
 #define ICE_TC_FLWR_FIELD_ETH_TYPE_ID		BIT(17)
 #define ICE_TC_FLWR_FIELD_ENC_OPTS		BIT(18)
+#define ICE_TC_FLWR_FIELD_PPPOE_SESSID		BIT(19)
+#define ICE_TC_FLWR_FIELD_PPP_PROTO		BIT(20)
 
 #define ICE_TC_FLOWER_MASK_32   0xFFFFFFFF
 
@@ -42,6 +44,11 @@ struct ice_tc_vlan_hdr {
 	u16 vlan_prio; /* Only last 3 bits valid (valid values: 0..7) */
 };
 
+struct ice_tc_pppoe_hdr {
+	__be16 session_id;
+	__be16 ppp_proto;
+};
+
 struct ice_tc_l2_hdr {
 	u8 dst_mac[ETH_ALEN];
 	u8 src_mac[ETH_ALEN];
@@ -81,6 +88,7 @@ struct ice_tc_flower_lyr_2_4_hdrs {
 	struct ice_tc_l2_hdr l2_key;
 	struct ice_tc_l2_hdr l2_mask;
 	struct ice_tc_vlan_hdr vlan_hdr;
+	struct ice_tc_pppoe_hdr pppoe_hdr;
 	/* L3 (IPv4[6]) layer fields with their mask */
 	struct ice_tc_l3_hdr l3_key;
 	struct ice_tc_l3_hdr l3_mask;
-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors

[Guillaume Nault]

On Wed, Jun 29, 2022 at 04:38:56PM +0200, Marcin Szycik wrote:
> From: Wojciech Drewek <wojciech.drewek@intel.com>
> 
> Allow to dissect PPPoE specific fields which are:
> - session ID (16 bits)
> - ppp protocol (16 bits)
> 
> The goal is to make the following TC command possible:
> 
>   # tc filter add dev ens6f0 ingress prio 1 protocol ppp_ses \
>       flower \
>         pppoe_sid 12 \
>         ppp_proto ip \
>       action drop
> 
> Note that only PPPoE Session is supported.
> 
> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> ---
> v3: revert byte order changes in is_ppp_proto_supported from previous 
>     version, add kernel-doc for is_ppp_proto_supported
> v2: use ntohs instead of htons in is_ppp_proto_supported
> 
>  include/net/flow_dissector.h | 11 ++++++++
>  net/core/flow_dissector.c    | 55 ++++++++++++++++++++++++++++++++----
>  2 files changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index a4c6057c7097..8ff40c7c3f1c 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -261,6 +261,16 @@ struct flow_dissector_key_num_of_vlans {
>  	u8 num_of_vlans;
>  };
>  
> +/**
> + * struct flow_dissector_key_pppoe:
> + * @session_id: pppoe session id
> + * @ppp_proto: ppp protocol
> + */
> +struct flow_dissector_key_pppoe {
> +	u16 session_id;
> +	__be16 ppp_proto;
> +};

Why isn't session_id __be16 too?

Also, I'm not sure I like mixing the PPPoE session ID and PPP protocol
fields in the same structure: they're part of two different protocols.
However, I can't anticipate any technical problem in doing so, and I
guess there's no easy way to let the flow dissector parse the PPP
header independently. So well, maybe we don't have choice...

>  enum flow_dissector_key_id {
>  	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
>  	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> @@ -291,6 +301,7 @@ enum flow_dissector_key_id {
>  	FLOW_DISSECTOR_KEY_CT, /* struct flow_dissector_key_ct */
>  	FLOW_DISSECTOR_KEY_HASH, /* struct flow_dissector_key_hash */
>  	FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */
> +	FLOW_DISSECTOR_KEY_PPPOE,  /* struct flow_dissector_key_pppoe */
>  
>  	FLOW_DISSECTOR_KEY_MAX,
>  };
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 6aee04f75e3e..42393af477a2 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -895,6 +895,39 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
>  	return result == BPF_OK;
>  }
>  
> +/**
> + * is_ppp_proto_supported - checks if inner PPP protocol should be dissected
> + * @proto: protocol type (PPP proto field)
> + */
> +static bool is_ppp_proto_supported(__be16 proto)
> +{
> +	switch (proto) {
> +	case htons(PPP_AT):
> +	case htons(PPP_IPX):
> +	case htons(PPP_VJC_COMP):
> +	case htons(PPP_VJC_UNCOMP):
> +	case htons(PPP_MP):
> +	case htons(PPP_COMPFRAG):
> +	case htons(PPP_COMP):
> +	case htons(PPP_MPLS_UC):
> +	case htons(PPP_MPLS_MC):
> +	case htons(PPP_IPCP):
> +	case htons(PPP_ATCP):
> +	case htons(PPP_IPXCP):
> +	case htons(PPP_IPV6CP):
> +	case htons(PPP_CCPFRAG):
> +	case htons(PPP_MPLSCP):
> +	case htons(PPP_LCP):
> +	case htons(PPP_PAP):
> +	case htons(PPP_LQR):
> +	case htons(PPP_CHAP):
> +	case htons(PPP_CBCP):
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  /**
>   * __skb_flow_dissect - extract the flow_keys struct and return it
>   * @net: associated network namespace, derived from @skb if NULL
> @@ -1221,19 +1254,29 @@ bool __skb_flow_dissect(const struct net *net,
>  		}
>  
>  		nhoff += PPPOE_SES_HLEN;
> -		switch (hdr->proto) {
> -		case htons(PPP_IP):
> +		if (hdr->proto == htons(PPP_IP)) {
>  			proto = htons(ETH_P_IP);
>  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> -			break;
> -		case htons(PPP_IPV6):
> +		} else if (hdr->proto == htons(PPP_IPV6)) {
>  			proto = htons(ETH_P_IPV6);
>  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> -			break;

1)
Looks like you could easily handle MPLS too. Did you skip it on
purpose? (not enough users to justify writing and maintaining
the code?).

I don't mean MPLS has to be supported; I'd just like to know if it was
considered.

2)
Also this whole test is a bit weak: the version, type and code fields
must have precise values for the PPPoE Session packet to be valid.
If either version or type is different than 1, then the packet
advertises a new version of the protocol that we don't know how to parse
(or most probably the packet was forged or corrupted). A non-zero code
is also invalid.

I know the code was already like this before your patch, but it's
probably better to fix it before implementing hardware offload.

3)
Finally, the PPP protocol could be compressed and stored in 1 byte
instead of 2. This case wasn't handled before your patch, but I think
that should be fixed too before implementing hardware offload.

> -		default:
> +		} else if (is_ppp_proto_supported(hdr->proto)) {
> +			fdret = FLOW_DISSECT_RET_OUT_GOOD;
> +		} else {
>  			fdret = FLOW_DISSECT_RET_OUT_BAD;
>  			break;
>  		}
> +
> +		if (dissector_uses_key(flow_dissector,
> +				       FLOW_DISSECTOR_KEY_PPPOE)) {
> +			struct flow_dissector_key_pppoe *key_pppoe;
> +
> +			key_pppoe = skb_flow_dissector_target(flow_dissector,
> +							      FLOW_DISSECTOR_KEY_PPPOE,
> +							      target_container);
> +			key_pppoe->session_id = ntohs(hdr->hdr.sid);
> +			key_pppoe->ppp_proto = hdr->proto;
> +		}
>  		break;
>  	}
>  	case htons(ETH_P_TIPC): {
> -- 
> 2.35.1
> 

Re: [Intel-wired-lan] [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors

[Guillaume Nault]

On Wed, Jun 29, 2022 at 04:38:56PM +0200, Marcin Szycik wrote:
> From: Wojciech Drewek <wojciech.drewek@intel.com>
> 
> Allow to dissect PPPoE specific fields which are:
> - session ID (16 bits)
> - ppp protocol (16 bits)
> 
> The goal is to make the following TC command possible:
> 
>   # tc filter add dev ens6f0 ingress prio 1 protocol ppp_ses \
>       flower \
>         pppoe_sid 12 \
>         ppp_proto ip \
>       action drop
> 
> Note that only PPPoE Session is supported.
> 
> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> ---
> v3: revert byte order changes in is_ppp_proto_supported from previous 
>     version, add kernel-doc for is_ppp_proto_supported
> v2: use ntohs instead of htons in is_ppp_proto_supported
> 
>  include/net/flow_dissector.h | 11 ++++++++
>  net/core/flow_dissector.c    | 55 ++++++++++++++++++++++++++++++++----
>  2 files changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index a4c6057c7097..8ff40c7c3f1c 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -261,6 +261,16 @@ struct flow_dissector_key_num_of_vlans {
>  	u8 num_of_vlans;
>  };
>  
> +/**
> + * struct flow_dissector_key_pppoe:
> + * @session_id: pppoe session id
> + * @ppp_proto: ppp protocol
> + */
> +struct flow_dissector_key_pppoe {
> +	u16 session_id;
> +	__be16 ppp_proto;
> +};

Why isn't session_id __be16 too?

Also, I'm not sure I like mixing the PPPoE session ID and PPP protocol
fields in the same structure: they're part of two different protocols.
However, I can't anticipate any technical problem in doing so, and I
guess there's no easy way to let the flow dissector parse the PPP
header independently. So well, maybe we don't have choice...

>  enum flow_dissector_key_id {
>  	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
>  	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> @@ -291,6 +301,7 @@ enum flow_dissector_key_id {
>  	FLOW_DISSECTOR_KEY_CT, /* struct flow_dissector_key_ct */
>  	FLOW_DISSECTOR_KEY_HASH, /* struct flow_dissector_key_hash */
>  	FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */
> +	FLOW_DISSECTOR_KEY_PPPOE,  /* struct flow_dissector_key_pppoe */
>  
>  	FLOW_DISSECTOR_KEY_MAX,
>  };
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index 6aee04f75e3e..42393af477a2 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -895,6 +895,39 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
>  	return result == BPF_OK;
>  }
>  
> +/**
> + * is_ppp_proto_supported - checks if inner PPP protocol should be dissected
> + * @proto: protocol type (PPP proto field)
> + */
> +static bool is_ppp_proto_supported(__be16 proto)
> +{
> +	switch (proto) {
> +	case htons(PPP_AT):
> +	case htons(PPP_IPX):
> +	case htons(PPP_VJC_COMP):
> +	case htons(PPP_VJC_UNCOMP):
> +	case htons(PPP_MP):
> +	case htons(PPP_COMPFRAG):
> +	case htons(PPP_COMP):
> +	case htons(PPP_MPLS_UC):
> +	case htons(PPP_MPLS_MC):
> +	case htons(PPP_IPCP):
> +	case htons(PPP_ATCP):
> +	case htons(PPP_IPXCP):
> +	case htons(PPP_IPV6CP):
> +	case htons(PPP_CCPFRAG):
> +	case htons(PPP_MPLSCP):
> +	case htons(PPP_LCP):
> +	case htons(PPP_PAP):
> +	case htons(PPP_LQR):
> +	case htons(PPP_CHAP):
> +	case htons(PPP_CBCP):
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  /**
>   * __skb_flow_dissect - extract the flow_keys struct and return it
>   * @net: associated network namespace, derived from @skb if NULL
> @@ -1221,19 +1254,29 @@ bool __skb_flow_dissect(const struct net *net,
>  		}
>  
>  		nhoff += PPPOE_SES_HLEN;
> -		switch (hdr->proto) {
> -		case htons(PPP_IP):
> +		if (hdr->proto == htons(PPP_IP)) {
>  			proto = htons(ETH_P_IP);
>  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> -			break;
> -		case htons(PPP_IPV6):
> +		} else if (hdr->proto == htons(PPP_IPV6)) {
>  			proto = htons(ETH_P_IPV6);
>  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> -			break;

1)
Looks like you could easily handle MPLS too. Did you skip it on
purpose? (not enough users to justify writing and maintaining
the code?).

I don't mean MPLS has to be supported; I'd just like to know if it was
considered.

2)
Also this whole test is a bit weak: the version, type and code fields
must have precise values for the PPPoE Session packet to be valid.
If either version or type is different than 1, then the packet
advertises a new version of the protocol that we don't know how to parse
(or most probably the packet was forged or corrupted). A non-zero code
is also invalid.

I know the code was already like this before your patch, but it's
probably better to fix it before implementing hardware offload.

3)
Finally, the PPP protocol could be compressed and stored in 1 byte
instead of 2. This case wasn't handled before your patch, but I think
that should be fixed too before implementing hardware offload.

> -		default:
> +		} else if (is_ppp_proto_supported(hdr->proto)) {
> +			fdret = FLOW_DISSECT_RET_OUT_GOOD;
> +		} else {
>  			fdret = FLOW_DISSECT_RET_OUT_BAD;
>  			break;
>  		}
> +
> +		if (dissector_uses_key(flow_dissector,
> +				       FLOW_DISSECTOR_KEY_PPPOE)) {
> +			struct flow_dissector_key_pppoe *key_pppoe;
> +
> +			key_pppoe = skb_flow_dissector_target(flow_dissector,
> +							      FLOW_DISSECTOR_KEY_PPPOE,
> +							      target_container);
> +			key_pppoe->session_id = ntohs(hdr->hdr.sid);
> +			key_pppoe->ppp_proto = hdr->proto;
> +		}
>  		break;
>  	}
>  	case htons(ETH_P_TIPC): {
> -- 
> 2.35.1
> 

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [RFC PATCH net-next v3 2/4] net/sched: flower: Add PPPoE filter

[Guillaume Nault]

On Wed, Jun 29, 2022 at 04:38:57PM +0200, Marcin Szycik wrote:
> From: Wojciech Drewek <wojciech.drewek@intel.com>
> 
> Add support for PPPoE specific fields for tc-flower.
> Those fields can be provided only when protocol was set
> to ETH_P_PPP_SES. Defines, dump, load and set are being done here.
> 
> Overwrite basic.n_proto only in case of PPP_IP and PPP_IPV6,
> otherwise leave it as ETH_P_PPP_SES.
> 
> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> ---
>  include/uapi/linux/pkt_cls.h |  3 +++
>  net/sched/cls_flower.c       | 46 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 9a2ee1e39fad..a67dcd8294c9 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -589,6 +589,9 @@ enum {
>  
>  	TCA_FLOWER_KEY_NUM_OF_VLANS,    /* u8 */
>  
> +	TCA_FLOWER_KEY_PPPOE_SID,	/* u16 */
> +	TCA_FLOWER_KEY_PPP_PROTO,	/* be16 */

Same as for patch 1, should PPPOE_SID be a be16?

>  	__TCA_FLOWER_MAX,
>  };
>  
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index dcca70144dff..f6a0bb87f869 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -16,6 +16,7 @@
>  #include <linux/in6.h>
>  #include <linux/ip.h>
>  #include <linux/mpls.h>
> +#include <linux/ppp_defs.h>
>  
>  #include <net/sch_generic.h>
>  #include <net/pkt_cls.h>
> @@ -73,6 +74,7 @@ struct fl_flow_key {
>  	struct flow_dissector_key_ct ct;
>  	struct flow_dissector_key_hash hash;
>  	struct flow_dissector_key_num_of_vlans num_of_vlans;
> +	struct flow_dissector_key_pppoe pppoe;
>  } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */
>  
>  struct fl_flow_mask_range {
> @@ -714,6 +716,8 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
>  	[TCA_FLOWER_KEY_HASH]		= { .type = NLA_U32 },
>  	[TCA_FLOWER_KEY_HASH_MASK]	= { .type = NLA_U32 },
>  	[TCA_FLOWER_KEY_NUM_OF_VLANS]	= { .type = NLA_U8 },
> +	[TCA_FLOWER_KEY_PPPOE_SID]	= { .type = NLA_U16 },
> +	[TCA_FLOWER_KEY_PPP_PROTO]	= { .type = NLA_U16 },
>  
>  };
>  
> @@ -1041,6 +1045,32 @@ static void fl_set_key_vlan(struct nlattr **tb,
>  	}
>  }
>  
> +static void fl_set_key_pppoe(struct nlattr **tb,
> +			     struct flow_dissector_key_pppoe *key_val,
> +			     struct flow_dissector_key_pppoe *key_mask,
> +			     struct fl_flow_key *key,
> +			     struct fl_flow_key *mask)
> +{
> +	if (tb[TCA_FLOWER_KEY_PPPOE_SID]) {
> +		key_val->session_id =
> +			nla_get_u16(tb[TCA_FLOWER_KEY_PPPOE_SID]);
> +		key_mask->session_id = 0xffff;
> +	}
> +	if (tb[TCA_FLOWER_KEY_PPP_PROTO]) {
> +		key_val->ppp_proto =
> +			nla_get_be16(tb[TCA_FLOWER_KEY_PPP_PROTO]);
> +		key_mask->ppp_proto = cpu_to_be16(~0);
> +
> +		if (key_val->ppp_proto == htons(PPP_IP)) {
> +			key->basic.n_proto = htons(ETH_P_IP);
> +			mask->basic.n_proto = cpu_to_be16(~0);
> +		} else if (key_val->ppp_proto == htons(PPP_IPV6)) {
> +			key->basic.n_proto = htons(ETH_P_IPV6);
> +			mask->basic.n_proto = cpu_to_be16(~0);
> +		}

Again, is the lack of MPLS support voluntary?

> +	}
> +}
> +
>  static void fl_set_key_flag(u32 flower_key, u32 flower_mask,
>  			    u32 *dissector_key, u32 *dissector_mask,
>  			    u32 flower_flag_bit, u32 dissector_flag_bit)
> @@ -1651,6 +1681,9 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
>  		}
>  	}
>  
> +	if (key->basic.n_proto == htons(ETH_P_PPP_SES))
> +		fl_set_key_pppoe(tb, &key->pppoe, &mask->pppoe, key, mask);
> +
>  	if (key->basic.n_proto == htons(ETH_P_IP) ||
>  	    key->basic.n_proto == htons(ETH_P_IPV6)) {
>  		fl_set_key_val(tb, &key->basic.ip_proto, TCA_FLOWER_KEY_IP_PROTO,
> @@ -1923,6 +1956,8 @@ static void fl_init_dissector(struct flow_dissector *dissector,
>  			     FLOW_DISSECTOR_KEY_HASH, hash);
>  	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
>  			     FLOW_DISSECTOR_KEY_NUM_OF_VLANS, num_of_vlans);
> +	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
> +			     FLOW_DISSECTOR_KEY_PPPOE, pppoe);
>  
>  	skb_flow_dissector_init(dissector, keys, cnt);
>  }
> @@ -3051,6 +3086,17 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net,
>  	    fl_dump_key_ip(skb, false, &key->ip, &mask->ip)))
>  		goto nla_put_failure;
>  
> +	if (mask->pppoe.session_id) {
> +		if (nla_put_u16(skb, TCA_FLOWER_KEY_PPPOE_SID,
> +				key->pppoe.session_id))
> +			goto nla_put_failure;
> +	}
> +	if (mask->basic.n_proto && mask->pppoe.ppp_proto) {
> +		if (nla_put_be16(skb, TCA_FLOWER_KEY_PPP_PROTO,
> +				 key->pppoe.ppp_proto))
> +			goto nla_put_failure;
> +	}
> +
>  	if (key->control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS &&
>  	    (fl_dump_key_val(skb, &key->ipv4.src, TCA_FLOWER_KEY_IPV4_SRC,
>  			     &mask->ipv4.src, TCA_FLOWER_KEY_IPV4_SRC_MASK,
> -- 
> 2.35.1
> 

Re: [Intel-wired-lan] [RFC PATCH net-next v3 2/4] net/sched: flower: Add PPPoE filter

[Guillaume Nault]

On Wed, Jun 29, 2022 at 04:38:57PM +0200, Marcin Szycik wrote:
> From: Wojciech Drewek <wojciech.drewek@intel.com>
> 
> Add support for PPPoE specific fields for tc-flower.
> Those fields can be provided only when protocol was set
> to ETH_P_PPP_SES. Defines, dump, load and set are being done here.
> 
> Overwrite basic.n_proto only in case of PPP_IP and PPP_IPV6,
> otherwise leave it as ETH_P_PPP_SES.
> 
> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> ---
>  include/uapi/linux/pkt_cls.h |  3 +++
>  net/sched/cls_flower.c       | 46 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
> index 9a2ee1e39fad..a67dcd8294c9 100644
> --- a/include/uapi/linux/pkt_cls.h
> +++ b/include/uapi/linux/pkt_cls.h
> @@ -589,6 +589,9 @@ enum {
>  
>  	TCA_FLOWER_KEY_NUM_OF_VLANS,    /* u8 */
>  
> +	TCA_FLOWER_KEY_PPPOE_SID,	/* u16 */
> +	TCA_FLOWER_KEY_PPP_PROTO,	/* be16 */

Same as for patch 1, should PPPOE_SID be a be16?

>  	__TCA_FLOWER_MAX,
>  };
>  
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index dcca70144dff..f6a0bb87f869 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -16,6 +16,7 @@
>  #include <linux/in6.h>
>  #include <linux/ip.h>
>  #include <linux/mpls.h>
> +#include <linux/ppp_defs.h>
>  
>  #include <net/sch_generic.h>
>  #include <net/pkt_cls.h>
> @@ -73,6 +74,7 @@ struct fl_flow_key {
>  	struct flow_dissector_key_ct ct;
>  	struct flow_dissector_key_hash hash;
>  	struct flow_dissector_key_num_of_vlans num_of_vlans;
> +	struct flow_dissector_key_pppoe pppoe;
>  } __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */
>  
>  struct fl_flow_mask_range {
> @@ -714,6 +716,8 @@ static const struct nla_policy fl_policy[TCA_FLOWER_MAX + 1] = {
>  	[TCA_FLOWER_KEY_HASH]		= { .type = NLA_U32 },
>  	[TCA_FLOWER_KEY_HASH_MASK]	= { .type = NLA_U32 },
>  	[TCA_FLOWER_KEY_NUM_OF_VLANS]	= { .type = NLA_U8 },
> +	[TCA_FLOWER_KEY_PPPOE_SID]	= { .type = NLA_U16 },
> +	[TCA_FLOWER_KEY_PPP_PROTO]	= { .type = NLA_U16 },
>  
>  };
>  
> @@ -1041,6 +1045,32 @@ static void fl_set_key_vlan(struct nlattr **tb,
>  	}
>  }
>  
> +static void fl_set_key_pppoe(struct nlattr **tb,
> +			     struct flow_dissector_key_pppoe *key_val,
> +			     struct flow_dissector_key_pppoe *key_mask,
> +			     struct fl_flow_key *key,
> +			     struct fl_flow_key *mask)
> +{
> +	if (tb[TCA_FLOWER_KEY_PPPOE_SID]) {
> +		key_val->session_id =
> +			nla_get_u16(tb[TCA_FLOWER_KEY_PPPOE_SID]);
> +		key_mask->session_id = 0xffff;
> +	}
> +	if (tb[TCA_FLOWER_KEY_PPP_PROTO]) {
> +		key_val->ppp_proto =
> +			nla_get_be16(tb[TCA_FLOWER_KEY_PPP_PROTO]);
> +		key_mask->ppp_proto = cpu_to_be16(~0);
> +
> +		if (key_val->ppp_proto == htons(PPP_IP)) {
> +			key->basic.n_proto = htons(ETH_P_IP);
> +			mask->basic.n_proto = cpu_to_be16(~0);
> +		} else if (key_val->ppp_proto == htons(PPP_IPV6)) {
> +			key->basic.n_proto = htons(ETH_P_IPV6);
> +			mask->basic.n_proto = cpu_to_be16(~0);
> +		}

Again, is the lack of MPLS support voluntary?

> +	}
> +}
> +
>  static void fl_set_key_flag(u32 flower_key, u32 flower_mask,
>  			    u32 *dissector_key, u32 *dissector_mask,
>  			    u32 flower_flag_bit, u32 dissector_flag_bit)
> @@ -1651,6 +1681,9 @@ static int fl_set_key(struct net *net, struct nlattr **tb,
>  		}
>  	}
>  
> +	if (key->basic.n_proto == htons(ETH_P_PPP_SES))
> +		fl_set_key_pppoe(tb, &key->pppoe, &mask->pppoe, key, mask);
> +
>  	if (key->basic.n_proto == htons(ETH_P_IP) ||
>  	    key->basic.n_proto == htons(ETH_P_IPV6)) {
>  		fl_set_key_val(tb, &key->basic.ip_proto, TCA_FLOWER_KEY_IP_PROTO,
> @@ -1923,6 +1956,8 @@ static void fl_init_dissector(struct flow_dissector *dissector,
>  			     FLOW_DISSECTOR_KEY_HASH, hash);
>  	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
>  			     FLOW_DISSECTOR_KEY_NUM_OF_VLANS, num_of_vlans);
> +	FL_KEY_SET_IF_MASKED(mask, keys, cnt,
> +			     FLOW_DISSECTOR_KEY_PPPOE, pppoe);
>  
>  	skb_flow_dissector_init(dissector, keys, cnt);
>  }
> @@ -3051,6 +3086,17 @@ static int fl_dump_key(struct sk_buff *skb, struct net *net,
>  	    fl_dump_key_ip(skb, false, &key->ip, &mask->ip)))
>  		goto nla_put_failure;
>  
> +	if (mask->pppoe.session_id) {
> +		if (nla_put_u16(skb, TCA_FLOWER_KEY_PPPOE_SID,
> +				key->pppoe.session_id))
> +			goto nla_put_failure;
> +	}
> +	if (mask->basic.n_proto && mask->pppoe.ppp_proto) {
> +		if (nla_put_be16(skb, TCA_FLOWER_KEY_PPP_PROTO,
> +				 key->pppoe.ppp_proto))
> +			goto nla_put_failure;
> +	}
> +
>  	if (key->control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS &&
>  	    (fl_dump_key_val(skb, &key->ipv4.src, TCA_FLOWER_KEY_IPV4_SRC,
>  			     &mask->ipv4.src, TCA_FLOWER_KEY_IPV4_SRC_MASK,
> -- 
> 2.35.1
> 

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [RFC PATCH net-next v3 4/4] ice: Add support for PPPoE hardware offload

[Guillaume Nault]

On Wed, Jun 29, 2022 at 04:38:59PM +0200, Marcin Szycik wrote:
> Add support for creating PPPoE filters in switchdev mode. Add support
> for parsing PPPoE and PPP-specific tc options: pppoe_sid and ppp_proto.
> 
> Example filter:
> tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
>     1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR
> 
> Changes in iproute2 are required to use the new fields.
> 
> ICE COMMS DDP package is required to create a filter as it contains PPPoE
> profiles. Added a warning message when loaded DDP package does not contain
> required profiles.
> 
> Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
> will add this feature.
> 
> [0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com

Out of curiosity, can ice direct PPPoE Session packets to different
queues with RSS (based on the session ID)?

Re: [Intel-wired-lan] [RFC PATCH net-next v3 4/4] ice: Add support for PPPoE hardware offload

[Guillaume Nault]

On Wed, Jun 29, 2022 at 04:38:59PM +0200, Marcin Szycik wrote:
> Add support for creating PPPoE filters in switchdev mode. Add support
> for parsing PPPoE and PPP-specific tc options: pppoe_sid and ppp_proto.
> 
> Example filter:
> tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
>     1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR
> 
> Changes in iproute2 are required to use the new fields.
> 
> ICE COMMS DDP package is required to create a filter as it contains PPPoE
> profiles. Added a warning message when loaded DDP package does not contain
> required profiles.
> 
> Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
> will add this feature.
> 
> [0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com

Out of curiosity, can ice direct PPPoE Session packets to different
queues with RSS (based on the session ID)?

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

RE: [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors

[Drewek, Wojciech]

Hi Guillaume,

Thanks for the review!

> -----Original Message-----
> From: Guillaume Nault <gnault@redhat.com>
> Sent: piątek, 1 lipca 2022 01:10
> To: Marcin Szycik <marcin.szycik@linux.intel.com>
> Cc: netdev@vger.kernel.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
> xiyou.wangcong@gmail.com; Brandeburg, Jesse <jesse.brandeburg@intel.com>; gustavoars@kernel.org;
> baowen.zheng@corigine.com; boris.sukholitko@broadcom.com; edumazet@google.com; kuba@kernel.org; jhs@mojatatu.com;
> jiri@resnulli.us; kurt@linutronix.de; pablo@netfilter.org; pabeni@redhat.com; paulb@nvidia.com; simon.horman@corigine.com;
> komachi.yoshiki@gmail.com; zhangkaiheb@126.com; intel-wired-lan@lists.osuosl.org; michal.swiatkowski@linux.intel.com; Drewek,
> Wojciech <wojciech.drewek@intel.com>; Lobakin, Alexandr <alexandr.lobakin@intel.com>; mostrows@earthlink.net;
> paulus@samba.org
> Subject: Re: [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors
> 
> On Wed, Jun 29, 2022 at 04:38:56PM +0200, Marcin Szycik wrote:
> > From: Wojciech Drewek <wojciech.drewek@intel.com>
> >
> > Allow to dissect PPPoE specific fields which are:
> > - session ID (16 bits)
> > - ppp protocol (16 bits)
> >
> > The goal is to make the following TC command possible:
> >
> >   # tc filter add dev ens6f0 ingress prio 1 protocol ppp_ses \
> >       flower \
> >         pppoe_sid 12 \
> >         ppp_proto ip \
> >       action drop
> >
> > Note that only PPPoE Session is supported.
> >
> > Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> > ---
> > v3: revert byte order changes in is_ppp_proto_supported from previous
> >     version, add kernel-doc for is_ppp_proto_supported
> > v2: use ntohs instead of htons in is_ppp_proto_supported
> >
> >  include/net/flow_dissector.h | 11 ++++++++
> >  net/core/flow_dissector.c    | 55 ++++++++++++++++++++++++++++++++----
> >  2 files changed, 60 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> > index a4c6057c7097..8ff40c7c3f1c 100644
> > --- a/include/net/flow_dissector.h
> > +++ b/include/net/flow_dissector.h
> > @@ -261,6 +261,16 @@ struct flow_dissector_key_num_of_vlans {
> >  	u8 num_of_vlans;
> >  };
> >
> > +/**
> > + * struct flow_dissector_key_pppoe:
> > + * @session_id: pppoe session id
> > + * @ppp_proto: ppp protocol
> > + */
> > +struct flow_dissector_key_pppoe {
> > +	u16 session_id;
> > +	__be16 ppp_proto;
> > +};
> 
> Why isn't session_id __be16 too?

I've got general impression that storing protocols values
in big endian is a standard through out the code and other values like vlan_id
don't have to be stored in big endian, but maybe it's just my illusion :)
I can change that in v3.

> 
> Also, I'm not sure I like mixing the PPPoE session ID and PPP protocol
> fields in the same structure: they're part of two different protocols.
> However, I can't anticipate any technical problem in doing so, and I
> guess there's no easy way to let the flow dissector parse the PPP
> header independently. So well, maybe we don't have choice...

We are not planning to match on other fields from PPP protocol so
separate structure just for it is not needed I guess.

> 
> >  enum flow_dissector_key_id {
> >  	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
> >  	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> > @@ -291,6 +301,7 @@ enum flow_dissector_key_id {
> >  	FLOW_DISSECTOR_KEY_CT, /* struct flow_dissector_key_ct */
> >  	FLOW_DISSECTOR_KEY_HASH, /* struct flow_dissector_key_hash */
> >  	FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */
> > +	FLOW_DISSECTOR_KEY_PPPOE,  /* struct flow_dissector_key_pppoe */
> >
> >  	FLOW_DISSECTOR_KEY_MAX,
> >  };
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 6aee04f75e3e..42393af477a2 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -895,6 +895,39 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
> >  	return result == BPF_OK;
> >  }
> >
> > +/**
> > + * is_ppp_proto_supported - checks if inner PPP protocol should be dissected
> > + * @proto: protocol type (PPP proto field)
> > + */
> > +static bool is_ppp_proto_supported(__be16 proto)
> > +{
> > +	switch (proto) {
> > +	case htons(PPP_AT):
> > +	case htons(PPP_IPX):
> > +	case htons(PPP_VJC_COMP):
> > +	case htons(PPP_VJC_UNCOMP):
> > +	case htons(PPP_MP):
> > +	case htons(PPP_COMPFRAG):
> > +	case htons(PPP_COMP):
> > +	case htons(PPP_MPLS_UC):
> > +	case htons(PPP_MPLS_MC):
> > +	case htons(PPP_IPCP):
> > +	case htons(PPP_ATCP):
> > +	case htons(PPP_IPXCP):
> > +	case htons(PPP_IPV6CP):
> > +	case htons(PPP_CCPFRAG):
> > +	case htons(PPP_MPLSCP):
> > +	case htons(PPP_LCP):
> > +	case htons(PPP_PAP):
> > +	case htons(PPP_LQR):
> > +	case htons(PPP_CHAP):
> > +	case htons(PPP_CBCP):
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  /**
> >   * __skb_flow_dissect - extract the flow_keys struct and return it
> >   * @net: associated network namespace, derived from @skb if NULL
> > @@ -1221,19 +1254,29 @@ bool __skb_flow_dissect(const struct net *net,
> >  		}
> >
> >  		nhoff += PPPOE_SES_HLEN;
> > -		switch (hdr->proto) {
> > -		case htons(PPP_IP):
> > +		if (hdr->proto == htons(PPP_IP)) {
> >  			proto = htons(ETH_P_IP);
> >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > -			break;
> > -		case htons(PPP_IPV6):
> > +		} else if (hdr->proto == htons(PPP_IPV6)) {
> >  			proto = htons(ETH_P_IPV6);
> >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > -			break;
> 
> 1)
> Looks like you could easily handle MPLS too. Did you skip it on
> purpose? (not enough users to justify writing and maintaining
> the code?).
> 
> I don't mean MPLS has to be supported; I'd just like to know if it was
> considered.

Yes, exactly as you write, not enough users, but I can see thet MPLS should 
be easy to implement so I'll include it in the next version.

> 
> 2)
> Also this whole test is a bit weak: the version, type and code fields
> must have precise values for the PPPoE Session packet to be valid.
> If either version or type is different than 1, then the packet
> advertises a new version of the protocol that we don't know how to parse
> (or most probably the packet was forged or corrupted). A non-zero code
> is also invalid.
> 
> I know the code was already like this before your patch, but it's
> probably better to fix it before implementing hardware offload.

Sure, I'll add packet validation in next version.

> 
> 3)
> Finally, the PPP protocol could be compressed and stored in 1 byte
> instead of 2. This case wasn't handled before your patch, but I think
> that should be fixed too before implementing hardware offload.

We faced that issue but we couldn't find out what indicates
when ppp protocol is stored in 1 byte instead of 2.

> 
> > -		default:
> > +		} else if (is_ppp_proto_supported(hdr->proto)) {
> > +			fdret = FLOW_DISSECT_RET_OUT_GOOD;
> > +		} else {
> >  			fdret = FLOW_DISSECT_RET_OUT_BAD;
> >  			break;
> >  		}
> > +
> > +		if (dissector_uses_key(flow_dissector,
> > +				       FLOW_DISSECTOR_KEY_PPPOE)) {
> > +			struct flow_dissector_key_pppoe *key_pppoe;
> > +
> > +			key_pppoe = skb_flow_dissector_target(flow_dissector,
> > +							      FLOW_DISSECTOR_KEY_PPPOE,
> > +							      target_container);
> > +			key_pppoe->session_id = ntohs(hdr->hdr.sid);
> > +			key_pppoe->ppp_proto = hdr->proto;
> > +		}
> >  		break;
> >  	}
> >  	case htons(ETH_P_TIPC): {
> > --
> > 2.35.1
> >

Re: [Intel-wired-lan] [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors

[Drewek, Wojciech]

Hi Guillaume,

Thanks for the review!

> -----Original Message-----
> From: Guillaume Nault <gnault@redhat.com>
> Sent: piątek, 1 lipca 2022 01:10
> To: Marcin Szycik <marcin.szycik@linux.intel.com>
> Cc: netdev@vger.kernel.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net;
> xiyou.wangcong@gmail.com; Brandeburg, Jesse <jesse.brandeburg@intel.com>; gustavoars@kernel.org;
> baowen.zheng@corigine.com; boris.sukholitko@broadcom.com; edumazet@google.com; kuba@kernel.org; jhs@mojatatu.com;
> jiri@resnulli.us; kurt@linutronix.de; pablo@netfilter.org; pabeni@redhat.com; paulb@nvidia.com; simon.horman@corigine.com;
> komachi.yoshiki@gmail.com; zhangkaiheb@126.com; intel-wired-lan@lists.osuosl.org; michal.swiatkowski@linux.intel.com; Drewek,
> Wojciech <wojciech.drewek@intel.com>; Lobakin, Alexandr <alexandr.lobakin@intel.com>; mostrows@earthlink.net;
> paulus@samba.org
> Subject: Re: [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors
> 
> On Wed, Jun 29, 2022 at 04:38:56PM +0200, Marcin Szycik wrote:
> > From: Wojciech Drewek <wojciech.drewek@intel.com>
> >
> > Allow to dissect PPPoE specific fields which are:
> > - session ID (16 bits)
> > - ppp protocol (16 bits)
> >
> > The goal is to make the following TC command possible:
> >
> >   # tc filter add dev ens6f0 ingress prio 1 protocol ppp_ses \
> >       flower \
> >         pppoe_sid 12 \
> >         ppp_proto ip \
> >       action drop
> >
> > Note that only PPPoE Session is supported.
> >
> > Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> > ---
> > v3: revert byte order changes in is_ppp_proto_supported from previous
> >     version, add kernel-doc for is_ppp_proto_supported
> > v2: use ntohs instead of htons in is_ppp_proto_supported
> >
> >  include/net/flow_dissector.h | 11 ++++++++
> >  net/core/flow_dissector.c    | 55 ++++++++++++++++++++++++++++++++----
> >  2 files changed, 60 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> > index a4c6057c7097..8ff40c7c3f1c 100644
> > --- a/include/net/flow_dissector.h
> > +++ b/include/net/flow_dissector.h
> > @@ -261,6 +261,16 @@ struct flow_dissector_key_num_of_vlans {
> >  	u8 num_of_vlans;
> >  };
> >
> > +/**
> > + * struct flow_dissector_key_pppoe:
> > + * @session_id: pppoe session id
> > + * @ppp_proto: ppp protocol
> > + */
> > +struct flow_dissector_key_pppoe {
> > +	u16 session_id;
> > +	__be16 ppp_proto;
> > +};
> 
> Why isn't session_id __be16 too?

I've got general impression that storing protocols values
in big endian is a standard through out the code and other values like vlan_id
don't have to be stored in big endian, but maybe it's just my illusion :)
I can change that in v3.

> 
> Also, I'm not sure I like mixing the PPPoE session ID and PPP protocol
> fields in the same structure: they're part of two different protocols.
> However, I can't anticipate any technical problem in doing so, and I
> guess there's no easy way to let the flow dissector parse the PPP
> header independently. So well, maybe we don't have choice...

We are not planning to match on other fields from PPP protocol so
separate structure just for it is not needed I guess.

> 
> >  enum flow_dissector_key_id {
> >  	FLOW_DISSECTOR_KEY_CONTROL, /* struct flow_dissector_key_control */
> >  	FLOW_DISSECTOR_KEY_BASIC, /* struct flow_dissector_key_basic */
> > @@ -291,6 +301,7 @@ enum flow_dissector_key_id {
> >  	FLOW_DISSECTOR_KEY_CT, /* struct flow_dissector_key_ct */
> >  	FLOW_DISSECTOR_KEY_HASH, /* struct flow_dissector_key_hash */
> >  	FLOW_DISSECTOR_KEY_NUM_OF_VLANS, /* struct flow_dissector_key_num_of_vlans */
> > +	FLOW_DISSECTOR_KEY_PPPOE,  /* struct flow_dissector_key_pppoe */
> >
> >  	FLOW_DISSECTOR_KEY_MAX,
> >  };
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index 6aee04f75e3e..42393af477a2 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -895,6 +895,39 @@ bool bpf_flow_dissect(struct bpf_prog *prog, struct bpf_flow_dissector *ctx,
> >  	return result == BPF_OK;
> >  }
> >
> > +/**
> > + * is_ppp_proto_supported - checks if inner PPP protocol should be dissected
> > + * @proto: protocol type (PPP proto field)
> > + */
> > +static bool is_ppp_proto_supported(__be16 proto)
> > +{
> > +	switch (proto) {
> > +	case htons(PPP_AT):
> > +	case htons(PPP_IPX):
> > +	case htons(PPP_VJC_COMP):
> > +	case htons(PPP_VJC_UNCOMP):
> > +	case htons(PPP_MP):
> > +	case htons(PPP_COMPFRAG):
> > +	case htons(PPP_COMP):
> > +	case htons(PPP_MPLS_UC):
> > +	case htons(PPP_MPLS_MC):
> > +	case htons(PPP_IPCP):
> > +	case htons(PPP_ATCP):
> > +	case htons(PPP_IPXCP):
> > +	case htons(PPP_IPV6CP):
> > +	case htons(PPP_CCPFRAG):
> > +	case htons(PPP_MPLSCP):
> > +	case htons(PPP_LCP):
> > +	case htons(PPP_PAP):
> > +	case htons(PPP_LQR):
> > +	case htons(PPP_CHAP):
> > +	case htons(PPP_CBCP):
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  /**
> >   * __skb_flow_dissect - extract the flow_keys struct and return it
> >   * @net: associated network namespace, derived from @skb if NULL
> > @@ -1221,19 +1254,29 @@ bool __skb_flow_dissect(const struct net *net,
> >  		}
> >
> >  		nhoff += PPPOE_SES_HLEN;
> > -		switch (hdr->proto) {
> > -		case htons(PPP_IP):
> > +		if (hdr->proto == htons(PPP_IP)) {
> >  			proto = htons(ETH_P_IP);
> >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > -			break;
> > -		case htons(PPP_IPV6):
> > +		} else if (hdr->proto == htons(PPP_IPV6)) {
> >  			proto = htons(ETH_P_IPV6);
> >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > -			break;
> 
> 1)
> Looks like you could easily handle MPLS too. Did you skip it on
> purpose? (not enough users to justify writing and maintaining
> the code?).
> 
> I don't mean MPLS has to be supported; I'd just like to know if it was
> considered.

Yes, exactly as you write, not enough users, but I can see thet MPLS should 
be easy to implement so I'll include it in the next version.

> 
> 2)
> Also this whole test is a bit weak: the version, type and code fields
> must have precise values for the PPPoE Session packet to be valid.
> If either version or type is different than 1, then the packet
> advertises a new version of the protocol that we don't know how to parse
> (or most probably the packet was forged or corrupted). A non-zero code
> is also invalid.
> 
> I know the code was already like this before your patch, but it's
> probably better to fix it before implementing hardware offload.

Sure, I'll add packet validation in next version.

> 
> 3)
> Finally, the PPP protocol could be compressed and stored in 1 byte
> instead of 2. This case wasn't handled before your patch, but I think
> that should be fixed too before implementing hardware offload.

We faced that issue but we couldn't find out what indicates
when ppp protocol is stored in 1 byte instead of 2.

> 
> > -		default:
> > +		} else if (is_ppp_proto_supported(hdr->proto)) {
> > +			fdret = FLOW_DISSECT_RET_OUT_GOOD;
> > +		} else {
> >  			fdret = FLOW_DISSECT_RET_OUT_BAD;
> >  			break;
> >  		}
> > +
> > +		if (dissector_uses_key(flow_dissector,
> > +				       FLOW_DISSECTOR_KEY_PPPOE)) {
> > +			struct flow_dissector_key_pppoe *key_pppoe;
> > +
> > +			key_pppoe = skb_flow_dissector_target(flow_dissector,
> > +							      FLOW_DISSECTOR_KEY_PPPOE,
> > +							      target_container);
> > +			key_pppoe->session_id = ntohs(hdr->hdr.sid);
> > +			key_pppoe->ppp_proto = hdr->proto;
> > +		}
> >  		break;
> >  	}
> >  	case htons(ETH_P_TIPC): {
> > --
> > 2.35.1
> >

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors

[Guillaume Nault]

On Fri, Jul 01, 2022 at 10:53:51AM +0000, Drewek, Wojciech wrote:
> > > +/**
> > > + * struct flow_dissector_key_pppoe:
> > > + * @session_id: pppoe session id
> > > + * @ppp_proto: ppp protocol
> > > + */
> > > +struct flow_dissector_key_pppoe {
> > > +	u16 session_id;
> > > +	__be16 ppp_proto;
> > > +};
> > 
> > Why isn't session_id __be16 too?
> 
> I've got general impression that storing protocols values
> in big endian is a standard through out the code and other values like vlan_id
> don't have to be stored in big endian, but maybe it's just my illusion :)
> I can change that in v3.

I don't know of any written rule, but looking at other keys, every
protocol field is stored with the endianess used on the wire. Only
metadata are stored in host byte order. For flow_dissector_key_vlan,
vlan_id is a bit special since it's only 12 bits long, but other vlan
fields are stored in big endian (vlan_tci is __be16 for example). And
vlan ids are special for another reason too: they're also metadata
stored in skbuffs because of vlan hardware offload.

But PPPoE Session Id is clearly read from the packet header, where it's
stored in network byte order.

> > Also, I'm not sure I like mixing the PPPoE session ID and PPP protocol
> > fields in the same structure: they're part of two different protocols.
> > However, I can't anticipate any technical problem in doing so, and I
> > guess there's no easy way to let the flow dissector parse the PPP
> > header independently. So well, maybe we don't have choice...
> 
> We are not planning to match on other fields from PPP protocol so
> separate structure just for it is not needed I guess.

FTR, I believe it's okay to take this shortcut but for different
reasons:

 * When transported over PPPoE, PPP frames are not supposed to have
   address and control fields. Therefore, in this case, the PPP header
   is limitted to the protocol field, so the dissector key would never
   have to be extended.

 * It's unlikely enough that we'd ever have any other protocol
   transporting PPP frames to implement in the flow dissector.
   Therefore, independent PPP dissection code probably won't be needed
   (even if one wants to add support for L2TP or PPTP in the flow
   dissector, that probably should be done with tunnel metadata, like
   VXLAN).

 * We have gotos for jumping to "network" or "transport" header dissection
   (proto_again and ip_proto_again), but no place to restart at the "link"
   header and no way to tell what type of link layer header we're
   requesting to parse (Ethernet or PPP).

For all these reasons, I believe your approach is an acceptable
shortcut. But I don't buy the "let's limit the flow dissector to what
we plan to support in ice" argument.

> > > @@ -1221,19 +1254,29 @@ bool __skb_flow_dissect(const struct net *net,
> > >  		}
> > >
> > >  		nhoff += PPPOE_SES_HLEN;
> > > -		switch (hdr->proto) {
> > > -		case htons(PPP_IP):
> > > +		if (hdr->proto == htons(PPP_IP)) {
> > >  			proto = htons(ETH_P_IP);
> > >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > > -			break;
> > > -		case htons(PPP_IPV6):
> > > +		} else if (hdr->proto == htons(PPP_IPV6)) {
> > >  			proto = htons(ETH_P_IPV6);
> > >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > > -			break;
> > 
> > 1)
> > Looks like you could easily handle MPLS too. Did you skip it on
> > purpose? (not enough users to justify writing and maintaining
> > the code?).
> > 
> > I don't mean MPLS has to be supported; I'd just like to know if it was
> > considered.
> 
> Yes, exactly as you write, not enough users, but I can see thet MPLS should
> be easy to implement so I'll include it in the next version.

Okay.

> > 2)
> > Also this whole test is a bit weak: the version, type and code fields
> > must have precise values for the PPPoE Session packet to be valid.
> > If either version or type is different than 1, then the packet
> > advertises a new version of the protocol that we don't know how to parse
> > (or most probably the packet was forged or corrupted). A non-zero code
> > is also invalid.
> > 
> > I know the code was already like this before your patch, but it's
> > probably better to fix it before implementing hardware offload.
> 
> Sure, I'll add packet validation in next version.

Great!

> > 3)
> > Finally, the PPP protocol could be compressed and stored in 1 byte
> > instead of 2. This case wasn't handled before your patch, but I think
> > that should be fixed too before implementing hardware offload.
> 
> We faced that issue but we couldn't find out what indicates
> when ppp protocol is stored in 1 byte instead of 2.

That depends on the least significant bit of the first byte. If it's 0
then the next byte is also part of the protocol field. If it's one,
the protocol is "compressed" (that is the high order 0x00 byte has been
stripped and we're left with only the least significant byte).

This is explained more formally in RFC 1661 section 2 (PPP Encapsulation):
  https://datatracker.ietf.org/doc/html/rfc1661#section-2

and section 6.5 (Protocol-Field-Compression (PFC)):
  https://datatracker.ietf.org/doc/html/rfc1661#section-6.5

There should be no reason to use this old PPP feature with PPPoE, but
it's still valid (even though it breaks IP header alignment).

Re: [Intel-wired-lan] [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors

[Guillaume Nault]

On Fri, Jul 01, 2022 at 10:53:51AM +0000, Drewek, Wojciech wrote:
> > > +/**
> > > + * struct flow_dissector_key_pppoe:
> > > + * @session_id: pppoe session id
> > > + * @ppp_proto: ppp protocol
> > > + */
> > > +struct flow_dissector_key_pppoe {
> > > +	u16 session_id;
> > > +	__be16 ppp_proto;
> > > +};
> > 
> > Why isn't session_id __be16 too?
> 
> I've got general impression that storing protocols values
> in big endian is a standard through out the code and other values like vlan_id
> don't have to be stored in big endian, but maybe it's just my illusion :)
> I can change that in v3.

I don't know of any written rule, but looking at other keys, every
protocol field is stored with the endianess used on the wire. Only
metadata are stored in host byte order. For flow_dissector_key_vlan,
vlan_id is a bit special since it's only 12 bits long, but other vlan
fields are stored in big endian (vlan_tci is __be16 for example). And
vlan ids are special for another reason too: they're also metadata
stored in skbuffs because of vlan hardware offload.

But PPPoE Session Id is clearly read from the packet header, where it's
stored in network byte order.

> > Also, I'm not sure I like mixing the PPPoE session ID and PPP protocol
> > fields in the same structure: they're part of two different protocols.
> > However, I can't anticipate any technical problem in doing so, and I
> > guess there's no easy way to let the flow dissector parse the PPP
> > header independently. So well, maybe we don't have choice...
> 
> We are not planning to match on other fields from PPP protocol so
> separate structure just for it is not needed I guess.

FTR, I believe it's okay to take this shortcut but for different
reasons:

 * When transported over PPPoE, PPP frames are not supposed to have
   address and control fields. Therefore, in this case, the PPP header
   is limitted to the protocol field, so the dissector key would never
   have to be extended.

 * It's unlikely enough that we'd ever have any other protocol
   transporting PPP frames to implement in the flow dissector.
   Therefore, independent PPP dissection code probably won't be needed
   (even if one wants to add support for L2TP or PPTP in the flow
   dissector, that probably should be done with tunnel metadata, like
   VXLAN).

 * We have gotos for jumping to "network" or "transport" header dissection
   (proto_again and ip_proto_again), but no place to restart at the "link"
   header and no way to tell what type of link layer header we're
   requesting to parse (Ethernet or PPP).

For all these reasons, I believe your approach is an acceptable
shortcut. But I don't buy the "let's limit the flow dissector to what
we plan to support in ice" argument.

> > > @@ -1221,19 +1254,29 @@ bool __skb_flow_dissect(const struct net *net,
> > >  		}
> > >
> > >  		nhoff += PPPOE_SES_HLEN;
> > > -		switch (hdr->proto) {
> > > -		case htons(PPP_IP):
> > > +		if (hdr->proto == htons(PPP_IP)) {
> > >  			proto = htons(ETH_P_IP);
> > >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > > -			break;
> > > -		case htons(PPP_IPV6):
> > > +		} else if (hdr->proto == htons(PPP_IPV6)) {
> > >  			proto = htons(ETH_P_IPV6);
> > >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > > -			break;
> > 
> > 1)
> > Looks like you could easily handle MPLS too. Did you skip it on
> > purpose? (not enough users to justify writing and maintaining
> > the code?).
> > 
> > I don't mean MPLS has to be supported; I'd just like to know if it was
> > considered.
> 
> Yes, exactly as you write, not enough users, but I can see thet MPLS should
> be easy to implement so I'll include it in the next version.

Okay.

> > 2)
> > Also this whole test is a bit weak: the version, type and code fields
> > must have precise values for the PPPoE Session packet to be valid.
> > If either version or type is different than 1, then the packet
> > advertises a new version of the protocol that we don't know how to parse
> > (or most probably the packet was forged or corrupted). A non-zero code
> > is also invalid.
> > 
> > I know the code was already like this before your patch, but it's
> > probably better to fix it before implementing hardware offload.
> 
> Sure, I'll add packet validation in next version.

Great!

> > 3)
> > Finally, the PPP protocol could be compressed and stored in 1 byte
> > instead of 2. This case wasn't handled before your patch, but I think
> > that should be fixed too before implementing hardware offload.
> 
> We faced that issue but we couldn't find out what indicates
> when ppp protocol is stored in 1 byte instead of 2.

That depends on the least significant bit of the first byte. If it's 0
then the next byte is also part of the protocol field. If it's one,
the protocol is "compressed" (that is the high order 0x00 byte has been
stripped and we're left with only the least significant byte).

This is explained more formally in RFC 1661 section 2 (PPP Encapsulation):
  https://datatracker.ietf.org/doc/html/rfc1661#section-2

and section 6.5 (Protocol-Field-Compression (PFC)):
  https://datatracker.ietf.org/doc/html/rfc1661#section-6.5

There should be no reason to use this old PPP feature with PPPoE, but
it's still valid (even though it breaks IP header alignment).

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

RE: [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors

[Drewek, Wojciech]

> -----Original Message-----
> From: Guillaume Nault <gnault@redhat.com>
> Sent: piątek, 1 lipca 2022 14:42
> To: Drewek, Wojciech <wojciech.drewek@intel.com>
> Cc: Marcin Szycik <marcin.szycik@linux.intel.com>; netdev@vger.kernel.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> davem@davemloft.net; xiyou.wangcong@gmail.com; Brandeburg, Jesse <jesse.brandeburg@intel.com>; gustavoars@kernel.org;
> baowen.zheng@corigine.com; boris.sukholitko@broadcom.com; edumazet@google.com; kuba@kernel.org; jhs@mojatatu.com;
> jiri@resnulli.us; kurt@linutronix.de; pablo@netfilter.org; pabeni@redhat.com; paulb@nvidia.com; simon.horman@corigine.com;
> komachi.yoshiki@gmail.com; zhangkaiheb@126.com; intel-wired-lan@lists.osuosl.org; michal.swiatkowski@linux.intel.com; Lobakin,
> Alexandr <alexandr.lobakin@intel.com>; mostrows@earthlink.net; paulus@samba.org
> Subject: Re: [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors
> 
> On Fri, Jul 01, 2022 at 10:53:51AM +0000, Drewek, Wojciech wrote:
> > > > +/**
> > > > + * struct flow_dissector_key_pppoe:
> > > > + * @session_id: pppoe session id
> > > > + * @ppp_proto: ppp protocol
> > > > + */
> > > > +struct flow_dissector_key_pppoe {
> > > > +	u16 session_id;
> > > > +	__be16 ppp_proto;
> > > > +};
> > >
> > > Why isn't session_id __be16 too?
> >
> > I've got general impression that storing protocols values
> > in big endian is a standard through out the code and other values like vlan_id
> > don't have to be stored in big endian, but maybe it's just my illusion :)
> > I can change that in v3.
> 
> I don't know of any written rule, but looking at other keys, every
> protocol field is stored with the endianess used on the wire. Only
> metadata are stored in host byte order. For flow_dissector_key_vlan,
> vlan_id is a bit special since it's only 12 bits long, but other vlan
> fields are stored in big endian (vlan_tci is __be16 for example). And
> vlan ids are special for another reason too: they're also metadata
> stored in skbuffs because of vlan hardware offload.
> 
> But PPPoE Session Id is clearly read from the packet header, where it's
> stored in network byte order.

Thanks for explanation! We'll use __be16 for session_id since now.

> 
> > > Also, I'm not sure I like mixing the PPPoE session ID and PPP protocol
> > > fields in the same structure: they're part of two different protocols.
> > > However, I can't anticipate any technical problem in doing so, and I
> > > guess there's no easy way to let the flow dissector parse the PPP
> > > header independently. So well, maybe we don't have choice...
> >
> > We are not planning to match on other fields from PPP protocol so
> > separate structure just for it is not needed I guess.
> 
> FTR, I believe it's okay to take this shortcut but for different
> reasons:
> 
>  * When transported over PPPoE, PPP frames are not supposed to have
>    address and control fields. Therefore, in this case, the PPP header
>    is limitted to the protocol field, so the dissector key would never
>    have to be extended.
> 
>  * It's unlikely enough that we'd ever have any other protocol
>    transporting PPP frames to implement in the flow dissector.
>    Therefore, independent PPP dissection code probably won't be needed
>    (even if one wants to add support for L2TP or PPTP in the flow
>    dissector, that probably should be done with tunnel metadata, like
>    VXLAN).
> 
>  * We have gotos for jumping to "network" or "transport" header dissection
>    (proto_again and ip_proto_again), but no place to restart at the "link"
>    header and no way to tell what type of link layer header we're
>    requesting to parse (Ethernet or PPP).
> 
> For all these reasons, I believe your approach is an acceptable
> shortcut. But I don't buy the "let's limit the flow dissector to what
> we plan to support in ice" argument.

Again thanks for explanation. Sorry, I didn't want to suggest that flow_dissector
should be designed based only on our needs. We are happy to change our
implementation if requested.

We will stay with the current approach if this is the conclusion.

> 
> > > > @@ -1221,19 +1254,29 @@ bool __skb_flow_dissect(const struct net *net,
> > > >  		}
> > > >
> > > >  		nhoff += PPPOE_SES_HLEN;
> > > > -		switch (hdr->proto) {
> > > > -		case htons(PPP_IP):
> > > > +		if (hdr->proto == htons(PPP_IP)) {
> > > >  			proto = htons(ETH_P_IP);
> > > >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > > > -			break;
> > > > -		case htons(PPP_IPV6):
> > > > +		} else if (hdr->proto == htons(PPP_IPV6)) {
> > > >  			proto = htons(ETH_P_IPV6);
> > > >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > > > -			break;
> > >
> > > 1)
> > > Looks like you could easily handle MPLS too. Did you skip it on
> > > purpose? (not enough users to justify writing and maintaining
> > > the code?).
> > >
> > > I don't mean MPLS has to be supported; I'd just like to know if it was
> > > considered.
> >
> > Yes, exactly as you write, not enough users, but I can see thet MPLS should
> > be easy to implement so I'll include it in the next version.
> 
> Okay.
> 
> > > 2)
> > > Also this whole test is a bit weak: the version, type and code fields
> > > must have precise values for the PPPoE Session packet to be valid.
> > > If either version or type is different than 1, then the packet
> > > advertises a new version of the protocol that we don't know how to parse
> > > (or most probably the packet was forged or corrupted). A non-zero code
> > > is also invalid.
> > >
> > > I know the code was already like this before your patch, but it's
> > > probably better to fix it before implementing hardware offload.
> >
> > Sure, I'll add packet validation in next version.
> 
> Great!
> 
> > > 3)
> > > Finally, the PPP protocol could be compressed and stored in 1 byte
> > > instead of 2. This case wasn't handled before your patch, but I think
> > > that should be fixed too before implementing hardware offload.
> >
> > We faced that issue but we couldn't find out what indicates
> > when ppp protocol is stored in 1 byte instead of 2.
> 
> That depends on the least significant bit of the first byte. If it's 0
> then the next byte is also part of the protocol field. If it's one,
> the protocol is "compressed" (that is the high order 0x00 byte has been
> stripped and we're left with only the least significant byte).
> 
> This is explained more formally in RFC 1661 section 2 (PPP Encapsulation):
>   https://datatracker.ietf.org/doc/html/rfc1661#section-2
> 
> and section 6.5 (Protocol-Field-Compression (PFC)):
>   https://datatracker.ietf.org/doc/html/rfc1661#section-6.5
> 
> There should be no reason to use this old PPP feature with PPPoE, but
> it's still valid (even though it breaks IP header alignment).

Thanks for explanation! From the next version we will support both options.

Re: [Intel-wired-lan] [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors

[Drewek, Wojciech]

> -----Original Message-----
> From: Guillaume Nault <gnault@redhat.com>
> Sent: piątek, 1 lipca 2022 14:42
> To: Drewek, Wojciech <wojciech.drewek@intel.com>
> Cc: Marcin Szycik <marcin.szycik@linux.intel.com>; netdev@vger.kernel.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> davem@davemloft.net; xiyou.wangcong@gmail.com; Brandeburg, Jesse <jesse.brandeburg@intel.com>; gustavoars@kernel.org;
> baowen.zheng@corigine.com; boris.sukholitko@broadcom.com; edumazet@google.com; kuba@kernel.org; jhs@mojatatu.com;
> jiri@resnulli.us; kurt@linutronix.de; pablo@netfilter.org; pabeni@redhat.com; paulb@nvidia.com; simon.horman@corigine.com;
> komachi.yoshiki@gmail.com; zhangkaiheb@126.com; intel-wired-lan@lists.osuosl.org; michal.swiatkowski@linux.intel.com; Lobakin,
> Alexandr <alexandr.lobakin@intel.com>; mostrows@earthlink.net; paulus@samba.org
> Subject: Re: [RFC PATCH net-next v3 1/4] flow_dissector: Add PPPoE dissectors
> 
> On Fri, Jul 01, 2022 at 10:53:51AM +0000, Drewek, Wojciech wrote:
> > > > +/**
> > > > + * struct flow_dissector_key_pppoe:
> > > > + * @session_id: pppoe session id
> > > > + * @ppp_proto: ppp protocol
> > > > + */
> > > > +struct flow_dissector_key_pppoe {
> > > > +	u16 session_id;
> > > > +	__be16 ppp_proto;
> > > > +};
> > >
> > > Why isn't session_id __be16 too?
> >
> > I've got general impression that storing protocols values
> > in big endian is a standard through out the code and other values like vlan_id
> > don't have to be stored in big endian, but maybe it's just my illusion :)
> > I can change that in v3.
> 
> I don't know of any written rule, but looking at other keys, every
> protocol field is stored with the endianess used on the wire. Only
> metadata are stored in host byte order. For flow_dissector_key_vlan,
> vlan_id is a bit special since it's only 12 bits long, but other vlan
> fields are stored in big endian (vlan_tci is __be16 for example). And
> vlan ids are special for another reason too: they're also metadata
> stored in skbuffs because of vlan hardware offload.
> 
> But PPPoE Session Id is clearly read from the packet header, where it's
> stored in network byte order.

Thanks for explanation! We'll use __be16 for session_id since now.

> 
> > > Also, I'm not sure I like mixing the PPPoE session ID and PPP protocol
> > > fields in the same structure: they're part of two different protocols.
> > > However, I can't anticipate any technical problem in doing so, and I
> > > guess there's no easy way to let the flow dissector parse the PPP
> > > header independently. So well, maybe we don't have choice...
> >
> > We are not planning to match on other fields from PPP protocol so
> > separate structure just for it is not needed I guess.
> 
> FTR, I believe it's okay to take this shortcut but for different
> reasons:
> 
>  * When transported over PPPoE, PPP frames are not supposed to have
>    address and control fields. Therefore, in this case, the PPP header
>    is limitted to the protocol field, so the dissector key would never
>    have to be extended.
> 
>  * It's unlikely enough that we'd ever have any other protocol
>    transporting PPP frames to implement in the flow dissector.
>    Therefore, independent PPP dissection code probably won't be needed
>    (even if one wants to add support for L2TP or PPTP in the flow
>    dissector, that probably should be done with tunnel metadata, like
>    VXLAN).
> 
>  * We have gotos for jumping to "network" or "transport" header dissection
>    (proto_again and ip_proto_again), but no place to restart at the "link"
>    header and no way to tell what type of link layer header we're
>    requesting to parse (Ethernet or PPP).
> 
> For all these reasons, I believe your approach is an acceptable
> shortcut. But I don't buy the "let's limit the flow dissector to what
> we plan to support in ice" argument.

Again thanks for explanation. Sorry, I didn't want to suggest that flow_dissector
should be designed based only on our needs. We are happy to change our
implementation if requested.

We will stay with the current approach if this is the conclusion.

> 
> > > > @@ -1221,19 +1254,29 @@ bool __skb_flow_dissect(const struct net *net,
> > > >  		}
> > > >
> > > >  		nhoff += PPPOE_SES_HLEN;
> > > > -		switch (hdr->proto) {
> > > > -		case htons(PPP_IP):
> > > > +		if (hdr->proto == htons(PPP_IP)) {
> > > >  			proto = htons(ETH_P_IP);
> > > >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > > > -			break;
> > > > -		case htons(PPP_IPV6):
> > > > +		} else if (hdr->proto == htons(PPP_IPV6)) {
> > > >  			proto = htons(ETH_P_IPV6);
> > > >  			fdret = FLOW_DISSECT_RET_PROTO_AGAIN;
> > > > -			break;
> > >
> > > 1)
> > > Looks like you could easily handle MPLS too. Did you skip it on
> > > purpose? (not enough users to justify writing and maintaining
> > > the code?).
> > >
> > > I don't mean MPLS has to be supported; I'd just like to know if it was
> > > considered.
> >
> > Yes, exactly as you write, not enough users, but I can see thet MPLS should
> > be easy to implement so I'll include it in the next version.
> 
> Okay.
> 
> > > 2)
> > > Also this whole test is a bit weak: the version, type and code fields
> > > must have precise values for the PPPoE Session packet to be valid.
> > > If either version or type is different than 1, then the packet
> > > advertises a new version of the protocol that we don't know how to parse
> > > (or most probably the packet was forged or corrupted). A non-zero code
> > > is also invalid.
> > >
> > > I know the code was already like this before your patch, but it's
> > > probably better to fix it before implementing hardware offload.
> >
> > Sure, I'll add packet validation in next version.
> 
> Great!
> 
> > > 3)
> > > Finally, the PPP protocol could be compressed and stored in 1 byte
> > > instead of 2. This case wasn't handled before your patch, but I think
> > > that should be fixed too before implementing hardware offload.
> >
> > We faced that issue but we couldn't find out what indicates
> > when ppp protocol is stored in 1 byte instead of 2.
> 
> That depends on the least significant bit of the first byte. If it's 0
> then the next byte is also part of the protocol field. If it's one,
> the protocol is "compressed" (that is the high order 0x00 byte has been
> stripped and we're left with only the least significant byte).
> 
> This is explained more formally in RFC 1661 section 2 (PPP Encapsulation):
>   https://datatracker.ietf.org/doc/html/rfc1661#section-2
> 
> and section 6.5 (Protocol-Field-Compression (PFC)):
>   https://datatracker.ietf.org/doc/html/rfc1661#section-6.5
> 
> There should be no reason to use this old PPP feature with PPPoE, but
> it's still valid (even though it breaks IP header alignment).

Thanks for explanation! From the next version we will support both options.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [RFC PATCH net-next v3 4/4] ice: Add support for PPPoE hardware offload

[Marcin Szycik]

On 01-Jul-22 01:12, Guillaume Nault wrote:
> On Wed, Jun 29, 2022 at 04:38:59PM +0200, Marcin Szycik wrote:
>> Add support for creating PPPoE filters in switchdev mode. Add support
>> for parsing PPPoE and PPP-specific tc options: pppoe_sid and ppp_proto.
>>
>> Example filter:
>> tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
>>     1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR
>>
>> Changes in iproute2 are required to use the new fields.
>>
>> ICE COMMS DDP package is required to create a filter as it contains PPPoE
>> profiles. Added a warning message when loaded DDP package does not contain
>> required profiles.
>>
>> Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
>> will add this feature.
>>
>> [0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com
> 
> Out of curiosity, can ice direct PPPoE Session packets to different
> queues with RSS (based on the session ID)?

Hardware should support it, but I'm not sure if it's possible with the current driver and how to configure it. I'll try to find out.

Re: [Intel-wired-lan] [RFC PATCH net-next v3 4/4] ice: Add support for PPPoE hardware offload

[Marcin Szycik]

On 01-Jul-22 01:12, Guillaume Nault wrote:
> On Wed, Jun 29, 2022 at 04:38:59PM +0200, Marcin Szycik wrote:
>> Add support for creating PPPoE filters in switchdev mode. Add support
>> for parsing PPPoE and PPP-specific tc options: pppoe_sid and ppp_proto.
>>
>> Example filter:
>> tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
>>     1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR
>>
>> Changes in iproute2 are required to use the new fields.
>>
>> ICE COMMS DDP package is required to create a filter as it contains PPPoE
>> profiles. Added a warning message when loaded DDP package does not contain
>> required profiles.
>>
>> Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
>> will add this feature.
>>
>> [0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com
> 
> Out of curiosity, can ice direct PPPoE Session packets to different
> queues with RSS (based on the session ID)?

Hardware should support it, but I'm not sure if it's possible with the current driver and how to configure it. I'll try to find out.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [RFC PATCH net-next v3 4/4] ice: Add support for PPPoE hardware offload

[Marcin Szycik]

On 01-Jul-22 18:12, Marcin Szycik wrote:
> 
> 
> On 01-Jul-22 01:12, Guillaume Nault wrote:
>> On Wed, Jun 29, 2022 at 04:38:59PM +0200, Marcin Szycik wrote:
>>> Add support for creating PPPoE filters in switchdev mode. Add support
>>> for parsing PPPoE and PPP-specific tc options: pppoe_sid and ppp_proto.
>>>
>>> Example filter:
>>> tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
>>>     1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR
>>>
>>> Changes in iproute2 are required to use the new fields.
>>>
>>> ICE COMMS DDP package is required to create a filter as it contains PPPoE
>>> profiles. Added a warning message when loaded DDP package does not contain
>>> required profiles.
>>>
>>> Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
>>> will add this feature.
>>>
>>> [0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com
>>
>> Out of curiosity, can ice direct PPPoE Session packets to different
>> queues with RSS (based on the session ID)?
> 
> Hardware should support it, but I'm not sure if it's possible with the current driver and how to configure it. I'll try to find out.

From what I understand, currently it's not possible to configure RSS for PPPoE session id, because ethtool does not support PPPoE.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [RFC PATCH net-next v3 4/4] ice: Add support for PPPoE hardware offload

[Marcin Szycik]

On 01-Jul-22 18:12, Marcin Szycik wrote:
> 
> 
> On 01-Jul-22 01:12, Guillaume Nault wrote:
>> On Wed, Jun 29, 2022 at 04:38:59PM +0200, Marcin Szycik wrote:
>>> Add support for creating PPPoE filters in switchdev mode. Add support
>>> for parsing PPPoE and PPP-specific tc options: pppoe_sid and ppp_proto.
>>>
>>> Example filter:
>>> tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
>>>     1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR
>>>
>>> Changes in iproute2 are required to use the new fields.
>>>
>>> ICE COMMS DDP package is required to create a filter as it contains PPPoE
>>> profiles. Added a warning message when loaded DDP package does not contain
>>> required profiles.
>>>
>>> Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
>>> will add this feature.
>>>
>>> [0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com
>>
>> Out of curiosity, can ice direct PPPoE Session packets to different
>> queues with RSS (based on the session ID)?
> 
> Hardware should support it, but I'm not sure if it's possible with the current driver and how to configure it. I'll try to find out.

From what I understand, currently it's not possible to configure RSS for PPPoE session id, because ethtool does not support PPPoE.

Re: [RFC PATCH net-next v3 4/4] ice: Add support for PPPoE hardware offload

[Guillaume Nault]

On Tue, Jul 05, 2022 at 11:54:08AM +0200, Marcin Szycik wrote:
> 
> 
> On 01-Jul-22 18:12, Marcin Szycik wrote:
> > 
> > 
> > On 01-Jul-22 01:12, Guillaume Nault wrote:
> >> On Wed, Jun 29, 2022 at 04:38:59PM +0200, Marcin Szycik wrote:
> >>> Add support for creating PPPoE filters in switchdev mode. Add support
> >>> for parsing PPPoE and PPP-specific tc options: pppoe_sid and ppp_proto.
> >>>
> >>> Example filter:
> >>> tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
> >>>     1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR
> >>>
> >>> Changes in iproute2 are required to use the new fields.
> >>>
> >>> ICE COMMS DDP package is required to create a filter as it contains PPPoE
> >>> profiles. Added a warning message when loaded DDP package does not contain
> >>> required profiles.
> >>>
> >>> Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
> >>> will add this feature.
> >>>
> >>> [0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com
> >>
> >> Out of curiosity, can ice direct PPPoE Session packets to different
> >> queues with RSS (based on the session ID)?
> > 
> > Hardware should support it, but I'm not sure if it's possible with the current driver and how to configure it. I'll try to find out.
> 
> From what I understand, currently it's not possible to configure RSS for PPPoE session id, because ethtool does not support PPPoE.

Thanks, that's interesting. PPPoE support in RSS would have been useful
to me a few years ago. I've heard some former collegues tried to use
eBPF to work around this limitation and spread packets to different
cores.

Re: [Intel-wired-lan] [RFC PATCH net-next v3 4/4] ice: Add support for PPPoE hardware offload

[Guillaume Nault]

On Tue, Jul 05, 2022 at 11:54:08AM +0200, Marcin Szycik wrote:
> 
> 
> On 01-Jul-22 18:12, Marcin Szycik wrote:
> > 
> > 
> > On 01-Jul-22 01:12, Guillaume Nault wrote:
> >> On Wed, Jun 29, 2022 at 04:38:59PM +0200, Marcin Szycik wrote:
> >>> Add support for creating PPPoE filters in switchdev mode. Add support
> >>> for parsing PPPoE and PPP-specific tc options: pppoe_sid and ppp_proto.
> >>>
> >>> Example filter:
> >>> tc filter add dev $PF1 ingress protocol ppp_ses prio 1 flower pppoe_sid \
> >>>     1234 ppp_proto ip skip_sw action mirred egress redirect dev $VF1_PR
> >>>
> >>> Changes in iproute2 are required to use the new fields.
> >>>
> >>> ICE COMMS DDP package is required to create a filter as it contains PPPoE
> >>> profiles. Added a warning message when loaded DDP package does not contain
> >>> required profiles.
> >>>
> >>> Note: currently matching on vlan + PPPoE fields is not supported. Patch [0]
> >>> will add this feature.
> >>>
> >>> [0] https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20220420210048.5809-1-martyna.szapar-mudlaw@intel.com
> >>
> >> Out of curiosity, can ice direct PPPoE Session packets to different
> >> queues with RSS (based on the session ID)?
> > 
> > Hardware should support it, but I'm not sure if it's possible with the current driver and how to configure it. I'll try to find out.
> 
> From what I understand, currently it's not possible to configure RSS for PPPoE session id, because ethtool does not support PPPoE.

Thanks, that's interesting. PPPoE support in RSS would have been useful
to me a few years ago. I've heard some former collegues tried to use
eBPF to work around this limitation and spread packets to different
cores.

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan