Phil Sutter <[email protected]> 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.

Attached patch makes things work much better for me (no need to take
this, consider it a bit more verbose bug report).

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).

Not a huge deal if we're only interested in "ebtables-nft only".

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");
 }
 
 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
@@ -1324,6 +1324,11 @@ retry:
 
 #define EBT_POLICY_COMMENT "XTABLES_EB_INTERNAL_POLICY_RULE"
 
+struct nftnl_rule_list_cb_data {
+       struct nft_handle *handle;
+       struct nftnl_chain *chain;
+};
+
 static bool nft_rule_is_policy_rule(struct nftnl_rule *r)
 {
        const void *data;
@@ -1341,47 +1346,56 @@ static bool nft_rule_is_policy_rule(struct nftnl_rule 
*r)
        return true;
 }
 
-struct nftnl_rule_list_cb_data {
-       struct nft_handle *handle;
-       struct nftnl_chain *chain;
-};
-
-static int nft_convert_policy_rule(struct nft_handle *h,
-                                  struct nftnl_chain *c, struct nftnl_rule *r)
+static void nft_bridge_chain_rule_add_tail(struct nft_handle *h,
+                                          struct nftnl_rule *r, struct 
nftnl_chain *c)
 {
        struct nftnl_expr_iter *iter;
        int verdict = NFT_RETURN;
        struct nftnl_expr *expr;
 
+       if (!nft_rule_is_policy_rule(r))
+               goto out_add;
+
        iter = nftnl_expr_iter_create(r);
        if (iter == NULL)
-               return 0;
+               goto out_add;
 
        expr = nftnl_expr_iter_next(iter);
-       while (expr != NULL) {
-               if (strcmp("immediate",
-                          nftnl_expr_get_str(expr, NFTNL_EXPR_NAME))) {
-                       expr = nftnl_expr_iter_next(iter);
-                       continue;
-               }
-               verdict = nftnl_expr_get_u32(expr, NFTNL_EXPR_IMM_VERDICT);
-               break;
-       }
+       if (!expr)
+               goto out_iter;
+
+       if (strcmp("counter", nftnl_expr_get_str(expr, NFTNL_EXPR_NAME)))
+              goto out_iter;
+
+       expr = nftnl_expr_iter_next(iter);
+       if (strcmp("immediate", nftnl_expr_get_str(expr, NFTNL_EXPR_NAME)))
+               goto out_iter;
+
        nftnl_expr_iter_destroy(iter);
 
+       /* rule must only consist of verdict (else its not unconditional) */
+       if (!nftnl_expr_is_set(expr, NFTNL_EXPR_IMM_VERDICT))
+               goto out_iter;
+
+       verdict = nftnl_expr_get_u32(expr, NFTNL_EXPR_IMM_VERDICT);
+
        switch (verdict) {
        case NF_ACCEPT:
        case NF_DROP:
                nftnl_chain_set_u32(c, NFTNL_CHAIN_POLICY, verdict);
                break;
+       default:
+               goto out_add;
        }
 
-       if (batch_rule_add(h, NFT_COMPAT_RULE_DELETE, r) < 0) {
+       if (batch_rule_add(h, NFT_COMPAT_RULE_DELETE, r) < 0)
                printf("%s: failed to delete old policy rule\n", __func__);
-               return -1;
-       }
 
-       return 0;
+       return;
+out_iter:
+       nftnl_expr_iter_destroy(iter);
+out_add:
+       nftnl_chain_rule_add_tail(r, c);
 }
 
 static int nftnl_rule_list_cb(const struct nlmsghdr *nlh, void *data)
@@ -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);
 
@@ -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;

Reply via email to