On 12/08/2024 16:11, Joshua Lant wrote:
Many of the uapi structs in the netfilter subsystem contain kernel pointers which cause alignment issues when the kernel parses netlink messages. Change these pointers to use unsigned longs, and cast to a pointer when used inside the kernel.
Thank you for putting together these patches! The general principle looks good to me (I've replied to a few patches with some nits).
That said, looking at all these added casts from/to unsigned long makes me realise this might not be such a good idea... As things stand, this should work as expected. The issue is that we are now baking into the uapi the assumption that kernel pointers are the size of unsigned long, in other words that the kernel is not itself purecap. If someone were to later try and move to a purecap kernel, that would immediately break.
I don't think there's a pretty solution to this, because fundamentally those pointers should never have ended up in uapi structs. My feeling is that the least bad option is to make those fields the size of a user pointer. This is wasteful on a hybrid kernel, but it is compatible with any kernel ABI, and another upside is that there is no uapi change (i.e. an application built against old uapi headers would still work fine). That could be done by simply adding __user to the field type, but this gets ugly on the kernel side in terms of casting. What is probably preferable is to define a new type, similar to __kernel_uintptr_t except that it is the size of an unsigned long in !PCuABI (instead of __u64). Its usage is quite specific to netfilter so we could name it accordingly, e.g. __nf_kptr_t. It's quite tricky to get the #ifdef'ing right but in principle it should be exactly the same as commit "uapi: types.h: define __kernel_uintptr_t type" [1], adding the type to netfilter kernel/uapi headers instead.
Once this is done your series should remain broadly the same - just replace unsigned long with __nf_kptr_t in all the changes.
Really sorry to make you change the approach again, I should have considered the kernel ABI assumption earlier.
Kevin
[1] https://git.morello-project.org/morello/kernel/linux/-/commit/f8a9c31d0342b
For more information on the necessity of these patches, see discussion in this thread:
https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...
Joshua Lant (16): x_tables.h: remove kernel pointer from uapi xt_entry_match struct x_tables.h: remove kernel pointer from uapi xt_entry_target struct xt_CT: remove pointer from uapi struct xt_IDLETIMER: remove pointer from uapi struct xt_RATEEST: remove pointer from uapi struct xt_TEE: remove pointer from uapi struct xt_bpf: remove pointer from uapi struct xt_connlimit: remove pointer from uapi struct xt_hashlimit: remove pointer from uapi struct xt_limit: remove pointer from uapi struct xt_nfacct: remove pointer from uapi struct xt_quota: remove pointer from uapi struct xt_rateest: remove pointer from uapi struct xt_statistic: remove pointer from uapi struct ebtables: remove pointer from uapi struct ipt_CLUSTERIP: remove pointer from uapi struct
include/uapi/linux/netfilter/x_tables.h | 4 +- include/uapi/linux/netfilter/xt_CT.h | 4 +- include/uapi/linux/netfilter/xt_IDLETIMER.h | 6 +- include/uapi/linux/netfilter/xt_RATEEST.h | 3 +- include/uapi/linux/netfilter/xt_TEE.h | 3 +- include/uapi/linux/netfilter/xt_bpf.h | 6 +- include/uapi/linux/netfilter/xt_connlimit.h | 3 +- include/uapi/linux/netfilter/xt_hashlimit.h | 17 ++- include/uapi/linux/netfilter/xt_limit.h | 3 +- include/uapi/linux/netfilter/xt_nfacct.h | 6 +- include/uapi/linux/netfilter/xt_quota.h | 3 +- include/uapi/linux/netfilter/xt_rateest.h | 5 +- include/uapi/linux/netfilter/xt_statistic.h | 3 +- .../uapi/linux/netfilter_bridge/ebtables.h | 15 +- .../uapi/linux/netfilter_ipv4/ipt_CLUSTERIP.h | 3 +- net/bridge/netfilter/ebtable_broute.c | 2 +- net/bridge/netfilter/ebtable_filter.c | 6 +- net/bridge/netfilter/ebtable_nat.c | 6 +- net/bridge/netfilter/ebtables.c | 64 ++++----- net/ipv4/netfilter/arp_tables.c | 22 +-- net/ipv4/netfilter/ip_tables.c | 44 +++--- net/ipv6/netfilter/ip6_tables.c | 44 +++--- net/netfilter/x_tables.c | 20 +-- net/netfilter/xt_CT.c | 10 +- net/netfilter/xt_IDLETIMER.c | 132 +++++++++--------- net/netfilter/xt_RATEEST.c | 12 +- net/netfilter/xt_TCPMSS.c | 2 +- net/netfilter/xt_TEE.c | 12 +- net/netfilter/xt_bpf.c | 18 +-- net/netfilter/xt_connlimit.c | 8 +- net/netfilter/xt_hashlimit.c | 24 ++-- net/netfilter/xt_limit.c | 6 +- net/netfilter/xt_nfacct.c | 8 +- net/netfilter/xt_quota.c | 12 +- net/netfilter/xt_rateest.c | 12 +- net/netfilter/xt_statistic.c | 12 +- 36 files changed, 294 insertions(+), 266 deletions(-)