On Thu, Feb 07, 2019 at 05:00:36PM +0100, Florian Westphal wrote:
> Phil Sutter <phil@xxxxxx> wrote:
> > > 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.
>
> Disregard that, I was low on caffeeine.
8)
> Attached patch makes things work much better for me (no need to take
> this, consider it a bit more verbose bug report).
Thanks!
> Not perfect, and one major issue remains.
>
> When someone munges the chain using native nft, ebtables-nft shows:
>
> Bridge chain: FOOBAR, entries: 1, policy: DROP
> -d 01:02:03:04:05:06 -j CONTINUE
>
> What it should show instead:
> Bridge chain: FOOBAR, entries: 1, policy: RETURN
> -j DROP
> -d 01:02:03:04:05:06 -j CONTINUE
>
> (because thats whats the actual state -- the last rule is unreachable).
Hmm. Yes, that's ugly. Also, if you perform a change to the ruleset in
that state (no matter what, e.g. just create another chain or add a rule
somewhere else), the policy rule will be moved to the end. Not sure how
we could handle this.
> Not a huge deal if we're only interested in "ebtables-nft only".
Maybe that's the only viable option? The only alternative I could think
of at this point would be to treat the whole chain as incompatible, but
that's probably not exactly a better way to handle it.
> diff --git a/iptables/nft-bridge.c b/iptables/nft-bridge.c
> --- 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, pol);
> + chain, entries, pol ? pol : "RETURN");
> }
>
That's what I've come up with, too.
> static void print_matches_and_watchers(const struct iptables_command_state *cs,
> diff --git a/iptables/nft.c b/iptables/nft.c
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
[...]
> @@ -1400,9 +1414,8 @@ static int nftnl_rule_list_cb(const struct nlmsghdr *nlh, void *data)
> return MNL_CB_OK;
> }
>
> - if (h->family == NFPROTO_BRIDGE &&
> - nft_rule_is_policy_rule(r))
> - nft_convert_policy_rule(h, c, r);
> + if (h->family == NFPROTO_BRIDGE)
> + nft_bridge_chain_rule_add_tail(h, r, c);
> else
> nftnl_chain_rule_add_tail(r, c);
>
Ah, that's nice!
> @@ -2758,7 +2771,9 @@ static int nft_action(struct nft_handle *h, int action)
> static int ebt_add_policy_rule(struct nftnl_chain *c, void *data)
> {
> uint32_t policy = nftnl_chain_get_u32(c, NFTNL_CHAIN_POLICY);
> - struct iptables_command_state cs = {};
> + struct iptables_command_state cs = {
> + .eb.bitmask = 0x2,
> + };
> struct nftnl_udata_buf *udata;
> struct nft_handle *h = data;
> struct nftnl_rule *r;
You're not really suggesting to hard-code EBT_NOPROTO value here, right?
Thanks, Phil
