| Message ID | 20200331163559.132240-1-zenczykowski@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | netfilter: IDLETIMER target v1 - match Android layout | expand |
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.
By client code do you mean code for the iptables userspace binary?
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.
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.
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.
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.
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?
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?
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.
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.
> 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>
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
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 */