Hi Kevin,
Sorry I know I said that I would get a spare moment to do this work over a month ago, but the University of Manchester's elves are very busy this time of year! Here are the changes you requested...
Please let me know if there are issues building. I still don't much understand why that last series had a problem.
Changes from v3: - [XX/05] Modified the use of __nf_kptr_t in the xtables plugin structs to use a union, with the original struct as a member. This trick allows for removal of the heavy casting in the kernel which was required in the earlier version. - [XX/05] Squashed many of the commits (those from the xtables plugin header files) into a single commit, since each individual commit now makes far fewer changes.
Testing: - Tested with purecap iptables tests (nftables only), passing 65/68 tests. Those which fail are expected to fail at this point, due primarily to improperly written test cases, or missing versions of userspace tooling.
Joshua Lant (5): netfilter: Create new type for kernel pointers. x_tables.h: pointers to unions in uapi struct xt plugins: pointers to unions in uapi struct ebtables: pointers to unions in uapi struct xtables: move include to headers
include/linux/netfilter.h | 6 +++++ include/uapi/linux/netfilter.h | 8 ++++++ include/uapi/linux/netfilter/x_tables.h | 18 +++++++++++-- include/uapi/linux/netfilter/xt_CT.h | 10 +++++-- include/uapi/linux/netfilter/xt_IDLETIMER.h | 12 +++++++-- include/uapi/linux/netfilter/xt_RATEEST.h | 6 ++++- include/uapi/linux/netfilter/xt_TEE.h | 6 ++++- include/uapi/linux/netfilter/xt_bpf.h | 13 +++++++-- include/uapi/linux/netfilter/xt_connlimit.h | 6 ++++- include/uapi/linux/netfilter/xt_hashlimit.h | 24 ++++++++++++++--- include/uapi/linux/netfilter/xt_limit.h | 6 ++++- include/uapi/linux/netfilter/xt_nfacct.h | 12 +++++++-- include/uapi/linux/netfilter/xt_quota.h | 6 ++++- include/uapi/linux/netfilter/xt_rateest.h | 9 +++++-- include/uapi/linux/netfilter/xt_statistic.h | 7 ++++- .../uapi/linux/netfilter_bridge/ebtables.h | 27 +++++++++++++++---- net/netfilter/xt_bpf.c | 1 - net/netfilter/xt_statistic.c | 1 - 18 files changed, 149 insertions(+), 29 deletions(-)
In the netfilter subsystem there are many structs inside the UAPI headers which contain kernel pointers. This causes misalignment between userspace and kernel space of these structures when using the PCuABI, due to the differing sizes of the pointer in kernel space, and the capability in userspace. This leads to issues with some netlink messages sent between user/kernel space.
In order to fix this, we require a new type which is the same size from the perspective of both the user and the kernel, and is able to hold a kernel pointer regardless of the ABI which is being used (non-purecap, hybrid pc-userspace/non-pc kernel, or a fully purecap kernel and userpsace).
In order to achieve this we create a new type which is the size of a capability when using CONFIG_CHERI_PURECAP_UABI, and the size of a pointer if not. We will then replace the kernel pointers in the UAPI structs with this new type. Doing this will maintain consistent struct sizing, even if a fully purecap kernel is used.
The drawback to this approach is that this method is unnecessary in the case where we have a purecap userspace and non-purecap kernel, since the kernel pointer in the structs are never actually touched by userspace, and so are not required to be the size of a capability. However, it is necessary to guarantee that the UAPI remains stable no matter what the kernel ABI uses.
The use of x_tables.h and netfilter.h appears to be mutually exclusive within the kernel.This is why it is required to add the new definition in both netfilter.h and x_tables.h. Since many conflicts between definitions exist when including netfilter.h in x_tables.h. This overlap is presumably is an artefact of xtables being superseded by nftables.
Signed-off-by: Joshua Lant joshualant@gmail.com --- include/linux/netfilter.h | 6 ++++++ include/uapi/linux/netfilter.h | 8 ++++++++ include/uapi/linux/netfilter/x_tables.h | 8 ++++++++ 3 files changed, 22 insertions(+)
diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h index 80900d910992..3041b4b78be9 100644 --- a/include/linux/netfilter.h +++ b/include/linux/netfilter.h @@ -17,6 +17,12 @@ #include <linux/sockptr.h> #include <net/net_namespace.h>
+#ifdef CONFIG_CHERI_PURECAP_UABI +typedef __uintcap_t __nf_kptr_t; +#else +typedef unsigned long __nf_kptr_t; +#endif + static inline int NF_DROP_GETERR(int verdict) { return -(verdict >> NF_VERDICT_QBITS); diff --git a/include/uapi/linux/netfilter.h b/include/uapi/linux/netfilter.h index 5a79ccb76701..bb793b70d64c 100644 --- a/include/uapi/linux/netfilter.h +++ b/include/uapi/linux/netfilter.h @@ -7,6 +7,14 @@ #include <linux/in.h> #include <linux/in6.h>
+#ifndef __KERNEL__ +#ifdef __CHERI_PURE_CAPABILITY__ +typedef __uintcap_t __nf_kptr_t; +#else +typedef unsigned long __nf_kptr_t; +#endif +#endif + /* Responses from hook functions. */ #define NF_DROP 0 #define NF_ACCEPT 1 diff --git a/include/uapi/linux/netfilter/x_tables.h b/include/uapi/linux/netfilter/x_tables.h index 796af83a963a..e695b6d34a6f 100644 --- a/include/uapi/linux/netfilter/x_tables.h +++ b/include/uapi/linux/netfilter/x_tables.h @@ -8,6 +8,14 @@ #define XT_EXTENSION_MAXNAMELEN 29 #define XT_TABLE_MAXNAMELEN 32
+#ifndef __KERNEL__ +#ifdef __CHERI_PURE_CAPABILITY__ +typedef __uintcap_t __nf_kptr_t; +#else +typedef unsigned long __nf_kptr_t; +#endif +#endif + struct xt_entry_match { union { struct {
On morello architecture, use of kernel pointers in the uapi structures is not permitted, due to different alignment requirements between kernel and userspace. These pointers should be the new type __nf_kptr_t, whose size changes depening on the pc/non-pcUABI. Modify these to use a union which will be accessed in the kernel using the original member pointer, but will actually be of size __nf_kptr_t, avoiding heavy casting needed when using to new type directly.
Signed-off-by: Joshua Lant joshualant@gmail.com --- include/uapi/linux/netfilter/x_tables.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/netfilter/x_tables.h b/include/uapi/linux/netfilter/x_tables.h index e695b6d34a6f..fe081b3a5293 100644 --- a/include/uapi/linux/netfilter/x_tables.h +++ b/include/uapi/linux/netfilter/x_tables.h @@ -29,7 +29,10 @@ struct xt_entry_match { __u16 match_size;
/* Used inside the kernel */ - struct xt_match *match; + union { + struct xt_match *match; + __nf_kptr_t __match; + }; } kernel;
/* Total length */ @@ -52,7 +55,10 @@ struct xt_entry_target { __u16 target_size;
/* Used inside the kernel */ - struct xt_target *target; + union { + struct xt_target *target; + __nf_kptr_t __target; + }; } kernel;
/* Total length */
On morello architecture, use of kernel pointers in the uapi structures is not permitted, due to different alignment requirements between kernel and userspace. These pointers should be the new type __nf_kptr_t, whose size changes depening on the pc/non-pcUABI. Modify these to use a union which will be accessed in the kernel using the original member pointer, but will actually be of size __nf_kptr_t, avoiding heavy casting needed when using to new type directly.
Signed-off-by: Joshua Lant joshualant@gmail.com --- include/uapi/linux/netfilter/xt_CT.h | 10 +++++++-- include/uapi/linux/netfilter/xt_IDLETIMER.h | 12 +++++++++-- include/uapi/linux/netfilter/xt_RATEEST.h | 6 +++++- include/uapi/linux/netfilter/xt_TEE.h | 6 +++++- include/uapi/linux/netfilter/xt_bpf.h | 12 +++++++++-- include/uapi/linux/netfilter/xt_connlimit.h | 6 +++++- include/uapi/linux/netfilter/xt_hashlimit.h | 24 +++++++++++++++++---- include/uapi/linux/netfilter/xt_limit.h | 6 +++++- include/uapi/linux/netfilter/xt_nfacct.h | 12 +++++++++-- include/uapi/linux/netfilter/xt_quota.h | 6 +++++- include/uapi/linux/netfilter/xt_rateest.h | 9 ++++++-- include/uapi/linux/netfilter/xt_statistic.h | 6 +++++- 12 files changed, 95 insertions(+), 20 deletions(-)
diff --git a/include/uapi/linux/netfilter/xt_CT.h b/include/uapi/linux/netfilter/xt_CT.h index 868fa08e1fbb..3fd5e53d7627 100644 --- a/include/uapi/linux/netfilter/xt_CT.h +++ b/include/uapi/linux/netfilter/xt_CT.h @@ -24,7 +24,10 @@ struct xt_ct_target_info { char helper[16];
/* Used internally by the kernel */ - struct nf_conn *ct __attribute__((aligned(8))); + union { + struct nf_conn *ct __attribute__((aligned(8))); + __nf_kptr_t __ct __attribute__((aligned(8))); + }; };
struct xt_ct_target_info_v1 { @@ -36,7 +39,10 @@ struct xt_ct_target_info_v1 { char timeout[32];
/* Used internally by the kernel */ - struct nf_conn *ct __attribute__((aligned(8))); + union { + struct nf_conn *ct __attribute__((aligned(8))); + __nf_kptr_t __ct __attribute__((aligned(8))); + }; };
#endif /* _XT_CT_H */ diff --git a/include/uapi/linux/netfilter/xt_IDLETIMER.h b/include/uapi/linux/netfilter/xt_IDLETIMER.h index 7bfb31a66fc9..81c3aedaae13 100644 --- a/include/uapi/linux/netfilter/xt_IDLETIMER.h +++ b/include/uapi/linux/netfilter/xt_IDLETIMER.h @@ -25,7 +25,11 @@ struct idletimer_tg_info { char label[MAX_IDLETIMER_LABEL_SIZE];
/* for kernel module internal use only */ - struct idletimer_tg *timer __attribute__((aligned(8))); + /* corresponds to the idletimer_tg struct */ + union { + struct idletimer_tg *timer __attribute__((aligned(8))); + __nf_kptr_t __timer __attribute__((aligned(8))); + }; };
struct idletimer_tg_info_v1 { @@ -37,6 +41,10 @@ struct idletimer_tg_info_v1 { __u8 timer_type;
/* for kernel module internal use only */ - struct idletimer_tg *timer __attribute__((aligned(8))); + /* corresponds to the idletimer_tg struct */ + union { + struct idletimer_tg *timer __attribute__((aligned(8))); + __nf_kptr_t __timer __attribute__((aligned(8))); + }; }; #endif diff --git a/include/uapi/linux/netfilter/xt_RATEEST.h b/include/uapi/linux/netfilter/xt_RATEEST.h index 2b87a71e6266..eae8ea1004a8 100644 --- a/include/uapi/linux/netfilter/xt_RATEEST.h +++ b/include/uapi/linux/netfilter/xt_RATEEST.h @@ -11,7 +11,11 @@ struct xt_rateest_target_info { __u8 ewma_log;
/* Used internally by the kernel */ - struct xt_rateest *est __attribute__((aligned(8))); + /* Corresponds to struct xt_rateest */ + union { + struct xt_rateest *est __attribute__((aligned(8))); + __nf_kptr_t __est __attribute__((aligned(8))); + }; };
#endif /* _XT_RATEEST_TARGET_H */ diff --git a/include/uapi/linux/netfilter/xt_TEE.h b/include/uapi/linux/netfilter/xt_TEE.h index eb854917f828..f46019a1fc03 100644 --- a/include/uapi/linux/netfilter/xt_TEE.h +++ b/include/uapi/linux/netfilter/xt_TEE.h @@ -9,7 +9,11 @@ struct xt_tee_tginfo { char oif[16];
/* used internally by the kernel */ - struct xt_tee_priv *priv __attribute__((aligned(8))); + /* Corresponds to struct xt_tee_priv */ + union { + struct xt_tee_priv *priv __attribute__((aligned(8))); + __nf_kptr_t __priv __attribute__((aligned(8))); + }; };
#endif /* _XT_TEE_TARGET_H */ diff --git a/include/uapi/linux/netfilter/xt_bpf.h b/include/uapi/linux/netfilter/xt_bpf.h index a05adda26d3e..3d2c08de0b08 100644 --- a/include/uapi/linux/netfilter/xt_bpf.h +++ b/include/uapi/linux/netfilter/xt_bpf.h @@ -16,7 +16,11 @@ struct xt_bpf_info { struct sock_filter bpf_program[XT_BPF_MAX_NUM_INSTR];
/* only used in the kernel */ - struct bpf_prog *filter __attribute__((aligned(8))); + /* Corresponds to the bpf_prog* struct */ + union { + struct bpf_prog *filter __attribute__((aligned(8))); + __nf_kptr_t __filter __attribute__((aligned(8))); + }; };
enum xt_bpf_modes { @@ -36,7 +40,11 @@ struct xt_bpf_info_v1 { };
/* only used in the kernel */ - struct bpf_prog *filter __attribute__((aligned(8))); + /* Corresponds to the bpf_prog* struct */ + union { + struct bpf_prog *filter __attribute__((aligned(8))); + __nf_kptr_t __filter __attribute__((aligned(8))); + }; };
#endif /*_XT_BPF_H */ diff --git a/include/uapi/linux/netfilter/xt_connlimit.h b/include/uapi/linux/netfilter/xt_connlimit.h index d4d1943dcd11..57459493d292 100644 --- a/include/uapi/linux/netfilter/xt_connlimit.h +++ b/include/uapi/linux/netfilter/xt_connlimit.h @@ -27,7 +27,11 @@ struct xt_connlimit_info { __u32 flags;
/* Used internally by the kernel */ - struct nf_conncount_data *data __attribute__((aligned(8))); + /* Corresponds to the struct nf_conncount_data * */ + union { + struct nf_conncount_data *data __attribute__((aligned(8))); + __nf_kptr_t __data __attribute__((aligned(8))); + }; };
#endif /* _XT_CONNLIMIT_H */ diff --git a/include/uapi/linux/netfilter/xt_hashlimit.h b/include/uapi/linux/netfilter/xt_hashlimit.h index 721a8de6c5b3..81ee6fd5ae3c 100644 --- a/include/uapi/linux/netfilter/xt_hashlimit.h +++ b/include/uapi/linux/netfilter/xt_hashlimit.h @@ -46,10 +46,14 @@ struct xt_hashlimit_info { struct hashlimit_cfg cfg;
/* Used internally by the kernel */ - struct xt_hashlimit_htable *hinfo; + /* Corresponds to struct xt_hashlimit_htable * */ + __nf_kptr_t hinfo; union { + /* Corresponds to struct xt_hashlimit_info *, or generic void ptr */ void *ptr; + __nf_kptr_t __ptr; struct xt_hashlimit_info *master; + __nf_kptr_t __master; } u; };
@@ -101,7 +105,11 @@ struct xt_hashlimit_mtinfo1 { struct hashlimit_cfg1 cfg;
/* Used internally by the kernel */ - struct xt_hashlimit_htable *hinfo __attribute__((aligned(8))); + /* Corresponds to struct xt_hashlimit_htable * */ + union { + struct xt_hashlimit_htable *hinfo __attribute__((aligned(8))); + __nf_kptr_t __hinfo __attribute__((aligned(8))); + }; };
struct xt_hashlimit_mtinfo2 { @@ -109,7 +117,11 @@ struct xt_hashlimit_mtinfo2 { struct hashlimit_cfg2 cfg;
/* Used internally by the kernel */ - struct xt_hashlimit_htable *hinfo __attribute__((aligned(8))); + /* Corresponds to struct xt_hashlimit_htable * */ + union { + struct xt_hashlimit_htable *hinfo __attribute__((aligned(8))); + __nf_kptr_t __hinfo __attribute__((aligned(8))); + }; };
struct xt_hashlimit_mtinfo3 { @@ -117,7 +129,11 @@ struct xt_hashlimit_mtinfo3 { struct hashlimit_cfg3 cfg;
/* Used internally by the kernel */ - struct xt_hashlimit_htable *hinfo __attribute__((aligned(8))); + /* Corresponds to struct xt_hashlimit_htable * */ + union { + struct xt_hashlimit_htable *hinfo __attribute__((aligned(8))); + __nf_kptr_t __hinfo __attribute__((aligned(8))); + }; };
#endif /* _UAPI_XT_HASHLIMIT_H */ diff --git a/include/uapi/linux/netfilter/xt_limit.h b/include/uapi/linux/netfilter/xt_limit.h index 1d6e4ce9a646..adfe83e6f784 100644 --- a/include/uapi/linux/netfilter/xt_limit.h +++ b/include/uapi/linux/netfilter/xt_limit.h @@ -20,6 +20,10 @@ struct xt_rateinfo { __u32 credit; /* moved to xt_limit_priv */ __u32 credit_cap, cost;
- struct xt_limit_priv *master; + /* Corresponds to the xt_limit_priv* struct */ + union { + struct xt_limit_priv *master; + __nf_kptr_t __master; + }; }; #endif /*_XT_RATE_H*/ diff --git a/include/uapi/linux/netfilter/xt_nfacct.h b/include/uapi/linux/netfilter/xt_nfacct.h index b5123ab8d54a..2ff8c34519b2 100644 --- a/include/uapi/linux/netfilter/xt_nfacct.h +++ b/include/uapi/linux/netfilter/xt_nfacct.h @@ -8,12 +8,20 @@ struct nf_acct;
struct xt_nfacct_match_info { char name[NFACCT_NAME_MAX]; - struct nf_acct *nfacct; + /* Corresponds to the nf_acct* struct */ + union { + struct nf_acct *nfacct; + __nf_kptr_t __nfacct; + }; };
struct xt_nfacct_match_info_v1 { char name[NFACCT_NAME_MAX]; - struct nf_acct *nfacct __attribute__((aligned(8))); + /* Corresponds to the nf_acct* struct */ + union { + struct nf_acct *nfacct __attribute__((aligned(8))); + __nf_kptr_t __nfacct __attribute__((aligned(8))); + }; };
#endif /* _XT_NFACCT_MATCH_H */ diff --git a/include/uapi/linux/netfilter/xt_quota.h b/include/uapi/linux/netfilter/xt_quota.h index f3ba5d9e58b6..6cb962e3806f 100644 --- a/include/uapi/linux/netfilter/xt_quota.h +++ b/include/uapi/linux/netfilter/xt_quota.h @@ -17,7 +17,11 @@ struct xt_quota_info { __aligned_u64 quota;
/* Used internally by the kernel */ - struct xt_quota_priv *master; + /* Corresponds to xt_quota_priv* */ + union { + struct xt_quota_priv *master; + __nf_kptr_t __master; + }; };
#endif /* _XT_QUOTA_H */ diff --git a/include/uapi/linux/netfilter/xt_rateest.h b/include/uapi/linux/netfilter/xt_rateest.h index 52a37bdc1837..4db78e9d887b 100644 --- a/include/uapi/linux/netfilter/xt_rateest.h +++ b/include/uapi/linux/netfilter/xt_rateest.h @@ -32,8 +32,13 @@ struct xt_rateest_match_info { __u32 pps2;
/* Used internally by the kernel */ - struct xt_rateest *est1 __attribute__((aligned(8))); - struct xt_rateest *est2 __attribute__((aligned(8))); + /* Corresponds to struct xt_rateest* */ + union { + struct xt_rateest *est1 __attribute__((aligned(8))); + __nf_kptr_t __est1 __attribute__((aligned(8))); + struct xt_rateest *est2 __attribute__((aligned(8))); + __nf_kptr_t __est2 __attribute__((aligned(8))); + }; };
#endif /* _XT_RATEEST_MATCH_H */ diff --git a/include/uapi/linux/netfilter/xt_statistic.h b/include/uapi/linux/netfilter/xt_statistic.h index bbce6fcb26e3..9d1bfa6ce662 100644 --- a/include/uapi/linux/netfilter/xt_statistic.h +++ b/include/uapi/linux/netfilter/xt_statistic.h @@ -31,7 +31,11 @@ struct xt_statistic_info { __u32 count; /* unused */ } nth; } u; - struct xt_statistic_priv *master __attribute__((aligned(8))); + /* Corresponds to struct xt_statistic_priv * */ + union { + struct xt_statistic_priv *master __attribute__((aligned(8))); + __nf_kptr_t __master __attribute__((aligned(8))); + }; };
#endif /* _XT_STATISTIC_H */
On morello architecture, use of kernel pointers in the uapi structures is not permitted, due to different alignment requirements between kernel and userspace. These pointers should be the new type __nf_kptr_t, whose size changes depening on the pc/non-pcUABI. Modify these to use a union which will be accessed in the kernel using the original member pointer, but will actually be of size __nf_kptr_t, avoiding heavy casting needed when using to new type directly.
Signed-off-by: Joshua Lant joshualant@gmail.com --- .../uapi/linux/netfilter_bridge/ebtables.h | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/netfilter_bridge/ebtables.h b/include/uapi/linux/netfilter_bridge/ebtables.h index 4ff328f3d339..2491b4acd590 100644 --- a/include/uapi/linux/netfilter_bridge/ebtables.h +++ b/include/uapi/linux/netfilter_bridge/ebtables.h @@ -65,11 +65,16 @@ struct ebt_replace_kernel { /* total size of the entries */ unsigned int entries_size; /* start of the chains */ - struct ebt_entries *hook_entry[NF_BR_NUMHOOKS]; + /* Corresponds to struct ebt_entries * */ + __nf_kptr_t hook_entry[NF_BR_NUMHOOKS]; /* nr of counters userspace expects back */ unsigned int num_counters; /* where the kernel will put the old counters */ - struct ebt_counter *counters; + /* Corresponds to struct ebt_counter * */ + union { + struct ebt_counter *counters; + __nf_kptr_t __counters; + }; char *entries; };
@@ -125,7 +130,11 @@ struct ebt_entry_match { char name[EBT_EXTENSION_MAXNAMELEN]; __u8 revision; }; - struct xt_match *match; + /* Corresponds to struct xt_match * */ + union { + struct xt_match *match; + __nf_kptr_t __match; + }; } u; /* size of data */ unsigned int match_size; @@ -138,7 +147,11 @@ struct ebt_entry_watcher { char name[EBT_EXTENSION_MAXNAMELEN]; __u8 revision; }; - struct xt_target *watcher; + /* Corresponds to struct xt_target * */ + union { + struct xt_target *watcher; + __nf_kptr_t __watcher; + }; } u; /* size of data */ unsigned int watcher_size; @@ -151,7 +164,11 @@ struct ebt_entry_target { char name[EBT_EXTENSION_MAXNAMELEN]; __u8 revision; }; - struct xt_target *target; + /* Corresponds to struct xt_target * */ + union { + struct xt_target *target; + __nf_kptr_t __target; + }; } u; /* size of data */ unsigned int target_size;
On 11/12/2024 17:17, Joshua Lant wrote:
diff --git a/include/uapi/linux/netfilter_bridge/ebtables.h b/include/uapi/linux/netfilter_bridge/ebtables.h index 4ff328f3d339..2491b4acd590 100644 --- a/include/uapi/linux/netfilter_bridge/ebtables.h +++ b/include/uapi/linux/netfilter_bridge/ebtables.h @@ -65,11 +65,16 @@ struct ebt_replace_kernel { /* total size of the entries */ unsigned int entries_size; /* start of the chains */
- struct ebt_entries *hook_entry[NF_BR_NUMHOOKS];
- /* Corresponds to struct ebt_entries * */
- __nf_kptr_t hook_entry[NF_BR_NUMHOOKS];
As I mentioned in my reply to v3, this is the one case where we can't transparently pad entries (this doesn't build: kernel code like in net/bridge/netfilter/ebtable_broute.c expects hook_entry to contain pointers, not integers). What I was wondering is whether we need to touch this struct at all, because surely it is never used by userspace and should never have appeared in a uapi header? If this assumption sounds reasonable to you I'll drop the changes to this struct.
There is another issue in patch 3 (struct xt_rateest_match_info::{est1,est2} are distinct members and so must be in separate unions), and patch 5 needs to be extended to #include the header in all modified uapi headers, but these are both trivial and I can take care of them too.
By the way I expect that the reason that you are missing various compiler errors is simply that you're not enabling many netfilter options, and as a result not building most of the netfilter files. I tried building those patches with make allyesconfig, that should provide (almost) full coverage.
- Kevin
Hi Kevin,
On 11/12/2024 17:17, Joshua Lant wrote:
diff --git a/include/uapi/linux/netfilter_bridge/ebtables.h b/include/uapi/linux/netfilter_bridge/ebtables.h index 4ff328f3d339..2491b4acd590 100644 --- a/include/uapi/linux/netfilter_bridge/ebtables.h +++ b/include/uapi/linux/netfilter_bridge/ebtables.h @@ -65,11 +65,16 @@ struct ebt_replace_kernel { /* total size of the entries */ unsigned int entries_size; /* start of the chains */
- struct ebt_entries *hook_entry[NF_BR_NUMHOOKS];
- /* Corresponds to struct ebt_entries * */
- __nf_kptr_t hook_entry[NF_BR_NUMHOOKS];
As I mentioned in my reply to v3, this is the one case where we can't transparently pad entries (this doesn't build: kernel code like in net/bridge/netfilter/ebtable_broute.c expects hook_entry to contain pointers, not integers). What I was wondering is whether we need to touch this struct at all, because surely it is never used by userspace and should never have appeared in a uapi header? If this assumption sounds reasonable to you I'll drop the changes to this struct.
Apologies. In my build these are giving me warnings rather than errors, and I did not spot them... I think that this is a somewhat reasonable assumption... There is a function in the ebtables userspace tool "translate_user2kernel". I am unsure its exact workings, but it uses the ebt_replace struct, which looks to be a carbon copy of the ebt_replace_kernel struct. You are right that it is likely not intended to be in the uapi headers. However, there may be an issue with the kernel reading netlink messages incorrectly if the userspace writes ebt_replace structs (with capabilities), and the kernel is trying to read corresponding ebt_replace_kernel structs with pointers. But at any rate I think this issue can be ignored since it only occurs in the legacy ebtables package, which is now deprecated, and certainly not ported to purecap.
There is another issue in patch 3 (struct xt_rateest_match_info::{est1,est2} are distinct members and so must be in separate unions), and patch 5 needs to be extended to #include the header in all modified uapi headers, but these are both trivial and I can take care of them too.
Okay, I have fixed up the patches and will post them.
By the way I expect that the reason that you are missing various compiler errors is simply that you're not enabling many netfilter options, and as a result not building most of the netfilter files. I tried building those patches with make allyesconfig, that should provide (almost) full coverage.
This is interesting. I was unable to compile using allyesconfig. I dont know if this is due to my flow (using Yocto) or what? I got all sorts of strange errors when building like this. Mainly due to it assuming that GCC should be used for building. Anyway, I simply took all of the netfilter/networking flags and turned all of them on to achieve the same result.
Cheers,
Josh
On 12/02/2025 11:54, Joshua Lant wrote:
Hi Kevin,
On 11/12/2024 17:17, Joshua Lant wrote:
diff --git a/include/uapi/linux/netfilter_bridge/ebtables.h b/include/uapi/linux/netfilter_bridge/ebtables.h index 4ff328f3d339..2491b4acd590 100644 --- a/include/uapi/linux/netfilter_bridge/ebtables.h +++ b/include/uapi/linux/netfilter_bridge/ebtables.h @@ -65,11 +65,16 @@ struct ebt_replace_kernel { /* total size of the entries */ unsigned int entries_size; /* start of the chains */
- struct ebt_entries *hook_entry[NF_BR_NUMHOOKS];
- /* Corresponds to struct ebt_entries * */
- __nf_kptr_t hook_entry[NF_BR_NUMHOOKS];
As I mentioned in my reply to v3, this is the one case where we can't transparently pad entries (this doesn't build: kernel code like in net/bridge/netfilter/ebtable_broute.c expects hook_entry to contain pointers, not integers). What I was wondering is whether we need to touch this struct at all, because surely it is never used by userspace and should never have appeared in a uapi header? If this assumption sounds reasonable to you I'll drop the changes to this struct.
Apologies. In my build these are giving me warnings rather than errors, and I did not spot them... I think that this is a somewhat reasonable assumption... There is a function in the ebtables userspace tool "translate_user2kernel". I am unsure its exact workings, but it uses the ebt_replace struct, which looks to be a carbon copy of the ebt_replace_kernel struct. You are right that it is likely not intended to be in the uapi headers. However, there may be an issue with the kernel reading netlink messages incorrectly if the userspace writes ebt_replace structs (with capabilities), and the kernel is trying to read corresponding ebt_replace_kernel structs with pointers. But at any rate I think this issue can be ignored since it only occurs in the legacy ebtables package, which is now deprecated, and certainly not ported to purecap.
Fair enough, I think it's reasonable to ignore this struct then.
There is another issue in patch 3 (struct xt_rateest_match_info::{est1,est2} are distinct members and so must be in separate unions), and patch 5 needs to be extended to #include the header in all modified uapi headers, but these are both trivial and I can take care of them too.
Okay, I have fixed up the patches and will post them.
Thank you!
By the way I expect that the reason that you are missing various compiler errors is simply that you're not enabling many netfilter options, and as a result not building most of the netfilter files. I tried building those patches with make allyesconfig, that should provide (almost) full coverage.
This is interesting. I was unable to compile using allyesconfig. I dont know if this is due to my flow (using Yocto) or what? I got all sorts of strange errors when building like this. Mainly due to it assuming that GCC should be used for building. Anyway, I simply took all of the netfilter/networking flags and turned all of them on to achieve the same result.
Just to be clear, I didn't enable PCuABI when building allyesconfig, that definitely won't work. Either way enabling the netfilter/networking configs manually should be good enough.
- Kevin
Just to be clear, I didn't enable PCuABI when building allyesconfig, that definitely won't work. Either way enabling the netfilter/networking configs manually should be good enough.
- Kevin
Aaaaaaah right okay. I was unable to compile immedaitely without PCuABI either... When compiling without pcuabi (not using allyesconfig), I have to also remove CONFIG_COMPAT, due to issues relating to vdso (sorry i dont have exact error messages to hand). Is this expected?
Cheers,
Josh
On 13/02/2025 11:57, Joshua Lant wrote:
Just to be clear, I didn't enable PCuABI when building allyesconfig, that definitely won't work. Either way enabling the netfilter/networking configs manually should be good enough.
- Kevin
Aaaaaaah right okay. I was unable to compile immedaitely without PCuABI either... When compiling without pcuabi (not using allyesconfig), I have to also remove CONFIG_COMPAT, due to issues relating to vdso (sorry i dont have exact error messages to hand). Is this expected?
That's rather strange because CONFIG_COMPAT_VDSO is only enabled if CROSS_COMPILE_COMPAT is defined. Leaving that undefined, CONFIG_COMPAT causes no issue without PCuABI for me.
- Kevin
In order to have the __nf_kptr_t type included, we need to move xtables.h include into the headers rather than the c files.
Signed-off-by: Joshua Lant joshualant@gmail.com --- include/uapi/linux/netfilter/xt_bpf.h | 1 + include/uapi/linux/netfilter/xt_statistic.h | 1 + net/netfilter/xt_bpf.c | 1 - net/netfilter/xt_statistic.c | 1 - 4 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/netfilter/xt_bpf.h b/include/uapi/linux/netfilter/xt_bpf.h index 3d2c08de0b08..343fe99451d2 100644 --- a/include/uapi/linux/netfilter/xt_bpf.h +++ b/include/uapi/linux/netfilter/xt_bpf.h @@ -5,6 +5,7 @@ #include <linux/filter.h> #include <linux/limits.h> #include <linux/types.h> +#include <linux/netfilter/x_tables.h>
#define XT_BPF_MAX_NUM_INSTR 64 #define XT_BPF_PATH_MAX (XT_BPF_MAX_NUM_INSTR * sizeof(struct sock_filter)) diff --git a/include/uapi/linux/netfilter/xt_statistic.h b/include/uapi/linux/netfilter/xt_statistic.h index 9d1bfa6ce662..e609895d8563 100644 --- a/include/uapi/linux/netfilter/xt_statistic.h +++ b/include/uapi/linux/netfilter/xt_statistic.h @@ -2,6 +2,7 @@ #ifndef _XT_STATISTIC_H #define _XT_STATISTIC_H
+#include <linux/netfilter/x_tables.h> #include <linux/types.h>
enum xt_statistic_mode { diff --git a/net/netfilter/xt_bpf.c b/net/netfilter/xt_bpf.c index 849ac552a154..0b7cdc9380bb 100644 --- a/net/netfilter/xt_bpf.c +++ b/net/netfilter/xt_bpf.c @@ -13,7 +13,6 @@ #include <linux/bpf.h>
#include <linux/netfilter/xt_bpf.h> -#include <linux/netfilter/x_tables.h>
MODULE_AUTHOR("Willem de Bruijn willemb@google.com"); MODULE_DESCRIPTION("Xtables: BPF filter match"); diff --git a/net/netfilter/xt_statistic.c b/net/netfilter/xt_statistic.c index b26c1dcfc27b..ee75dd56835c 100644 --- a/net/netfilter/xt_statistic.c +++ b/net/netfilter/xt_statistic.c @@ -12,7 +12,6 @@ #include <linux/slab.h>
#include <linux/netfilter/xt_statistic.h> -#include <linux/netfilter/x_tables.h> #include <linux/module.h>
struct xt_statistic_priv {
linux-morello@op-lists.linaro.org
-
Joshua Lant -
Kevin Brodsky