Add netlink support to if_gre
Details
Details
I also wrote tests for gre, since it doesn't have it before.
# kyua test -k /usr/tests/sys/netlink/Kyuafile test_rtnl_gre:test_rtnl_gre test_rtnl_gre:test_rtnl_gre -> passed [0.002s] Results file id is usr_tests_sys_netlink.20260101-180800-774463 Results saved to /root/.kyua/store/results.usr_tests_sys_netlink.20260101-180800-774463.db 1/1 passed (0 failed)
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 70529 Build 67412: arc lint + arc unit
Event Timeline
Comment Actions
There're three tasks in this change,
- Migrate to new if_clone KPI
- Refactor some ioctls, say GRESKEY
- Add netlink support
The first two are simple and easy to review, I expect them can be landed quickly. I'd support you to split the change.
Comment Actions
I understand your perspective, however, migrating directly to the new if_clone_addreq_v2 without netlink support is not possible.
I could migrate to if_clone_addreq without netlink support first, but then transition to v2 to support netlink would ultimately be redundant.
The same reasoning applies to the GRESKEY ioctl.
Refactoring that ioctl is intended to reuse those configuration functions through netlink.
Therefore, without netlink support, refactoring the ioctl interface is pointless.
Comment Actions
- Add types in if_gre.h to fix world build
- Add copyright to test_rtnl_gre.c
- Rebase to main and fix cleanup in test_rtnl_gre.
@glebius done.
Comment Actions
Approved, but please address comment in the module event switch statement.
| sys/net/if_gre.c | ||
|---|---|---|
| 1156 | See note about /* FALLTHROUGH */ in style(9). This actually is not only about style. Some linters may complain on this function unless this special comment is added. Adding break; instead will also work. So add either of them, depending on how would you expect gremodevent() change in the future. IMHO, break; is more likely to stay here in future. | |
This comment was removed by pouria.
Comment Actions
I see no problems with this patch alongside my changes.
We have a minor conflict, which I resolved at github.
Also, AFAICU, I don't need to use priv instead of softc for netlink because I didn't touch the data plane.
Your code will synchronize priv and softc together so there should be no problem to directly using the softc.
Should I do anything else?
Comment Actions
@zlei
Can I have your opinion on this review, too?
I'm ready to commit it, but I'd prefer to wait for your feedback as well.
Comment Actions
Except for the tests, do you have real usage of the netlink protocol ? The commit message does not mention it. So it is better to explain, otherwise would confuse people.
I'm still learning netlink. @melifaro introduced netlink to FreeBSD and I believe he has better insight over the design.
| sys/net/if_gre.c | ||
|---|---|---|
| 417 | Naming is hard. I'd personally prefer to use prefix, say gre_nl_set_addr() other than suffix gre_set_addr_nl(), so that it will be easier to grep all netlink related function. | |
| 417 | Probably you do not want to interleave netlink related functions with domain specific functions. I'd suggest you move all netlink related functions down to the end of if_gre.c, just before gremodevent(). | |
| sys/net/if_gre.h | ||
| 163 | Naming. I think nl_gre_parsed reads more fluently than nl_parsed_gre. Well I'm not a native speaker, just my personal opinion. Will this be public KPI ? Probably this should be in sys/net/if_gre.c ? | |
| sys/netlink/route/interface.h | ||
| 251 | It appears these will be public KPI. Will we want to sync with Linux about the naming ? ( I have not looked at Linux sources about them, just presume it has. ) If roll our own types, what's the policy to assign type to existing tunnels ( me, gif, vxlan, etc ) ? Probably @melifaro had thoughts on this ( ever ). | |
| tests/sys/netlink/test_rtnl_gre.c | ||
| 70 | Small nits. Probably test_gre_nl or test_nl_gre is better ? test_rtnl_gre reads "test route netlink gre" to me. Get a little confused by the naming rt. | |
| sys/net/if_gre.c | ||
|---|---|---|
| 417 |
This suffix is already widely used inside the kernel. | |
| 417 |
Good point, I will. Thanks! | |
| sys/net/if_gre.h | ||
| 163 |
The reason for this is trying to match naming of other netlink parsers.
Yes it's, I'll add the support for ifconfig netlink implementation of gre interface in another review. | |
| sys/netlink/route/interface.h | ||
| 251 |
I have tried to find a middle ground between melifaro implementation and linux naming!
Currently, there is no policy. | |
| tests/sys/netlink/test_rtnl_gre.c | ||
| 70 |
I tried to match the current naming, especially test_rtnl_iface since it already has another test interface (if_vlan) inside it. | |
Comment Actions
Make nl_parsed_gre private.
@zlei done.
After doing my research, If find out you were right.
Dictating netlink data structure to userland defeats
the purpose of using netlink in first place.
| sys/net/if_gre.c | ||
|---|---|---|
| 1123 | going to fix this. | |
Comment Actions
Hi, please go ahead with your patch.
I've had a regression test for GRE that depends on your changes, since creating and destroying GRE tunnels too quickly will result in a panic in the test.
Comment Actions
It seems that the test added by this commit is failing in CI: https://ci.freebsd.org/job/FreeBSD-main-amd64-test/27898/testReport/sys.netlink/test_rtnl_gre/test_rtnl_gre/
@pouria can you take a look?
Comment Actions
I thought required_kmods was optional. so lessons learned!
I've added atf_tc_set_md_var(tc, "require.kmods", "netlink if_gre"); to my test and waited to run tests locally before commit.
However, it gave me
# kyua test -k /usr/tests/sys/netlink/Kyuafile test_rtnl_gre test_rtnl_gre:__test_cases_list__ -> broken: Unknown test case metadata property 'require.kmods' [0.001s]
Why is that happens?
I simply reused the require.kmods metadata property of other atf tests:
fgrep -r 'atf_tc_set_md_var(tc, "require.kmods", "netlink' tests/sys/netlink/
Comment Actions
Here is the patch:
https://github.com/spmzt/freebsd-src/commit/1635ba90615a5d9342604d495b71ac5380030b36
I'm not sure why it tells unknown property.
But I'm sure the reason for CI failure is the if_gre is not loaded.
EDIT: it's working on my servers, despite running the make clean before build it still gave me unknown property on my test VMs.
However, this error happens to other tests with require.kmods too. so, maybe ccache -C?
Comment Actions
Done.
https://ci.freebsd.org/job/FreeBSD-main-amd64-test/27903/consoleFull
11:09:58 sys/netlink/test_rtnl_gre:test_rtnl_gre -> skipped: Required kmods are not loaded: if_gre. [0.006s]
However, I see gcc14 complaining too:
/workspace/src/tests/sys/netlink/test_rtnl_gre.c: In function 'atfu_test_rtnl_gre_body':
/workspace/src/tests/sys/netlink/test_rtnl_gre.c:91:28: note: at offset 8 into source object 'src' of size 16
91 | struct sockaddr_in src, dst;
| ^~~
In function 'snl_add_msg_attr',
inlined from 'snl_add_msg_attr_ip' at /workspace/src/sys/netlink/netlink_snl_route.h:153:11,
inlined from 'atfu_test_rtnl_gre_body' at /workspace/src/tests/sys/netlink/test_rtnl_gre.c:117:2:
/workspace/src/sys/netlink/netlink_snl.h:1166:17: error: 'memcpy' reading 16 bytes from a region of size 8 [-Werror=stringop-overread]
1166 | memcpy((nla + 1), data, attr_len);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/workspace/src/tests/sys/netlink/test_rtnl_gre.c: In function 'atfu_test_rtnl_gre_body':
/workspace/src/tests/sys/netlink/test_rtnl_gre.c:91:33: note: at offset 8 into source object 'dst' of size 16
91 | struct sockaddr_in src, dst;
| ^~~working on it.
Comment Actions
AFAICU, this is clearly a false positive by gcc14.
We can silence it by avoiding direct calls to snl_add_msg_attr_ip and use snl_add_msg_attr_ip4 instead.
But this will not silence future calls to snl_add_msg_attr_ip with IPv4 address.
any idea?
Comment Actions
Yeah. I'm working on spinning a new release for kyua soon that should have that support. Thanks for the reminder!