Re: [PATCH net 7/9] netfilter: arp_tables: remove the 32bit compat interface

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] The arptables 32-bit compat interface is only partially removed.

--

netfilter: arp_tables: remove the 32bit compat interface

This removes all arptables-specific 32-bit compat translation code from
net/ipv4/netfilter/arp_tables.c, drops struct compat_arpt_entry and its
accessor from include/linux/netfilter_arp/arp_tables.h, and updates the
NETFILTER_XTABLES_COMPAT help text to no longer mention arp.

> diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> index a87e07e80d0de..db307fa49f3f6 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c

[ ... ]

> @@ -811,23 +736,11 @@ static int get_info(struct net *net, void __user *user, const int *len)
>  		return -EFAULT;
>  
>  	name[XT_TABLE_MAXNAMELEN-1] = '\0';
> -#ifdef CONFIG_NETFILTER_XTABLES_COMPAT
> -	if (in_compat_syscall())
> -		xt_compat_lock(NFPROTO_ARP);
> -#endif
>  	t = xt_request_find_table_lock(net, NFPROTO_ARP, name);
>  	if (!IS_ERR(t)) {
>  		struct arpt_getinfo info;
>  		const struct xt_table_info *private = t->private;
> -#ifdef CONFIG_NETFILTER_XTABLES_COMPAT
> -		struct xt_table_info tmp;
>  
> -		if (in_compat_syscall()) {
> -			ret = compat_table_info(private, &tmp);
> -			xt_compat_flush_offsets(NFPROTO_ARP);
> -			private = &tmp;
> -		}
> -#endif
>  		memset(&info, 0, sizeof(info));
>  		info.valid_hooks = t->valid_hooks;
>  		memcpy(info.hook_entry, private->hook_entry,

[ ... ]

> @@ -1432,12 +981,7 @@ static int do_arpt_set_ctl(struct sock *sk, int cmd, sockptr_t arg,
>  
>  	switch (cmd) {
>  	case ARPT_SO_SET_REPLACE:
> -#ifdef CONFIG_NETFILTER_XTABLES_COMPAT
> -		if (in_compat_syscall())
> -			ret = compat_do_replace(sock_net(sk), arg, len);
> -		else
> -#endif
> -			ret = do_replace(sock_net(sk), arg, len);
> +		ret = do_replace(sock_net(sk), arg, len);
>  		break;
>  
>  	case ARPT_SO_SET_ADD_COUNTERS:

[Severity: Low]

Should the two arptables ctl handlers also stop accepting compat callers?
Both do_arpt_set_ctl() and do_arpt_get_ctl() still start with:

	if (!xt_compat_check())
		return -EPERM;

and that helper intentionally lets 32-bit callers continue when
CONFIG_NETFILTER_XTABLES_COMPAT is set:

include/linux/netfilter/x_tables.h:xt_compat_check() {
	if (!in_compat_syscall())
		return true;

	pr_warn_once("%s %s\n",
		     "xtables 32bit compat interface no longer supported",
		     "in namespaces and will be removed soon.");

	if (!capable(CAP_NET_ADMIN))
		return false;
#endif
	return true;
}

So a 32-bit task with CAP_NET_ADMIN now falls straight into the native
handlers, which gives three different behaviours:

ARPT_SO_GET_INFO succeeds and copies out the native xt_table_info values,
since the compat_table_info() rescaling above is gone:

		memcpy(info.hook_entry, private->hook_entry,
		       sizeof(info.hook_entry));
		memcpy(info.underflow, private->underflow,
		       sizeof(info.underflow));
		info.num_entries = private->number;
		info.size = private->size;

Are those offsets and size still meaningful to a caller that will walk them
using the 32-bit struct arpt_entry layout?

ARPT_SO_SET_REPLACE and ARPT_SO_GET_ENTRIES end up failing with -EINVAL
purely as a side effect of header size arithmetic in do_replace() and
get_entries() (76 vs 80 bytes for the replace header, 36 + size vs
40 + size for the get_entries header) rather than a defined unsupported
error.  Would an explicit rejection be clearer here, for example:

	if (in_compat_syscall())
		return -EOPNOTSUPP;

ARPT_SO_SET_ADD_COUNTERS keeps a working 32-bit ABI, because
do_add_counters() -> xt_copy_counters() still carries its own compat branch
that converts compat_xt_counters_info for every family, NFPROTO_ARP
included:

net/netfilter/x_tables.c:xt_copy_counters() {
#ifdef CONFIG_NETFILTER_XTABLES_COMPAT
	if (in_compat_syscall()) {
		/* structures only differ in size due to alignment */
		struct compat_xt_counters_info compat_tmp;
	...
}

Does "remove the 32bit compat interface" then hold for that sockopt?  If
leaving the shared counter path compat-capable until the other three
flavors are converted is intended, could the commit message say so?

One more asymmetry: the ip_tables, ip6_tables and ebtables ctl handlers
still branch on in_compat_syscall() after the same xt_compat_check() call,
so the shared helper now means "translate" for those flavors and "warn,
then run the native path" for arptables.  Along the same lines, a 32-bit
task without CAP_NET_ADMIN now gets -EPERM plus a warning about a compat
interface that no longer exists for this flavor.

> @@ -1466,12 +1010,7 @@ static int do_arpt_get_ctl(struct sock *sk, int cmd, void __user *user, int *len
>  		break;
>  
>  	case ARPT_SO_GET_ENTRIES:
> -#ifdef CONFIG_NETFILTER_XTABLES_COMPAT
> -		if (in_compat_syscall())
> -			ret = compat_get_entries(sock_net(sk), user, len);
> -		else
> -#endif
> -			ret = get_entries(sock_net(sk), user, len);
> +		ret = get_entries(sock_net(sk), user, len);
>  		break;
>  
>  	case ARPT_SO_GET_REVISION_TARGET: {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907171732.1407739-2-pablo%40netfilter.org