net_core: Decide about l2-processing based on l2_processed-flag #93050

ClaCodes · 2025-07-12T06:41:41Z

Use the l2_processed-flag to decide whether a network packet needs to be processed by an L2-handler. This could be used in the future to requeue packets for later processing by a different traffic class queue.

Note: this would break all out-of-tree code, that assumed setting reassembled is enough for the packet to be not processed by l2 handlers. Not sure, if this is a valid concern?

Finally goal is to reach something like, so that processing of packets can be deferred easily:

void process_data(struct net_pkt *pkt) {
    // ...
    if (!net_pkt_is_l2_processed(pkt)) {
        process_l2(pkt);
        net_pkt_set_l2_processed(pkt, true);
        bool updated = update_priority(pkt);
        if (updated) {
            net_queue_rx(net_pkt_iface(pkt), pkt);
            return;
        }
    }
    // ...
    if (!net_pkt_is_l3_processed(pkt)) {
        process_l3(pkt);
        net_pkt_set_l3_processed(pkt, true);
        bool updated = update_priority(pkt);
        if (updated) {
            net_queue_rx(net_pkt_iface(pkt), pkt);
            return;
        }
    }
    // ...
    if (!net_pkt_is_l4_processed(pkt)) {
        process_l4(pkt);
        net_pkt_set_l4_processed(pkt, true);
        bool updated = update_priority(pkt);
        if (updated) {
            net_queue_rx(net_pkt_iface(pkt), pkt);
            return;
        }
    }
}

Maybe instead:

    process_ll(pkt);
    process_transport(pkt); // net_ipvX_input
    process_connection(pkt); // net_conn_input

rlubos

Generally no objections, but I would split the loopback flag change from other changes into as separate commit.

ClaCodes · 2025-07-15T04:53:08Z

Generally no objections, but I would split the loopback flag change from other changes into as separate commit.

done
rebased

include/zephyr/net/net_pkt.h

rlubos · 2025-07-15T09:41:02Z

subsys/net/ip/net_core.c

@@ -493,6 +483,7 @@ static void net_rx(struct net_if *iface, struct net_pkt *pkt)
 #ifdef CONFIG_NET_L2_DUMMY
 		if (net_if_l2(iface) == &NET_L2_GET_NAME(DUMMY)) {
 			net_pkt_set_loopback(pkt, true);
+			net_pkt_set_l2_processed(pkt, true);


This seems to break AF_PACKET/SOCK_RAW sockets processing on loopback, some tests use this combination and now fail.

We need to find a solution for AF_PACKET/SOCK_RAW. It can not stay unconditionally at the begining of process_data, as a packet will soon pass there multiple times potentially (unless we want the packet to be inputted many times into a AF_PACKET/SOCK_RAW, which I assume we do not want). When we have deffered packet processing a packet may go through process_data many times with different stages of its processing. Initially, maybe l2_processed is false, then on second pass, l2_processed is true, but l3_processed is false, .... That is at least my vision.

Then, maybe we could also move this into the layer 2 if:

if (!net_pkt_is_l2_processed(pkt)) { /* forward packets to SOCK_RAW packet sockets. */ ret = net_packet_socket_input(pkt, ETH_P_ALL); if (ret != NET_CONTINUE) { return ret; } ret = net_if_recv_data(net_pkt_iface(pkt), pkt); net_pkt_set_l2_processed(pkt, true); // ...

But this will of course equally break the tests, that want to send a loopback packet and expect it to be passed to AF_PACKET/SOCK_RAW. Do locally routed packets have to be inputed in those sockets?

Note: We also have tests, that check that reassembled packets reach the raw sockets...

Not sure, what to do with this either?

One more note (sorry for spam): we may even want to input the packet into raw socket only after it was processed by L2. See zpacket_set_source_addr where it currently has to do ethernet specific address parsing, because the packet was not yet processed by L2, when it was inputed into the raw socket.

Note: We also have tests, that check that reassembled packets reach the raw sockets...

Which tests are you referring to, I don't see any packet sockets being used in IP fragmentation tests?

we may even want to input the packet into raw socket only after it was processed by L2. See zpacket_set_source_addr where it currently has to do ethernet specific address parsing, because the packet was not yet processed by L2, when it was inputed into the raw socket.

I don't think that's a viable approach (at least not without some even further refactoring) as L2 strips the L2 header, which is expected to be present in packet with SOCK_RAW socket.

We need to find a solution for AF_PACKET/SOCK_RAW. It can not stay unconditionally at the begining of process_data, as a packet will soon pass there multiple times potentially (unless we want the packet to be inputted many times into a AF_PACKET/SOCK_RAW, which I assume we do not want). When we have deffered packet processing a packet may go through process_data many times with different stages of its processing. Initially, maybe l2_processed is false, then on second pass, l2_processed is true, but l3_processed is false, .... That is at least my vision.

Then perhaps we need yet another flag on the packet similar to l2_processed, something like raw_processed to ensure packet is delivered only once. For sure we do not want the packet socket to receive multiple copies of the same packet.

Do locally routed packets have to be inputed in those sockets?

To be fair, I'm not sure if they have to (we currently only seem to use this "feature" in tests) but at the same time I'm not convinced on limiting functionality because of implementation details. Probably would be good to check how it works in other systems (with Linux in mind), i.e. whether AF_PACKET/SOCK_RAW socket is able to receive loopback packets.

Which tests are you referring to, I don't see any packet sockets being used in IP fragmentation tests?

My bad, I was confused.

I don't think that's a viable approach (at least not without some even further refactoring) as L2 strips the L2 header, which is expected to be present in packet with SOCK_RAW socket.

Ok, makes sense.

Then perhaps we need yet another flag on the packet similar to l2_processed, something like raw_processed to ensure packet is delivered only once. For sure we do not want the packet socket to receive multiple copies of the same packet.

Ok, will go this direction in future PRs. We can discuss then in detail.

rlubos · 2025-07-15T09:43:11Z

subsys/net/ip/net_core.c

@@ -92,8 +82,9 @@ static inline enum net_verdict process_data(struct net_pkt *pkt)
 		return NET_DROP;
 	}

-	if (!net_pkt_is_loopback(pkt) && !locally_routed) {
+	if (!net_pkt_is_l2_processed(pkt)) {


Referring to the previous commit, perhaps this should check for both flags, i.e.:

if (!net_pkt_is_loopback(pkt) && !net_pkt_is_l2_processed(pkt)) {

instead of setting l2_processed flag for loopback packets?

Ok, will do but see comment below.

2bd0fc4 reverts and tries a different approach.

Store the flag in the packet meta-data so that processing may be deferred if necessary. Signed-off-by: Cla Mattia Galliard <clamattia@gmail.com>

ClaCodes

Thanks for the quick feedback.

ClaCodes · 2025-07-15T20:00:34Z

subsys/net/ip/net_core.c

@@ -493,6 +483,7 @@ static void net_rx(struct net_if *iface, struct net_pkt *pkt)
 #ifdef CONFIG_NET_L2_DUMMY
 		if (net_if_l2(iface) == &NET_L2_GET_NAME(DUMMY)) {
 			net_pkt_set_loopback(pkt, true);
+			net_pkt_set_l2_processed(pkt, true);


We need to find a solution for AF_PACKET/SOCK_RAW. It can not stay unconditionally at the begining of process_data, as a packet will soon pass there multiple times potentially (unless we want the packet to be inputted many times into a AF_PACKET/SOCK_RAW, which I assume we do not want). When we have deffered packet processing a packet may go through process_data many times with different stages of its processing. Initially, maybe l2_processed is false, then on second pass, l2_processed is true, but l3_processed is false, .... That is at least my vision.

Then, maybe we could also move this into the layer 2 if:

if (!net_pkt_is_l2_processed(pkt)) { /* forward packets to SOCK_RAW packet sockets. */ ret = net_packet_socket_input(pkt, ETH_P_ALL); if (ret != NET_CONTINUE) { return ret; } ret = net_if_recv_data(net_pkt_iface(pkt), pkt); net_pkt_set_l2_processed(pkt, true); // ...

But this will of course equally break the tests, that want to send a loopback packet and expect it to be passed to AF_PACKET/SOCK_RAW. Do locally routed packets have to be inputed in those sockets?

ClaCodes · 2025-07-15T20:06:18Z

subsys/net/ip/net_core.c

@@ -92,8 +82,9 @@ static inline enum net_verdict process_data(struct net_pkt *pkt)
 		return NET_DROP;
 	}

-	if (!net_pkt_is_loopback(pkt) && !locally_routed) {
+	if (!net_pkt_is_l2_processed(pkt)) {


Ok, will do but see comment below.

include/zephyr/net/net_pkt.h

Use the l2_processed-flag to decide whether a network packet needs to be processed by an L2-handler. This could be used in the future to requeue packets for later processing by a different traffic class queue. Signed-off-by: Cla Mattia Galliard <clamattia@gmail.com>

Specify the socket type, when inputing a packet into a packet-socket. Signed-off-by: Cla Mattia Galliard <clamattia@gmail.com>

sonarqubecloud · 2025-07-16T16:47:51Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.5% Duplication on New Code

See analysis details on SonarQube Cloud

ClaCodes · 2025-07-16T17:05:05Z

On the meaning of `l2_processed`

There are basically two possible interpretation of the flag at the moment:

Does the flag mean, that l2-processing is no longer needed and so the respective step in process_data can be skipped
or does it mean, that the l2-header information is present in the pkts meta data?

In the first case (I am assuming in this PR), the users have to set the flag (for reassembled and for loopback), in the second case, it is set l2_processing when the header information is put in the packets meta-data and we need to check the other flags as well to determine, whether L2-processing is necessary (!is_loopback && !is_reassembed && !is_l2_processed).

I will revert if decision is made for the second interpretation.

ClaCodes · 2025-07-17T05:38:55Z

See also: #93245

rlubos · 2025-07-17T09:54:33Z

There are basically two possible interpretation of the flag at the moment:

Does the flag mean, that l2-processing is no longer needed and so the respective step in process_data can be skipped

or does it mean, that the l2-header information is present in the pkts meta data?

Those two are not contradictory IMO, and actually I think both assumptions are correct, which I think can be summarized with a single sentence "L2 header is not longer present in the packet" and that indicates that both:

All meta-data (proto type, LL address etc.) should be set in the packet,
L2 processing is not only no longer needed, but is not really possible anymore as there's no L2 header to work with.

clamattia · 2025-07-17T11:08:18Z

I think both assumptions are correct

Not sure, example:

loopback (L2 processing is not needed, but AFAIK no LL address meta-data is set in the header)
same with reassembled

rlubos · 2025-07-17T11:27:21Z

loopback (L2 processing is not needed, but AFAIK no LL address meta-data is set in the header)

I didn't dig into details, but there's definitely some logic for copying the proto and LL address for the loopback case:

zephyr/subsys/net/ip/net_core.c

Lines 327 to 328 in 31ef45e

    
           net_pkt_set_ll_proto_type(pkt, ETH_P_IP); 
        
           copy_ll_addr(pkt);

same with reassembled

That should probably be revisited then, IMHO reassembly code should set any metadata that might be needed by upper layers.

clamattia · 2025-07-17T11:39:19Z

IMHO reassembly code should set any metadata that might be needed by upper layers.

Maybe my bad. Will dig into it.

clamattia · 2025-07-17T11:40:07Z

Those two are not contradictory IMO, and actually I think both assumptions are correct

That would be ideal and least confusing.

github-actions bot added the area: Networking label Jul 12, 2025

github-actions bot requested review from jukkar, pdgendt, rlubos and ssharks July 12, 2025 06:42

github-actions bot assigned rlubos and jukkar Jul 12, 2025

ClaCodes force-pushed the main branch 4 times, most recently from 48aa070 to a2aea74 Compare July 12, 2025 07:42

rlubos reviewed Jul 14, 2025

View reviewed changes

ClaCodes force-pushed the main branch from a2aea74 to c1e4c22 Compare July 15, 2025 04:50

ClaCodes force-pushed the main branch from c1e4c22 to 63f4ac7 Compare July 15, 2025 04:56

pdgendt reviewed Jul 15, 2025

View reviewed changes

include/zephyr/net/net_pkt.h Outdated Show resolved Hide resolved

rlubos reviewed Jul 15, 2025

View reviewed changes

net_pkt: Store is_loopback info in packet meta-data

74aa269

Store the flag in the packet meta-data so that processing may be deferred if necessary. Signed-off-by: Cla Mattia Galliard <clamattia@gmail.com>

ClaCodes force-pushed the main branch from 63f4ac7 to c8829ae Compare July 15, 2025 20:15

ClaCodes commented Jul 15, 2025

View reviewed changes

ClaCodes force-pushed the main branch 2 times, most recently from fd65f8b to 54a1036 Compare July 15, 2025 20:21

ClaCodes force-pushed the main branch 2 times, most recently from 2bd0fc4 to 2730c6c Compare July 16, 2025 06:18

packet_socket: Specify the socket-type, when inputing

888723c

Specify the socket type, when inputing a packet into a packet-socket. Signed-off-by: Cla Mattia Galliard <clamattia@gmail.com>

ClaCodes force-pushed the main branch from 2730c6c to 888723c Compare July 16, 2025 16:42

ClaCodes mentioned this pull request Jul 17, 2025

net: connection: Unconditionally forward packets when handling packet sockets #93246

Open

net_core: Decide about l2-processing based on l2_processed-flag #93050

Are you sure you want to change the base?

net_core: Decide about l2-processing based on l2_processed-flag #93050

Conversation

ClaCodes commented Jul 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rlubos left a comment

Choose a reason for hiding this comment

Uh oh!

ClaCodes commented Jul 15, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClaCodes Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClaCodes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud bot commented Jul 16, 2025

Quality Gate passed

Uh oh!

ClaCodes commented Jul 16, 2025

On the meaning of l2_processed

Uh oh!

ClaCodes commented Jul 17, 2025

Uh oh!

rlubos commented Jul 17, 2025

Uh oh!

clamattia commented Jul 17, 2025

Uh oh!

rlubos commented Jul 17, 2025

Uh oh!

clamattia commented Jul 17, 2025

Uh oh!

clamattia commented Jul 17, 2025

Uh oh!

Uh oh!

ClaCodes commented Jul 12, 2025 •

edited

Loading

ClaCodes Jul 16, 2025 •

edited

Loading

On the meaning of `l2_processed`