On 06/11/2024 11:28, Joshua Lant wrote:
Hi Kevin,
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.
I cannot remember off-hand what exactly what was giving me grief, but it was indeed when trying to compile iptables. IIRC I originally tried fixing up the iptables code to cope with this scenario you describe, including one header in the other, but I could not get it to work so had to duplicate the declaration in both headers...
Fair enough, I trust that you tried your best - let's keep that duplication to avoid creating further headaches.
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
Interesting, I wonder if this is an artefact of the version of the xt_IDLETIMER patch you aplpied. If you notice on the v2 patches I messed up one of the commits (xt_IDLETIMER), and then made a v3 revision of the xt_IDLETIMER patch. But then named this whole patch series v3 rather than v4 as I thought that would be even more confusing... https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/... I wonder if that has anything to do with it?
I don't think so, in fact the patch you sent as a follow-up to v2 should build fine, the issue is with the patch in this series. This hunk for instance:
@@ -136,48 +136,50 @@ static int idletimer_check_sysfs_name(const char *name, unsigned int size) static int idletimer_tg_create(struct idletimer_tg_info *info) { int ret; + struct idletimer_tg *timer; - info->timer = kzalloc(sizeof(*info->timer), GFP_KERNEL); + info->timer = kzalloc(sizeof(struct idletimer_tg), GFP_KERNEL);
This won't build since info->timer is an integer (__nf_kptr_t) and kzalloc() returns a pointer. Either way all these complications should disappear with the new approach.
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):
Sorry yeah I have removed -Werror from my build...
Can't really blame you as this isn't enabled by default, I'd argue it should be but that is a separate topic :)
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
Don't worry about it. You say jump, I say how high ahahaha. I will make the changes. It feels very much like this is my cross to bear... I will hopefully have some time to do this at some point next week. Thanks again for your input. Let's hope that this is the last of this :P
I very much hope so as well... I really appreciate your patience once again!
Feel free to reorganise patches as you see fit (it might not be necessary to have one patch per header since the changes become much smaller, but this is really up to you). One more note: in patch 10, struct xt_hashlimit_info already has a union "u" for the ptr and master pointers, so there's no need to nest another union inside - just add an __nf_kptr_t member inside "u".
Kevin