On 11/10/2024 17:31, Joshua Lant wrote:
Changes from v2:
- [01/17] Updated first commit comment to make more sense to the reader.
- [XX/17] Numerous formatting nits fixed.
- [05/17] Fixed up local assignment and functional regression found.
- [08/17] Fixed up functional regression found.
Requested changes not addressed:
In patch [01/17]:
Do we really have to duplicate the typedefs here? There are already quite a few headers including <linux/netfilter.h> under include/uapi/linux/netfilter/ so including it in x_tables.h too shouldn't be too problematic.
Unfortunately this duplication is required. I encountered many issues building when I tried this initially before the last patch submission. Presumably this is an artefact of xtables being deprecated by nftables? Many definitions are duplicated between these two headers.
Could you elaborate on exactly what the errors are/what is duplicated? I don't see any issue building the kernel when including <linux/netfilter.h> in include/uapi/linux/netfilter/x_tables.h but I suppose there could be trouble when building userspace.
Many thanks,
Josh
Joshua Lant (17): netfilter: Create new type for kernel pointers. 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 xtables: move include to headers
I have tried applying those patches. Regardless of the ABI, I've found that the xt_IDLETIMER patch doesn't build - that's easily fixed. What is much more problematic is that building them in purecap generates a huge stream of warnings, for instance (showing up as an error here as I use -Werror):
kernel/private/arm/net/ipv4/netfilter/ip_tables.c:291:22: error: the following conversion will result in a CToPtr operation; the behaviour of CToPtr can be confusing since using CToPtr on an untagged capability will give 0 instead of the integer value and should therefore be explicitly annotated [-Werror,-Wcheri-pointer-conversion] acpar.match = (const struct xt_match *)ematch->u.kernel.match; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This is because casting uintcap_t to an (integer) pointer in hybrid is considered ambiguous by the compiler - does the uintcap_t value represent a capability, and do you wish to extract its address? That's not the case here and the cast is harmless, but we can't easily disable the warning. The standard way to suppress it would be to cast to ptraddr_t first, but this gets really cumbersome (there are dozens of such casts in this series):
acpar.match = (const struct xt_match *)(ptraddr_t)ematch->u.kernel.match;
This made me realise that we can take a different approach, which involves some union magic but has the huge advantage of requiring no code change whatsoever besides headers. Let's take the example of this struct:
struct xt_entry_match { union { struct { ... } user; struct { __u16 match_size;
/* Used inside the kernel */ struct xt_match *match; } kernel;
/* Total length */ __u16 match_size; } u;
unsigned char data[]; };
What we want to ensure is that the "kernel" union member has the same size regardless of the kernel ABI. We do *not* actually need its "match" member to be consistent between the kernel and userspace, because only the kernel ever uses it. So what we could do is this:
struct xt_entry_match { union { struct { ... } user; struct { __u16 match_size;
/* Used inside the kernel */ union { struct xt_match *match; __nf_kptr_t __match; }; } kernel;
/* Total length */ __u16 match_size; } u;
unsigned char data[]; };
The anonymous union enables us to keep "match" unchanged while ensuring that both sides agree on the size and layout of the overall struct (this is the only purpose of the __match member, it is never directly used).
I believe that this trick works for all the uapi structs involved in this series. There is one exception, struct ebt_replace_kernel, which stores an array of kernel pointers. I doubt that this struct is ever manipulated from userspace though (i.e. its inclusion in a uapi header is probably a mistake), so leaving it unchanged should be safe.
I am frankly embarrassed to be suggesting another comprehensive change for that series, I should have realised earlier that this is the right approach to ensure that struct layouts are stable without a huge amount of churn. I'm happy to be posting a new version with that approach if you would prefer, I'd just ask you to do another round of testing.
Kevin
include/linux/netfilter.h | 6 + include/uapi/linux/netfilter.h | 8 + include/uapi/linux/netfilter/x_tables.h | 12 +- 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 | 7 +- 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 | 4 +- .../uapi/linux/netfilter_bridge/ebtables.h | 15 +- 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 | 143 ++++++++++-------- net/netfilter/xt_RATEEST.c | 12 +- net/netfilter/xt_TCPMSS.c | 2 +- net/netfilter/xt_TEE.c | 12 +- net/netfilter/xt_bpf.c | 19 +-- 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 | 13 +- 37 files changed, 327 insertions(+), 267 deletions(-)