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

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

[Maciej Żenczykowski]

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 */
 	__u8 timer_type;
 
 	/* for kernel module internal use only */
-- 
2.26.0.rc2.310.g2932bb562d-goog

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

[Pablo Neira Ayuso]

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.

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

[Maciej Żenczykowski]

By client code do you mean code for the iptables userspace binary?

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

[Pablo Neira Ayuso]

On Tue, Mar 31, 2020 at 09:41:39AM -0700, Maciej Żenczykowski wrote:
> By client code do you mean code for the iptables userspace binary?

Code that uses this, please.

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

[Jan Engelhardt]

On Tuesday 2020-03-31 18:35, Maciej Żenczykowski wrote:
>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 */
> 	__u8 timer_type;
> 
> 	/* for kernel module internal use only */
>-- 

This breaks the ABI for law-abiding Linux users (i.e. the GNU/Linux 
subgroup of it), which is equally terrible.

You will have to introduce a IDLETIMER v2.

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

[Pablo Neira Ayuso]

On Tue, Mar 31, 2020 at 08:14:17PM +0200, Jan Engelhardt wrote:
> 
> On Tuesday 2020-03-31 18:35, Maciej Żenczykowski wrote:
> >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 */
> > 	__u8 timer_type;
> > 
> > 	/* for kernel module internal use only */
> >-- 
> 
> This breaks the ABI for law-abiding Linux users (i.e. the GNU/Linux 
> subgroup of it), which is equally terrible.
> 
> You will have to introduce a IDLETIMER v2.

IIRC, IDLETIMER v1 is in net-next, scheduled for 5.7-rc, there is no
release for this code yet.

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

[Maciej Żenczykowski]

Right, this is not in 5.6 as it's only in net-next atm as it was only
merged very recently.
I mentioned this in the commit message.

I'm not sure what you mean by code that uses this.
You can checkout aosp source and look there...
There's the kernel code (that's effectively already linked from the
commit message), and the iptables userspace changes (
https://android.googlesource.com/platform/external/iptables/+/refs/heads/master/extensions/libxt_IDLETIMER.c#39
), and the netd C++/Java layer that uses iptables -j IDLETIMER
--send_nl_msg 1 (
https://android.googlesource.com/platform/system/netd/+/refs/heads/master/server/IdletimerController.cpp#151
) and the resulting notifications parsing (can't easily find it atm).

If you mean by code that uses this patch... that's impossible as this
patch doesn't implement a usable feature.
It just moves the offset.

Could you clarify what you're asking for?

On Tue, Mar 31, 2020 at 11:16 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Tue, Mar 31, 2020 at 08:14:17PM +0200, Jan Engelhardt wrote:
> >
> > On Tuesday 2020-03-31 18:35, Maciej Żenczykowski wrote:
> > >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 */
> > >     __u8 timer_type;
> > >
> > >     /* for kernel module internal use only */
> > >--
> >
> > This breaks the ABI for law-abiding Linux users (i.e. the GNU/Linux
> > subgroup of it), which is equally terrible.
> >
> > You will have to introduce a IDLETIMER v2.
>
> IIRC, IDLETIMER v1 is in net-next, scheduled for 5.7-rc, there is no
> release for this code yet.

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

[Pablo Neira Ayuso]

On Tue, Mar 31, 2020 at 02:21:00PM -0700, Maciej Żenczykowski wrote:
> Right, this is not in 5.6 as it's only in net-next atm as it was only
> merged very recently.
> I mentioned this in the commit message.
> 
> I'm not sure what you mean by code that uses this.
> You can checkout aosp source and look there...
> There's the kernel code (that's effectively already linked from the
> commit message), and the iptables userspace changes (
> https://android.googlesource.com/platform/external/iptables/+/refs/heads/master/extensions/libxt_IDLETIMER.c#39

OK, so this is field ised set in userspace.

> ), and the netd C++/Java layer that uses iptables -j IDLETIMER
> --send_nl_msg 1 (
> https://android.googlesource.com/platform/system/netd/+/refs/heads/master/server/IdletimerController.cpp#151
> ) and the resulting notifications parsing (can't easily find it atm).
> 
> If you mean by code that uses this patch... that's impossible as this
> patch doesn't implement a usable feature.
> It just moves the offset.
> 
> Could you clarify what you're asking for?

Maybe I'm misunderstanding. How is this field used in aosp?

I mean, if --send_nl_msg 1 is passed, how does the existing behaviour
changes?

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

[Maciej Żenczykowski]

Right, so if you look at the android common kernel implementation.
  https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/net/netfilter/xt_IDLETIMER.c#201

(and in particular grep for send_nl_msg throughout the file) the core
difference is the call to notify_netlink_uevent() at
  https://android.googlesource.com/kernel/common/+/refs/heads/android-mainline/net/netfilter/xt_IDLETIMER.c#101

this function is also defined in the same file, and it ends up calling
  kobject_uevent_env(idletimer_tg_kobj, KOBJ_CHANGE, envp);
with INTERFACE, STATE, UID and TIME_NS metadata.

this is later parsed in netd C++:
  https://android.googlesource.com/platform/system/netd/+/refs/heads/master/server/NetlinkHandler.cpp#208

which then via notifyInterfaceClassActivityChanged() I believe
generates a binder call into the java network stack
for further processing.

void NetlinkHandler::notifyInterfaceClassActivityChanged(int label,
bool isActive,
int64_t timestamp, int uid) {
LOG_EVENT_FUNC(BINDER_RETRY, onInterfaceClassActivityChanged,
isActive, label, timestamp, uid);
}

Ultimately the goal is to know WHO (uid - on Android it's 1 unix uid
per {user,app} combination) generated traffic
activity, WHEN (timestamp) and WHERE (label ie. interface).

ie. To figure out whether an interface is idle or not and can be
shutdown, or powersaved, etc.
WHO matters because some users (apps!) might be insufficiently
important (lack of background data privs) to prevent
a network device powersavings/sleep/disconnect or to cause a wakeup (I think).

My understanding is the 'timer' thing (aka HARDIDLETIMER) was added
because phones are almost constantly
entering suspend state, so we care about real clock time
(CLOCK_BOOTTIME) rather than cpu activity time (CLOCK_MONOTONIC which
doesn't include time the device is suspended).

I think this could probably be eventually switched to use the sysfs
notification mechanism,
but we can't change that on any of the already shipped devices and
it's kind of late even for devices shipping this year,
and that system would presumably need to be extended to support uids.

On Tue, Mar 31, 2020 at 4:28 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Tue, Mar 31, 2020 at 02:21:00PM -0700, Maciej Żenczykowski wrote:
> > Right, this is not in 5.6 as it's only in net-next atm as it was only
> > merged very recently.
> > I mentioned this in the commit message.
> >
> > I'm not sure what you mean by code that uses this.
> > You can checkout aosp source and look there...
> > There's the kernel code (that's effectively already linked from the
> > commit message), and the iptables userspace changes (
> > https://android.googlesource.com/platform/external/iptables/+/refs/heads/master/extensions/libxt_IDLETIMER.c#39
>
> OK, so this is field ised set in userspace.
>
> > ), and the netd C++/Java layer that uses iptables -j IDLETIMER
> > --send_nl_msg 1 (
> > https://android.googlesource.com/platform/system/netd/+/refs/heads/master/server/IdletimerController.cpp#151
> > ) and the resulting notifications parsing (can't easily find it atm).
> >
> > If you mean by code that uses this patch... that's impossible as this
> > patch doesn't implement a usable feature.
> > It just moves the offset.
> >
> > Could you clarify what you're asking for?
>
> Maybe I'm misunderstanding. How is this field used in aosp?
>
> I mean, if --send_nl_msg 1 is passed, how does the existing behaviour
> changes?

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

[Jan Engelhardt]

On Tuesday 2020-03-31 23:21, Maciej Żenczykowski wrote:

>Right, this is not in 5.6 as it's only in net-next atm as it was only
>merged very recently.

Sorry. I read between the lines and had picked up "v0 is not present" 
and erroneously applied that statement to the standard kernel.

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

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

Hi Maciej,

On Tue, Mar 31, 2020 at 09:35:59AM -0700, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
[...]
> 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.

I'm attaching a new version, including EOPNOTSUPP if the send_nl_msg
field is set, it's the most basic handling I can think of until this
option becomes useful.

Would you commit to send a patch for this new merge window to make it
useful?

Thank you.

[-- Attachment #2: 0001-netfilter-IDLETIMER-target-v1-match-Android-layout.patch --]
[-- Type: text/x-diff, Size: 4111 bytes --]

From f29c2a23ac49126886a2248e5948a8c62888e3dc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Maciej=20=C5=BBenczykowski?= <maze@google.com>
Date: Tue, 31 Mar 2020 09:35:59 -0700
Subject: [PATCH] netfilter: IDLETIMER target v1 - match Android layout
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/uapi/linux/netfilter/xt_IDLETIMER.h | 1 +
 net/netfilter/xt_IDLETIMER.c                | 3 +++
 2 files changed, 4 insertions(+)

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 */
 	__u8 timer_type;
 
 	/* for kernel module internal use only */
diff --git a/net/netfilter/xt_IDLETIMER.c b/net/netfilter/xt_IDLETIMER.c
index 75bd0e5dd312..7b2f359bfce4 100644
--- a/net/netfilter/xt_IDLETIMER.c
+++ b/net/netfilter/xt_IDLETIMER.c
@@ -346,6 +346,9 @@ static int idletimer_tg_checkentry_v1(const struct xt_tgchk_param *par)
 
 	pr_debug("checkentry targinfo%s\n", info->label);
 
+	if (info->send_nl_msg)
+		return -EOPNOTSUPP;
+
 	ret = idletimer_tg_helper((struct idletimer_tg_info *)info);
 	if(ret < 0)
 	{
-- 
2.11.0

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

[Maciej Żenczykowski]

> Hi Maciej,
>
> I'm attaching a new version, including EOPNOTSUPP if the send_nl_msg
> field is set, it's the most basic handling I can think of until this
> option becomes useful.
>
> Would you commit to send a patch for this new merge window to make it
> useful?

Yes, I can do that.

Thank you.

Reviewed-by: Maciej Żenczykowski <maze@google.com>

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

[Pablo Neira Ayuso]

On Sun, Apr 05, 2020 at 01:14:39PM -0700, Maciej Żenczykowski wrote:
> > Hi Maciej,
> >
> > I'm attaching a new version, including EOPNOTSUPP if the send_nl_msg
> > field is set, it's the most basic handling I can think of until this
> > option becomes useful.
> >
> > Would you commit to send a patch for this new merge window to make it
> > useful?
> 
> Yes, I can do that.

Thank you.

Patch is applied to nf.git