net: Extend the protocol handling in Ethernet #84037

jukkar · 2025-01-15T15:22:30Z

Allow user to specify protocol extensions when receiving data from Ethernet network. This means that user can register L3 protocol handler using NET_L3_REGISTER() with the desired protocol type. Ethernet code will then call the handler if such a protocol type packet is received. This is currently only implemented for Ethernet. The original IPv4 and IPv6 handling is left intact even if they can be considered to be L3 layer protocol. This could be changed in the future if needed so that IPv4 and IPv6 handling could be made pluggable protocols

clamattia

LGTM, please have a look at some ideas and comments from my side.

clamattia · 2025-01-16T07:49:07Z

subsys/net/lib/ptp/ptp.c

+	return NET_CONTINUE;
+}
+
+NET_L3_REGISTER(arp, NET_ETH_PTYPE_PTP, ptp_recv);


typo?

Suggested change

NET_L3_REGISTER(arp, NET_ETH_PTYPE_PTP, ptp_recv);

NET_L3_REGISTER(ptp, NET_ETH_PTYPE_PTP, ptp_recv);

Yes, I noticed it too and it should already fixed by latest version.

clamattia · 2025-01-16T07:51:14Z

include/zephyr/net/net_core.h

+	/** Is L2 address correct */
+	bool (*l2_addr_check)(struct net_if *iface,
+			      uint16_t ptype,
+			      struct net_pkt *pkt,
+			      struct net_linkaddr *dst_lladdr);


I support the idea of this, but would it not be enough for the handler to check first and then to return NET_DROP if the address check fails. I believe the separate l2_addr_check function is maybe not necessary?

I considered it but there are couple of issues with that:

There is a check whether the packet is destined to this host. The check cannot be done after the handler function because it is too late, and it cannot be done before the handler function because then possible multicast addresses specific to some protocol (gPTP or LLDP) would be dropped.

We remove the L2 header from the net_buf before calling the handler function to allow the handler to directly use its payload. Then it is more cumbersome to check the Ethernet addresses in the handler function.

There is a check whether the packet is destined to this host.

In my view, this is protocol specific (for example different multicast addresses).

If you are refering to :

if (!net_eth_is_addr_broadcast((struct net_eth_addr *)lladdr->addr) && !net_eth_is_addr_multicast((struct net_eth_addr *)lladdr->addr) && !net_eth_is_addr_lldp_multicast( (struct net_eth_addr *)lladdr->addr) && !net_eth_is_addr_ptp_multicast((struct net_eth_addr *)lladdr->addr) && !net_linkaddr_cmp(net_if_get_link_addr(iface), lladdr)) {

I think it maybe enough to drop there all frames that are not:

any multicast (net_eth_is_addr_group, i do not understand the name btw)

unicast with us as destination

broadcast

Specific multicast checks can then be done inside the handlers.

So the (adjusted) check can be done before, I would argue.

Then it is more cumbersome to check the Ethernet addresses in the handler function

But is it worth complicating the callback interface for this? I do not know. It is a balancing of interests.

Maybe, if we pass src and dst to the handler as @pdgendt suggests, there is even less inconvenience and less an argument for having a separate function.

I revisited this and removed the additional address checker. This now expects that the mac group address bit is set for the special mac addresses we need to check.

clamattia · 2025-01-16T07:52:29Z

subsys/net/l2/ethernet/ethernet.c

 	if (!net_eth_is_addr_broadcast((struct net_eth_addr *)lladdr->addr) &&
 	    !net_eth_is_addr_multicast((struct net_eth_addr *)lladdr->addr) &&
-	    !net_eth_is_addr_lldp_multicast(


Here you could check the is_group, I believe.

clamattia · 2025-01-16T07:53:53Z

subsys/net/l2/ethernet/ethernet.c

+			handled = true;
+			break;
+		} else {
+			goto drop;


What if multiple handlers are registered for the same ptype?

Maybe we could say "it is not allowed", but then ideally this is checked at either BUILD-time or if not possible at init?

The alternative would be to just pass the packet by all the registered handlers and only drop it if all of them had a chance to look at it.

What is the use case for having multiple handlers for a same ptype, it is not clear to me why would we allow that.

I do not see a a usecase either right now.
Maybe it would just be helpful to avoid misconfiguration, if possible.

clamattia · 2025-01-16T07:56:16Z

subsys/net/l2/ethernet/ethernet.c

+#endif
+
+#ifdef CONFIG_NET_ARP
+static enum net_verdict ethernet_arp_recv(struct net_if *iface,


Maybe this could be moved to the arp file?

Could be, I will check it.

pdgendt · 2025-01-16T09:08:36Z

include/zephyr/net/net_core.h

+#define NET_L3_REGISTER(_name, _ptype, _handler, _is_addr_ok)		 \
+	static const STRUCT_SECTION_ITERABLE(net_l3_register,		 \
+					     NET_L3_GET_NAME(_name)) = { \
+		.ptype = _ptype,					 \


One optimization could be to sort these on the ptype value for faster lookups.

Could you elaborate this comment more, I thought that there is not much possibilities to say what order the linker places these.

I have something in a downstream project, but realized it's only compatible with ld scripts. I've created a PR #84089 where you can create a named section using a numeric value to sort.

You would be able to use the ptype value to sort the sections.

Ok, I could add the ptype to the section name but as the value is defined as hex in ethernet.h the sections names look like

._net_l3_register.static.__net_l3_register_ARP_0x0806_ ._net_l3_register.static.__net_l3_register_IPv4_0x0800_ ._net_l3_register.static.__net_l3_register_IPv6_0x86dd_ ._net_l3_register.static.__net_l3_register_gPTP_0x88f7_

Is your PR able to sort these?

Sadly not :(

pdgendt · 2025-01-16T09:12:51Z

include/zephyr/net/net_core.h

+	bool (*l2_addr_check)(struct net_if *iface,
+			      uint16_t ptype,
+			      struct net_pkt *pkt,
+			      struct net_linkaddr *dst_lladdr);


Not with a usecase in mind, but maybe pass both source and destination address?

Suggested change

bool (*l2_addr_check)(struct net_if *iface,

uint16_t ptype,

struct net_pkt *pkt,

struct net_linkaddr *dst_lladdr);

bool (*l2_addr_check)(struct net_if *iface,

uint16_t ptype,

struct net_pkt *pkt,

struct net_linkaddr *src_lladdr,

struct net_linkaddr *dst_lladdr);

I removed the extra address checker to simplify things and refactored the code so the link address parameter is not needed any more.

include/zephyr/net/net_core.h

pdgendt · 2025-01-16T09:18:15Z

subsys/net/l2/ethernet/gptp/gptp.c

+		       uint16_t ptype,
+		       struct net_pkt *pkt,
+		       struct net_linkaddr *dst_lladdr)
+{


Missing ARG_UNUSED(iface);, same for other arguments.

pdgendt · 2025-01-16T09:18:41Z

subsys/net/l2/ethernet/lldp/lldp.c

+		       uint16_t ptype,
+		       struct net_pkt *pkt,
+		       struct net_linkaddr *dst_lladdr)
+{


cmake/linker_script/common/common-rom.cmake

include/zephyr/net/net_core.h

subsys/net/l2/ethernet/ethernet.c

jukkar · 2025-01-16T14:38:20Z

Updated a new version and tried to address all the comments

jukkar · 2025-01-16T14:43:31Z

include/zephyr/net/ethernet.h

@@ -119,6 +119,7 @@ struct net_eth_addr {
 #define NET_ETH_MINIMAL_FRAME_SIZE	60   /**< Minimum Ethernet frame size */
 #define NET_ETH_MTU			1500 /**< Ethernet MTU size */

+


🤦 will remove this extra empty line in next version

clamattia

LGTM, just a observation and question to help avoid bugs under misconfiguration.

clamattia · 2025-01-16T16:24:20Z

subsys/net/l2/ethernet/gptp/gptp.c

+
+	if (!(net_eth_is_addr_ptp_multicast(
+		      (struct net_eth_addr *)net_pkt_lladdr_dst(pkt)->addr) ||
+	      net_eth_is_addr_lldp_multicast(


Are you intentionally checking lldp here?

Yes, gPTP has two well known multicast mac addresses that it needs to listen.

clamattia · 2025-01-16T16:25:55Z

subsys/net/l2/ethernet/ethernet.c

-		goto drop;
-	}
+	STRUCT_SECTION_FOREACH(net_l3_register, l3) {
+		if (l3->ptype != type || l3->l2 != &NET_L2_GET_NAME(ETHERNET)) {


Would it be worth checking, that l3->handler!=NULL?

I will add build assert for this, it should be enough. It is very unlikely that the handler would not be set properly.

Allow user to specify protocol extensions when receiving data from Ethernet network. This means that user can register L3 protocol handler using NET_L3_REGISTER() with the desired protocol type. Ethernet code will then call the handler if such a protocol type packet is received. This is currently only implemented for Ethernet. The original IPv4 and IPv6 handling is left intact even if they can be considered to be L3 layer protocol. This could be changed in the future if needed so that IPv4 and IPv6 handling could be made pluggable protocols. Signed-off-by: Jukka Rissanen <[email protected]>

jukkar · 2025-01-17T07:12:56Z

CI fixes

jukkar mentioned this pull request Jan 16, 2025

net: ethernet: Align handling of different ethernet protocols in recv #83967

Closed

jukkar force-pushed the devel/net-l2-extend-ptype-handling branch from da49086 to ef8c9e5 Compare January 16, 2025 08:19

jukkar marked this pull request as ready for review January 16, 2025 08:19

zephyrbot added area: Wi-Fi Wi-Fi area: Linker Scripts area: PTP IEEE 1588 PTP Protocol area: Networking area: Build System labels Jan 16, 2025

zephyrbot requested review from 57300, awojasinski, jeremybettis, krish2718, MaochenWang1, nashif, nordicjm, pdgendt, rlubos, ssharks, tbursztyka and tejlmand January 16, 2025 08:20

zephyrbot assigned tejlmand Jan 16, 2025

jukkar assigned rlubos and unassigned tejlmand Jan 16, 2025

clamattia reviewed Jan 16, 2025

View reviewed changes

pdgendt reviewed Jan 16, 2025

View reviewed changes

rlubos reviewed Jan 16, 2025

View reviewed changes

cmake/linker_script/common/common-rom.cmake Outdated Show resolved Hide resolved

include/zephyr/net/net_core.h Show resolved Hide resolved

subsys/net/l2/ethernet/ethernet.c Outdated Show resolved Hide resolved

jukkar force-pushed the devel/net-l2-extend-ptype-handling branch from ef8c9e5 to 8f280f5 Compare January 16, 2025 14:37

zephyrbot added the area: Ethernet label Jan 16, 2025

zephyrbot requested a review from decsny January 16, 2025 14:38

jukkar requested review from clamattia, rlubos and pdgendt January 16, 2025 14:40

jukkar commented Jan 16, 2025

View reviewed changes

pdgendt previously approved these changes Jan 16, 2025

View reviewed changes

awojasinski previously approved these changes Jan 16, 2025

View reviewed changes

rlubos previously approved these changes Jan 16, 2025

View reviewed changes

clamattia reviewed Jan 16, 2025

View reviewed changes

jukkar dismissed stale reviews from rlubos, awojasinski, and pdgendt via 613cd7b January 17, 2025 07:12

jukkar force-pushed the devel/net-l2-extend-ptype-handling branch from 8f280f5 to 613cd7b Compare January 17, 2025 07:12

pdgendt approved these changes Jan 17, 2025

View reviewed changes

rlubos approved these changes Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: Extend the protocol handling in Ethernet #84037

net: Extend the protocol handling in Ethernet #84037

jukkar commented Jan 15, 2025 •

edited by kartben

Loading

clamattia left a comment

clamattia Jan 16, 2025

jukkar Jan 16, 2025

clamattia Jan 16, 2025 •

edited

Loading

jukkar Jan 16, 2025

clamattia Jan 16, 2025 •

edited

Loading

clamattia Jan 16, 2025

jukkar Jan 16, 2025

clamattia Jan 16, 2025

clamattia Jan 16, 2025

jukkar Jan 16, 2025

clamattia Jan 16, 2025

clamattia Jan 16, 2025

jukkar Jan 16, 2025

pdgendt Jan 16, 2025

jukkar Jan 16, 2025

pdgendt Jan 16, 2025 •

edited

Loading

jukkar Jan 16, 2025

pdgendt Jan 16, 2025

pdgendt Jan 16, 2025

jukkar Jan 16, 2025

pdgendt Jan 16, 2025

pdgendt Jan 16, 2025

jukkar commented Jan 16, 2025

jukkar Jan 16, 2025

clamattia left a comment

clamattia Jan 16, 2025

jukkar Jan 17, 2025

clamattia Jan 16, 2025

jukkar Jan 17, 2025

jukkar commented Jan 17, 2025

	NET_L3_REGISTER(arp, NET_ETH_PTYPE_PTP, ptp_recv);
	NET_L3_REGISTER(ptp, NET_ETH_PTYPE_PTP, ptp_recv);

		@@ -119,6 +119,7 @@ struct net_eth_addr {
		#define NET_ETH_MINIMAL_FRAME_SIZE 60 /*< Minimum Ethernet frame size /
		#define NET_ETH_MTU 1500 /*< Ethernet MTU size /

net: Extend the protocol handling in Ethernet #84037

Are you sure you want to change the base?

net: Extend the protocol handling in Ethernet #84037

Conversation

jukkar commented Jan 15, 2025 • edited by kartben Loading

clamattia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamattia Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamattia Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdgendt Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jukkar commented Jan 16, 2025

Choose a reason for hiding this comment

clamattia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jukkar commented Jan 17, 2025

jukkar commented Jan 15, 2025 •

edited by kartben

Loading

clamattia Jan 16, 2025 •

edited

Loading

clamattia Jan 16, 2025 •

edited

Loading

pdgendt Jan 16, 2025 •

edited

Loading