On Thu, Feb 07, 2019 at 04:29:52PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@xxxxxx> wrote:
> > Legacy ebtables supports policies for user-defined chains - and what's
> > worse, they default to ACCEPT unlike anywhere else. So lack of support
> > for this braindead feature in ebtables-nft is actually a change of
> > behaviour which very likely affects all ebtables users out there.
> >
> > The solution implemented here uses an implicit (and transparent) last
> > rule in all user-defined ebtables-nft chains with policy other than
> > RETURN. This rule is identified by an nft comment
> > "XTABLES_EB_INTERNAL_POLICY_RULE" (since commit ccf154d7420c0 ("xtables:
> > Don't use native nftables comments") nft comments are not used
> > otherwise).
> >
> > To minimize interference with existing code, this policy rule is removed
> > from chains during cache population and the policy is saved in
> > NFTNL_CHAIN_POLICY attribute. When committing changes to the kernel,
> > nft_commit() traverses through the list of chains and (re-)creates
> > policy rules if required.
> >
> > In ebtables-nft-restore, table flushes are problematic. To avoid weird
> > kernel error responses, introduce a custom 'table_flush' callback which
> > removes any pending policy rule add/remove jobs prior to creating the
> > NFT_COMPAT_TABLE_FLUSH one.
> >
> > I've hidden all this mess behind checks for h->family, so hopefully
> > impact on {ip,ip6,arp}tables-nft should be negligible.
> >
> > Signed-off-by: Phil Sutter <phil@xxxxxx>
> > ---
> > iptables/nft-bridge.c | 2 +-
> > iptables/nft.c | 215 +++++++++++++++++-
> > iptables/nft.h | 4 +
> > .../ebtables/0002-ebtables-save-restore_0 | 7 +
> > iptables/xtables-eb.c | 20 +-
> > iptables/xtables-restore.c | 23 +-
> > 6 files changed, 249 insertions(+), 22 deletions(-)
> >
> > diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
> > index 7c390dfa2a898..72c89987a07f2 100644
> > --- a/iptables/nft-bridge.c
> > +++ b/iptables/nft-bridge.c
> > @@ -358,7 +358,7 @@ static void nft_bridge_print_header(unsigned int format, const char *chain,
> > bool basechain, uint32_t refs, uint32_t entries)
> > {
> > printf("Bridge chain: %s, entries: %u, policy: %s\n",
> > - chain, entries, basechain ? pol : "RETURN");
> > + chain, entries, pol);
>
> This causes
>
> Bridge chain: FOOBAR, entries: 0, policy: (null)
>
> after "ebtables -P FOOBAR RETURN".
> Reverting this hunk shows "RETURN" as expected.
Oh. I missed that case. Of course that's not a real solution, otherwise
user-defined chain policies will always show up as RETURN. I'll find a
real fix.
> Doing "ebtables -P FOOBAR DROP" sets a hidden rule, but its still shown
> as ACCEPT. I'll debiug this further.
Not happening here, not sure why.
> plain nft shows:
> ether type 0x0000 counter packets 0 bytes 0 drop comment "XTABLES_EB_INTERNAL_POLICY_RULE"
>
> This "ether type" is bonkers as well, it should not be there.
> Looks like ebt_add_policy_rule needs to set "cs.eb.bitmask = EBT_NOPROTO".
Yes, indeed. And since EBT_NOPROTO is defined in nft-bridge.h, I have to
include that in nft.c. Initially I wanted to avoid that. But if it's
needed anyway, I could move all introduced functions into nft-bridge.c.
The only problem with that is nft_rule_new() being static, but I could
either change that or duplicate its content since it's quite small.
> > + while (expr != NULL) {
> > + if (strcmp("immediate",
> > + nftnl_expr_get_str(expr, NFTNL_EXPR_NAME))) {
> > + expr = nftnl_expr_iter_next(iter);
> > + continue;
> > + }
>
> The imm should come first, in case there is a different expr this isn't
> an "implicit policy" (because its not unconditional verdict).
Good point! No need to traverse the whole expr list.
> Should perhaps also check nftnl_expr_is_set(expr, NFTNL_EXPR_IMM_VERDICT)
> so we really don't fall for random immediates.
Will do, thanks!
Cheers, Phil
