Re: [PATCH] netfilter: IDLETIMER target v1 - match Android layout

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Tue, Mar 31, 2020 at 09:35:59AM -0700, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
> 
> Android has long had an extension to IDLETIMER to send netlink
> messages to userspace, see:
>   https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/include/uapi/linux/netfilter/xt_IDLETIMER.h#42
> Note: this is idletimer target rev 1, there is no rev 0 in
> the Android common kernel sources, see registration at:
>   https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/net/netfilter/xt_IDLETIMER.c#483
> 
> When we compare that to upstream's new idletimer target rev 1:
>   https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git/tree/include/uapi/linux/netfilter/xt_IDLETIMER.h#n46
> 
> We immediately notice that these two rev 1 structs are the
> same size and layout, and that while timer_type and send_nl_msg
> are differently named and serve a different purpose, they're
> at the same offset.
> 
> This makes them impossible to tell apart - and thus one cannot
> know in a mixed Android/vanilla environment whether one means
> timer_type or send_nl_msg.
> 
> Since this is iptables/netfilter uapi it introduces a problem
> between iptables (vanilla vs Android) userspace and kernel
> (vanilla vs Android) if the two don't match each other.
> 
> Additionally when at some point in the future Android picks up
> 5.7+ it's not at all clear how to resolve the resulting merge
> conflict.
> 
> Furthermore, since upgrading the kernel on old Android phones
> is pretty much impossible there does not seem to be an easy way
> out of this predicament.
> 
> The only thing I've been able to come up with is some super
> disgusting kernel version >= 5.7 check in the iptables binary
> to flip between different struct layouts.
> 
> By adding a dummy field to the vanilla Linux kernel header file
> we can force the two structs to be compatible with each other.
> 
> Long term I think I would like to deprecate send_nl_msg out of
> Android entirely, but I haven't quite been able to figure out
> exactly how we depend on it.  It seems to be very similar to
> sysfs notifications but with some extra info.
> 
> Currently it's actually always enabled whenever Android uses
> the IDLETIMER target, so we could also probably entirely
> remove it from the uapi in favour of just always enabling it,
> but again we can't upgrade old kernels already in the field.
> 
> (Also note that this doesn't change the structure's size,
> as it is simply fitting into the pre-existing padding, and
> that since 5.7 hasn't been released yet, there's still time
> to make this uapi visible change)
> 
> Cc: Manoj Basapathi <manojbm@codeaurora.org>
> Cc: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
>  include/uapi/linux/netfilter/xt_IDLETIMER.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/netfilter/xt_IDLETIMER.h b/include/uapi/linux/netfilter/xt_IDLETIMER.h
> index 434e6506abaa..49ddcdc61c09 100644
> --- a/include/uapi/linux/netfilter/xt_IDLETIMER.h
> +++ b/include/uapi/linux/netfilter/xt_IDLETIMER.h
> @@ -48,6 +48,7 @@ struct idletimer_tg_info_v1 {
>  
>  	char label[MAX_IDLETIMER_LABEL_SIZE];
>  
> +	__u8 send_nl_msg;   /* unused: for compatibility with Android */

Please, add client code for this send_nl_msg field.

Thank you.