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

["Drewek, Wojciech" <wojciech.drewek@intel.com>]

Thanks for the review!

> -----Original Message-----
> From: Baowen Zheng <baowen.zheng@corigine.com>
> Sent: niedziela, 26 czerwca 2022 09:20
> To: Marcin Szycik <marcin.szycik@linux.intel.com>; netdev@vger.kernel.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; davem@davemloft.net; xiyou.wangcong@gmail.com; Brandeburg, Jesse
> <jesse.brandeburg@intel.com>; gustavoars@kernel.org; boris.sukholitko@broadcom.com; elic@nvidia.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 <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>
> Subject: RE: [RFC PATCH net-next 1/4] flow_dissector: Add PPPoE dissectors
> 
> On Friday, June 24, 2022 9:42 PM, Marcin Szycik wrote:
> 
> >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>
> >---
> > include/net/flow_dissector.h | 11 ++++++++
> > net/core/flow_dissector.c    | 51 +++++++++++++++++++++++++++++++-----
> > 2 files changed, 56 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;
> > };
> >
> [..]
> >+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;
> >+	}
> >+}
> >+
> Just a suggestion, for the above code segment, will it be more clean to change as the format:
> 	switch (ntohs(proto)) {
> 	case PPP_AT:
> 	case PPP_IPX:
> 	case PPP_VJC_COMP:
> then you will not need to call function of htons repeatedly.

Sure, will be included in second version.

> > /**
> >  * __skb_flow_dissect - extract the flow_keys struct and return it
> >  * @net: associated network namespace, derived from @skb if NULL @@ -
> >1221,19 +1250,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