Re: [PATCH iptables] nft: fix interface comparisons in `-C` commands

[Phil Sutter <phil@nwl.cc>]

Hi Jeremy,

On Mon, Nov 18, 2024 at 01:56:50PM +0000, Jeremy Sowden wrote:
[...]
> Remove the mask parameters from `is_same_interfaces`.  Add a test-case.

Thanks for the fix and test-case!

Some remarks below:

[...]
>  bool is_same_interfaces(const char *a_iniface, const char *a_outiface,
> -			unsigned const char *a_iniface_mask,
> -			unsigned const char *a_outiface_mask,
> -			const char *b_iniface, const char *b_outiface,
> -			unsigned const char *b_iniface_mask,
> -			unsigned const char *b_outiface_mask)
> +			const char *b_iniface, const char *b_outiface)
>  {
>  	int i;
>  
>  	for (i = 0; i < IFNAMSIZ; i++) {
> -		if (a_iniface_mask[i] != b_iniface_mask[i]) {
> -			DEBUGP("different iniface mask %x, %x (%d)\n",
> -			a_iniface_mask[i] & 0xff, b_iniface_mask[i] & 0xff, i);
> -			return false;
> -		}
> -		if ((a_iniface[i] & a_iniface_mask[i])
> -		    != (b_iniface[i] & b_iniface_mask[i])) {
> +		if (a_iniface[i] != b_iniface[i]) {
>  			DEBUGP("different iniface\n");
>  			return false;
>  		}
> -		if (a_outiface_mask[i] != b_outiface_mask[i]) {
> -			DEBUGP("different outiface mask\n");
> -			return false;
> -		}
> -		if ((a_outiface[i] & a_outiface_mask[i])
> -		    != (b_outiface[i] & b_outiface_mask[i])) {
> +		if (a_outiface[i] != b_outiface[i]) {
>  			DEBUGP("different outiface\n");
>  			return false;
>  		}

My draft fix converts this to strncmp() calls, I don't think we should
inspect bytes past the NUL-char. Usually we parse into a zeroed
iptables_command_state, but if_indextoname(3P) does not define output
buffer contents apart from "shall place in this buffer the name of the
interface", so it may put garbage in there (although unlikely).

Another thing is a potential follow-up: There are remains in
nft_arp_post_parse() and ipv6_post_parse(), needless filling of the mask
buffers. They may be dropped along with the now unused mask fields in
struct xtables_args.

WDYT?

Thanks, Phil