-
Notifications
You must be signed in to change notification settings - Fork 7.4k
net: sockets: Add SOCK_RAW support for AF_INET/AF_INET6 sockets #88044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
net: sockets: Add SOCK_RAW support for AF_INET/AF_INET6 sockets #88044
Conversation
b4d4adb
to
88fffdd
Compare
Just wondering should we have a page in network documentation or doxygen comments in the code that describes how the various options are working (I am referring to the table in the issue of the PR). We mostly follow Linux but there are some deviations so perhaps these should be mentioned somewhere. |
That's a good idea, we have a dedicated documentation page for sockets, perhaps it'd make most sense to add a table in there summarizing what socket types we do support. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
I am thinking about the tests, how can we verify that we have tested all combinations? Should we have a table in the test describing what we are testing and have a dedicated test function for each "cell" in the table, WDYT?
subsys/net/ip/connection.c
Outdated
|
||
raw_pkt = net_pkt_clone(pkt, CLONE_TIMEOUT); | ||
if (!raw_pkt) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor will be incorrect now. Perhaps have here goto to the end of the function as the cursor is restored there.
subsys/net/ip/connection.c
Outdated
@@ -757,6 +797,10 @@ enum net_verdict net_conn_input(struct net_pkt *pkt, | |||
if (proto != ETH_P_ALL && proto != IPPROTO_RAW) { | |||
continue; /* wrong protocol */ | |||
} | |||
} else if (IS_ENABLED(CONFIG_NET_SOCKETS_INET_RAW) && raw_ip_pkt) { | |||
if (conn->proto != 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do the check like this
if (conn->proto != IPPROTO_IP) {
to make it clear what we are checking against (value 0 is a bit vague).
@@ -793,6 +837,15 @@ enum net_verdict net_conn_input(struct net_pkt *pkt, | |||
|
|||
continue; /* packet was consumed */ | |||
} | |||
} else if (IS_ENABLED(CONFIG_NET_SOCKETS_INET_RAW) && raw_ip_pkt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that few lines above, the code is still checking IPPROTO_RAW
for packet socket, we need to remove/change those checks from packet socket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, do you want me to extend this PR with AF_PACKET/IPPROTO_RAW
removal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to remove the IPPROTO_RAW
from AF_PACKET
, that was actually the point of the original issue.
@@ -664,6 +680,11 @@ ssize_t zsock_sendmsg_ctx(struct net_context *ctx, const struct msghdr *msg, | |||
k_timepoint_t buf_timeout, end; | |||
int status; | |||
|
|||
if (net_context_get_type(ctx) == SOCK_RAW) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we prevent sendmsg
for raw sockets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably no good reason now that I think of it, I can drop this restriction and add some tests for sendmsg()/recvmsg().
@@ -1479,6 +1503,11 @@ ssize_t zsock_recvmsg_ctx(struct net_context *ctx, struct msghdr *msg, | |||
enum net_sock_type sock_type = net_context_get_type(ctx); | |||
size_t i, max_len = 0; | |||
|
|||
if (net_context_get_type(ctx) == SOCK_RAW) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why prevent recvmsg
for raw sockets?
I can refer to the table from the issue, and extend the test suite a bit. We'll likely duplicate some tests, but that's probably not a big deal. And now I think we should also add tests verifying that all this still works if we enable AF_PACKET support. |
88fffdd
to
e96c469
Compare
I have addressed the initial feedback, will work on extending the testsuite even more. |
subsys/net/ip/net_context.c
Outdated
@@ -2711,6 +2851,14 @@ int net_context_send(struct net_context *context, | |||
addrlen = 0; | |||
} | |||
|
|||
if (dst_check) { | |||
if (!(context->flags & NET_CONTEXT_REMOTE_ADDR_SET) || | |||
!net_sin(&context->remote)->sin_port) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit, could be written as
net_sin(&context->remote)->sin_port == 0
which is slightly easier to read
e96c469
to
6309400
Compare
In last push:
|
1479c42
to
d617ec2
Compare
d617ec2
to
8f7173a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation looks great. I think we need to mention in migration doc that the AF_PACKET will not accept IPPROTO_RAW as a protocol any more.
Register connection type along with family and protocol, so that it's possible to differentiate between connection listening for raw IP datagrams and TCP/UDP/other packets. Signed-off-by: Robert Lubos <[email protected]>
8f7173a
to
2255341
Compare
2255341
to
d3f2108
Compare
Ok this should be more or less ready, changes since last week:
|
b5f9065
to
615833b
Compare
CI fixes |
@pdgendt please take a look if you have time |
cc: @go2sh please try if it solves your issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments
Introduce changes in the networking stack which allow to create raw IP sockets, so that applications can send and receive raw IP datagrams. Signed-off-by: Robert Lubos <[email protected]>
Add test suite verifying AF_INET/AF_INET6 and SOCK_RAW sockets. Signed-off-by: Robert Lubos <[email protected]>
Add bullet point about raw IP sockets support. Signed-off-by: Robert Lubos <[email protected]>
Add a table in sockets documentation summarizing what socket types are being supported. Signed-off-by: Robert Lubos <[email protected]>
IPPROTO_RAW is not a valid protocol type for AF_PACKET sockets, which should only use IEEE 802.3 protocol numbers. Therefore remove support for this type of sockets. As an alternative, users can use AF_PACKET/SOCK_DGRAM or AF_INET(6)/SOCK_RAW, depending on the actual use case. Signed-off-by: Robert Lubos <[email protected]>
Add migration guide entry about dropped IPPROTO_RAW support from AF_PACKET sockets, with possible alternatives. Signed-off-by: Robert Lubos <[email protected]>
615833b
to
847c208
Compare
Introduce support for raw IP sockets.
Fixes #86827