Android boot failure with 6.12

Android boot failure with 6.12

[Maciej Żenczykowski]

We've had to:
  Revert "netfilter: xtables: avoid NFPROTO_UNSPEC where needed"
  https://android-review.googlesource.com/c/kernel/common/+/3305935/2

It seems the failure is (probably related to):
...
E IptablesRestoreController: -A bw_INPUT -j MARK --or-mark 0x100000
...
E IptablesRestoreController: -------  ERROR -------
E IptablesRestoreController: Warning: Extension MARK revision 0 not
supported, missing kernel module?
E IptablesRestoreController: ip6tables-restore v1.8.10 (legacy): MARK
target: kernel too old for --or-mark
E IptablesRestoreController: Error occurred at line: 27

But, I don't see an obvious bug in the CL we had to revert...

Re: Android boot failure with 6.12

[Maciej Żenczykowski]

Oh, wait

.family         = NFPROTO_IPV4,

in the v6 section

On Fri, Jan 10, 2025 at 2:20 PM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
>
> We've had to:
>   Revert "netfilter: xtables: avoid NFPROTO_UNSPEC where needed"
>   https://android-review.googlesource.com/c/kernel/common/+/3305935/2
>
> It seems the failure is (probably related to):
> ...
> E IptablesRestoreController: -A bw_INPUT -j MARK --or-mark 0x100000
> ...
> E IptablesRestoreController: -------  ERROR -------
> E IptablesRestoreController: Warning: Extension MARK revision 0 not
> supported, missing kernel module?
> E IptablesRestoreController: ip6tables-restore v1.8.10 (legacy): MARK
> target: kernel too old for --or-mark
> E IptablesRestoreController: Error occurred at line: 27
>
> But, I don't see an obvious bug in the CL we had to revert...

Re: Android boot failure with 6.12

[Maciej Żenczykowski]

Which likely means the fix would be:

https://android-review.googlesource.com/c/kernel/common/+/3445350/1..2

On Fri, Jan 10, 2025 at 2:23 PM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
>
> Oh, wait
>
> .family         = NFPROTO_IPV4,
>
> in the v6 section
>
> On Fri, Jan 10, 2025 at 2:20 PM Maciej Żenczykowski
> <zenczykowski@gmail.com> wrote:
> >
> > We've had to:
> >   Revert "netfilter: xtables: avoid NFPROTO_UNSPEC where needed"
> >   https://android-review.googlesource.com/c/kernel/common/+/3305935/2
> >
> > It seems the failure is (probably related to):
> > ...
> > E IptablesRestoreController: -A bw_INPUT -j MARK --or-mark 0x100000
> > ...
> > E IptablesRestoreController: -------  ERROR -------
> > E IptablesRestoreController: Warning: Extension MARK revision 0 not
> > supported, missing kernel module?
> > E IptablesRestoreController: ip6tables-restore v1.8.10 (legacy): MARK
> > target: kernel too old for --or-mark
> > E IptablesRestoreController: Error occurred at line: 27
> >
> > But, I don't see an obvious bug in the CL we had to revert...

Re: Android boot failure with 6.12

[Maciej Żenczykowski]

nvm - https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/netfilter/xt_mark.c?id=306ed1728e8438caed30332e1ab46b28c25fe3d8

Re: Android boot failure with 6.12

[Jozsef Kadlecsik]

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

Hi,

On Fri, 10 Jan 2025, Maciej Żenczykowski wrote:

> nvm - https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/netfilter/xt_mark.c?id=306ed1728e8438caed30332e1ab46b28c25fe3d8

Sorry, but I don't understand the patch at all. With it applied now it'd 
be not possible to load in the "MARK" target with IPv4. The code segment 
after the patch:

static struct xt_target mark_tg_reg[] __read_mostly = {
        {
                .name           = "MARK",
                .revision       = 2,
                .family         = NFPROTO_IPV6,
                .target         = mark_tg,
                .targetsize     = sizeof(struct xt_mark_tginfo2),
                .me             = THIS_MODULE,
        },
#if IS_ENABLED(CONFIG_IP_NF_ARPTABLES)
        {
                .name           = "MARK",
                .revision       = 2,
                .family         = NFPROTO_ARP,
                .target         = mark_tg,
                .targetsize     = sizeof(struct xt_mark_tginfo2),
                .me             = THIS_MODULE,
        },
#endif
#if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
        {
                .name           = "MARK",
                .revision       = 2,
                .family         = NFPROTO_IPV6,
                .target         = mark_tg,
                .targetsize     = sizeof(struct xt_mark_tginfo2),
                .me             = THIS_MODULE,
        },
#endif
};

How is it supposed to work for IPv4?

Why the "IS_ENABLED(CONFIG_IP6_NF_IPTABLES)" part was not enough for the 
IPv6-specific MARK target to be compiled in? Isn't it an issue about 
selecting CONFIG_IP6_NF_IPTABLES vs CONFIG_IP6_NF_IPTABLES_LEGACY?

Also, why the "mark" match was not split into NFPROTO_IPV4, NFPROTO_ARP, 
NFPROTO_IPV6 explicitly (and other matches where the target was split)?

Best regards,
Jozsef
-- 
E-mail : kadlec@netfilter.org, kadlec@blackhole.kfki.hu,
         kadlecsik.jozsef@wigner.hu
Address: Wigner Research Centre for Physics
         H-1525 Budapest 114, POB. 49, Hungary

Re: Android boot failure with 6.12

[Pablo Neira Ayuso]

On Fri, Jan 10, 2025 at 02:36:56PM -0800, Maciej Żenczykowski wrote:
> nvm - https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/netfilter/xt_mark.c?id=306ed1728e8438caed30332e1ab46b28c25fe3d8

Yes, 306ed1728e8438ca is the follow up fix you need:

commit 306ed1728e8438caed30332e1ab46b28c25fe3d8
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Sun Oct 20 14:49:51 2024 +0200

    netfilter: xtables: fix typo causing some targets not to load on IPv6
    
    - There is no NFPROTO_IPV6 family for mark and NFLOG.
    - TRACE is also missing module autoload with NFPROTO_IPV6.
    
    This results in ip6tables failing to restore a ruleset. This issue has been
    reported by several users providing incomplete patches.
    
    Very similar to Ilya Katsnelson's patch including a missing chunk in the
    TRACE extension.

Thanks.

Re: Android boot failure with 6.12

[Florian Westphal]

Jozsef Kadlecsik <kadlec@netfilter.org> wrote:
> > nvm - https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/netfilter/xt_mark.c?id=306ed1728e8438caed30332e1ab46b28c25fe3d8
> 
> Sorry, but I don't understand the patch at all. With it applied now it'd 
> be not possible to load in the "MARK" target with IPv4. The code segment 
> after the patch:
> 
> static struct xt_target mark_tg_reg[] __read_mostly = {
>         {
>                 .name           = "MARK",
>                 .revision       = 2,
>                 .family         = NFPROTO_IPV6,
>                 .target         = mark_tg,
>                 .targetsize     = sizeof(struct xt_mark_tginfo2),
>                 .me             = THIS_MODULE,
>         },
> #if IS_ENABLED(CONFIG_IP_NF_ARPTABLES)

Then you re-applied the patch, its already in 6.12.
NFPROTO_IPV6 is only set in the IP6_NF_IPTABLES section.

> Why the "IS_ENABLED(CONFIG_IP6_NF_IPTABLES)" part was not enough for the 
> IPv6-specific MARK target to be compiled in? Isn't it an issue about 
> selecting CONFIG_IP6_NF_IPTABLES vs CONFIG_IP6_NF_IPTABLES_LEGACY?

No, _LEGACY is about the set/gersockopt interface and the old
xt traversers, we could still use e.g. xt_mark.ko via NFT_COMPAT
interface.

> Also, why the "mark" match was not split into NFPROTO_IPV4, NFPROTO_ARP, 
> NFPROTO_IPV6 explicitly (and other matches where the target was split)?

mark match is fine, afaics.  Whats the concern?

The target got split because ebtables EBT_CONTINUE isn't equal to
XT_CONTINUE, so it won't do the right thing.

Re: Android boot failure with 6.12

[Pablo Neira Ayuso]

Hi Jozsef,

On Sat, Jan 11, 2025 at 02:31:18PM +0100, Jozsef Kadlecsik wrote:
> Hi,
> 
> On Fri, 10 Jan 2025, Maciej Żenczykowski wrote:
> 
> > nvm - https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/netfilter/xt_mark.c?id=306ed1728e8438caed30332e1ab46b28c25fe3d8
> 
> Sorry, but I don't understand the patch at all. With it applied now it'd 
> be not possible to load in the "MARK" target with IPv4. The code segment 
> after the patch:
> 
> static struct xt_target mark_tg_reg[] __read_mostly = {
>         {
>                 .name           = "MARK",
>                 .revision       = 2,
>                 .family         = NFPROTO_IPV6,
>                 .target         = mark_tg,
>                 .targetsize     = sizeof(struct xt_mark_tginfo2),
>                 .me             = THIS_MODULE,
>         },
> #if IS_ENABLED(CONFIG_IP_NF_ARPTABLES)
>         {
>                 .name           = "MARK",
>                 .revision       = 2,
>                 .family         = NFPROTO_ARP,
>                 .target         = mark_tg,
>                 .targetsize     = sizeof(struct xt_mark_tginfo2),
>                 .me             = THIS_MODULE,
>         },
> #endif
> #if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
>         {
>                 .name           = "MARK",
>                 .revision       = 2,
>                 .family         = NFPROTO_IPV6,
>                 .target         = mark_tg,
>                 .targetsize     = sizeof(struct xt_mark_tginfo2),
>                 .me             = THIS_MODULE,
>         },
> #endif
> };
> 
> How is it supposed to work for IPv4?
> 
> Why the "IS_ENABLED(CONFIG_IP6_NF_IPTABLES)" part was not enough for the 
> IPv6-specific MARK target to be compiled in? Isn't it an issue about 
> selecting CONFIG_IP6_NF_IPTABLES vs CONFIG_IP6_NF_IPTABLES_LEGACY?

This was fixed by an incremental patch:

  306ed1728e84 ("netfilter: xtables: fix typo causing some targets not to load on IPv6")

so there is no two MARK targets for NFPROTO_IPV6.

> Also, why the "mark" match was not split into NFPROTO_IPV4, NFPROTO_ARP,
> NFPROTO_IPV6 explicitly (and other matches where the target was split)?

The audit to tighten this interface searched for:

- use of xtables verdicts are incompatible with ebtables.
- IP header cannot be assumed to be linear on ebtables.

xt_mark match can be restricted too, ebtables uses ebt_mark. But this
should be safe, so this patch should probably go via nf-next.

Thanks.

Re: Android boot failure with 6.12

[Jozsef Kadlecsik]

Hi Florian,

On Sat, 11 Jan 2025, Florian Westphal wrote:

> > Also, why the "mark" match was not split into NFPROTO_IPV4, 
> > NFPROTO_ARP, NFPROTO_IPV6 explicitly (and other matches where the 
> > target was split)?
> 
> mark match is fine, afaics.  Whats the concern?
> 
> The target got split because ebtables EBT_CONTINUE isn't equal to
> XT_CONTINUE, so it won't do the right thing.

I have just reread the description of your patch "netfilter: xtables: 
avoid NFPROTO_UNSPEC where needed" and now I see why the match is fine as 
is. Thanks!

Best regards,
Jozsef
-- 
E-mail : kadlec@netfilter.org, kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
Address: Wigner Research Centre for Physics
         H-1525 Budapest 114, POB. 49, Hungary

Re: Android boot failure with 6.12

[Jozsef Kadlecsik]

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

Hi Pablo,

On Sat, 11 Jan 2025, Pablo Neira Ayuso wrote:

> On Sat, Jan 11, 2025 at 02:31:18PM +0100, Jozsef Kadlecsik wrote:
> > 
> > On Fri, 10 Jan 2025, Maciej Żenczykowski wrote:
> > 
> > > nvm - https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/net/netfilter/xt_mark.c?id=306ed1728e8438caed30332e1ab46b28c25fe3d8
> > 
> > Sorry, but I don't understand the patch at all. With it applied now it'd 
> > be not possible to load in the "MARK" target with IPv4. The code segment 
> > after the patch:
> > 
> > static struct xt_target mark_tg_reg[] __read_mostly = {
> >         {
> >                 .name           = "MARK",
> >                 .revision       = 2,
> >                 .family         = NFPROTO_IPV6,
> >                 .target         = mark_tg,
> >                 .targetsize     = sizeof(struct xt_mark_tginfo2),
> >                 .me             = THIS_MODULE,
> >         },
> > #if IS_ENABLED(CONFIG_IP_NF_ARPTABLES)
> >         {
> >                 .name           = "MARK",
> >                 .revision       = 2,
> >                 .family         = NFPROTO_ARP,
> >                 .target         = mark_tg,
> >                 .targetsize     = sizeof(struct xt_mark_tginfo2),
> >                 .me             = THIS_MODULE,
> >         },
> > #endif
> > #if IS_ENABLED(CONFIG_IP6_NF_IPTABLES)
> >         {
> >                 .name           = "MARK",
> >                 .revision       = 2,
> >                 .family         = NFPROTO_IPV6,
> >                 .target         = mark_tg,
> >                 .targetsize     = sizeof(struct xt_mark_tginfo2),
> >                 .me             = THIS_MODULE,
> >         },
> > #endif
> > };
> > 
> > How is it supposed to work for IPv4?
> > 
> > Why the "IS_ENABLED(CONFIG_IP6_NF_IPTABLES)" part was not enough for the 
> > IPv6-specific MARK target to be compiled in? Isn't it an issue about 
> > selecting CONFIG_IP6_NF_IPTABLES vs CONFIG_IP6_NF_IPTABLES_LEGACY?
> 
> This was fixed by an incremental patch:
> 
>   306ed1728e84 ("netfilter: xtables: fix typo causing some targets not to load on IPv6")
> 
> so there is no two MARK targets for NFPROTO_IPV6.

No, it was my fault not checking the patch properly. It's completely fine, 
sorry for the noise!

Best regards,
Jozsef
-- 
E-mail : kadlec@netfilter.org, kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu
Address: Wigner Research Centre for Physics
         H-1525 Budapest 114, POB. 49, Hungary