| Message ID | 20231102112122.383527-2-thaller@redhat.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft,1/2] json: implement json() hook for "symbol_expr_ops"/"variabl_expr_ops" | expand |
On Thu, Nov 02, 2023 at 12:20:29PM +0100, Thomas Haller wrote: > By now, all "struct expr_ops" have a json() hook set. Thus, drop > handling the possibility that they might not. From now on, it's a bug > to create a ops without the hook. > > It's not clear what the code tried to do. It bothered to implement a > fallback via fmemopen(), but apparently that fallback is no considered a > good solution as it also printed a "warning". Either the fallback is > good and does not warrant a warning. Or the condition is to be avoided > to begin with, which we should do by testing the expr_ops structures. > > As the fallback path has an overhead to create the memory stream, the > fallback path is indeed not great. That is the reason to outlaw a > missing json() hook, to require that all hooks are present, and to drop > the fallback path. > > A missing hook is very easy to cover with unit tests. Such a test shall > be added soon. That's fine to simplify code. But then, in 1/2 you better set some STUB that hits BUG() because we should not ever see variable and symbol expression from json listing path ever.
On Thu, 2023-11-02 at 12:27 +0100, Pablo Neira Ayuso wrote: > On Thu, Nov 02, 2023 at 12:20:29PM +0100, Thomas Haller wrote: > > By now, all "struct expr_ops" have a json() hook set. Thus, drop > > handling the possibility that they might not. From now on, it's a > > bug > > to create a ops without the hook. > > > > It's not clear what the code tried to do. It bothered to implement > > a > > fallback via fmemopen(), but apparently that fallback is no > > considered a > > good solution as it also printed a "warning". Either the fallback > > is > > good and does not warrant a warning. Or the condition is to be > > avoided > > to begin with, which we should do by testing the expr_ops > > structures. > > > > As the fallback path has an overhead to create the memory stream, > > the > > fallback path is indeed not great. That is the reason to outlaw a > > missing json() hook, to require that all hooks are present, and to > > drop > > the fallback path. > > > > A missing hook is very easy to cover with unit tests. Such a test > > shall > > be added soon. > > That's fine to simplify code. > > But then, in 1/2 you better set some STUB that hits BUG() because we > should not ever see variable and symbol expression from json listing > path ever. > I think BUG() would not work. This does happen, as the tests in patches Subject: [PATCH nft 0/7] add and check dump files for JSON in tests/shell Date: Tue, 31 Oct 2023 19:53:26 +0100 expose. When you apply the patchset (without patch 2/7) you can get those failures easily. Thomas
On Thu, Nov 02, 2023 at 03:09:42PM +0100, Thomas Haller wrote: > On Thu, 2023-11-02 at 12:27 +0100, Pablo Neira Ayuso wrote: > > On Thu, Nov 02, 2023 at 12:20:29PM +0100, Thomas Haller wrote: > > > By now, all "struct expr_ops" have a json() hook set. Thus, drop > > > handling the possibility that they might not. From now on, it's a > > > bug > > > to create a ops without the hook. > > > > > > It's not clear what the code tried to do. It bothered to implement > > > a > > > fallback via fmemopen(), but apparently that fallback is no > > > considered a > > > good solution as it also printed a "warning". Either the fallback > > > is > > > good and does not warrant a warning. Or the condition is to be > > > avoided > > > to begin with, which we should do by testing the expr_ops > > > structures. > > > > > > As the fallback path has an overhead to create the memory stream, > > > the > > > fallback path is indeed not great. That is the reason to outlaw a > > > missing json() hook, to require that all hooks are present, and to > > > drop > > > the fallback path. > > > > > > A missing hook is very easy to cover with unit tests. Such a test > > > shall > > > be added soon. > > > > That's fine to simplify code. > > > > But then, in 1/2 you better set some STUB that hits BUG() because we > > should not ever see variable and symbol expression from json listing > > path ever. > > > > I think BUG() would not work. This does happen, as the tests in patches > > Subject: [PATCH nft 0/7] add and check dump files for JSON in tests/shell > Date: Tue, 31 Oct 2023 19:53:26 +0100 > > expose. No listing from the kernel would use the variable expression. What example would be triggering bug?
On Thu, 2023-11-02 at 16:54 +0100, Pablo Neira Ayuso wrote: > On Thu, Nov 02, 2023 at 03:09:42PM +0100, Thomas Haller wrote: > > > > I think BUG() would not work. This does happen, as the tests in > > patches > > > > Subject: [PATCH nft 0/7] add and check dump files for JSON > > in tests/shell > > Date: Tue, 31 Oct 2023 19:53:26 +0100 > > > > expose. > > No listing from the kernel would use the variable expression. > > What example would be triggering bug? when you apply above patchset (and revert patch 2/7), and: $ make $ ./tests/shell/run-tests.sh ... $ grep '^[^ ]*W[^ ]*:' /tmp/nft-test.latest.*/test.log W: [CHK DUMP] 8/381 tests/shell/testcases/chains/0041chain_binding_0 W: [CHK DUMP] 66/381 tests/shell/testcases/cache/0010_implicit_chain_0 W: [CHK DUMP] 226/381 tests/shell/testcases/nft-f/sample-ruleset $ grep -R 'Command `./tests/shell/../../src/nft -j list ruleset""` failed' /tmp/nft-test.latest.*/ ... Gives: tests/shell/testcases/nft-f/sample-ruleset tests/shell/testcases/cache/0010_implicit_chain_0 tests/shell/testcases/chains/0041chain_binding_0 For example: /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains-0041chain_binding_0.4/rc-failed-chkdump:Command `./tests/shell/../../src/nft -j list ruleset""` failed /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains-0041chain_binding_0.4/rc-failed-chkdump:>>>> /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains-0041chain_binding_0.4/rc-failed-chkdump:warning: stmt ops chain have no json callback /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains-0041chain_binding_0.4/rc-failed-chkdump:warning: stmt ops chain have no json callback /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains-0041chain_binding_0.4/rc-failed-chkdump:<<<< There are also other failures. e.g. tests/shell/testcases/parsing/large_rule_pipe does not give stable output. I need to drop that .json-nft file in v2. Using Fedora kernel 6.5.6-300.fc39.x86_64. Thomas
On Thu, 2023-11-02 at 12:20 +0100, Thomas Haller wrote: > > > static json_t *stmt_print_json(const struct stmt *stmt, struct > output_ctx *octx) > { > - char buf[1024]; > - FILE *fp; > - > - if (stmt->ops->json) > - return stmt->ops->json(stmt, octx); > - > - fprintf(stderr, "warning: stmt ops %s have no json > callback\n", > - stmt->ops->name); > - > - fp = octx->output_fp; > - octx->output_fp = fmemopen(buf, 1024, "w"); > - > - stmt->ops->print(stmt, octx); > - > - fclose(octx->output_fp); > - octx->output_fp = fp; > - > - return json_pack("s", buf); > + return stmt->ops->json(stmt, octx); > } Regardless the ongoing discussion... This part of the patch is wrong. Must not be applied. stmt->ops is a "struct stmt_ops", which still has NULL "json" callbacks. The patch 1/1 only addresses "json" hooks in "struct expr_ops". Thomas
On Thu, 2023-11-02 at 18:39 +0100, Thomas Haller wrote: > > This part of the patch is wrong. Must not be applied. > > > stmt->ops is a "struct stmt_ops", which still has NULL "json" > callbacks. The patch 1/1 only addresses "json" hooks in "struct > expr_ops". The NULL pointers in json() hooks are: in struct expr_ops: - symbol_expr_ops - variabl_expr_ops in struct stmt_ops: - chain_stmt_ops The two patches lack handling chain_stmt_ops.json. Thomas
On Thu, Nov 02, 2023 at 05:17:56PM +0100, Thomas Haller wrote: > On Thu, 2023-11-02 at 16:54 +0100, Pablo Neira Ayuso wrote: > > What example would be triggering bug? > > when you apply above patchset (and revert patch 2/7), and: > > $ make > $ ./tests/shell/run-tests.sh > ... > $ grep '^[^ ]*W[^ ]*:' /tmp/nft-test.latest.*/test.log > W: [CHK DUMP] 8/381 tests/shell/testcases/chains/0041chain_binding_0 > W: [CHK DUMP] 66/381 tests/shell/testcases/cache/0010_implicit_chain_0 > W: [CHK DUMP] 226/381 tests/shell/testcases/nft-f/sample-ruleset > $ grep -R 'Command `./tests/shell/../../src/nft -j list ruleset""` failed' /tmp/nft-test.latest.*/ > ... > > Gives: > > tests/shell/testcases/nft-f/sample-ruleset > tests/shell/testcases/cache/0010_implicit_chain_0 > tests/shell/testcases/chains/0041chain_binding_0 > > For example: > > /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains-0041chain_binding_0.4/rc-failed-chkdump:Command `./tests/shell/../../src/nft -j list ruleset""` failed > /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains-0041chain_binding_0.4/rc-failed-chkdump:>>>> > /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains-0041chain_binding_0.4/rc-failed-chkdump:warning: stmt ops chain have no json callback > /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains-0041chain_binding_0.4/rc-failed-chkdump:warning: stmt ops chain have no json callback Yes, chain statement is lacking a json output, that is correct, that needs to be done. But, as for variable and symbol expressions, I do not see how those can be found in the 'list ruleset' path. Note that symbol expressions represent a preliminary state of the expression, these type of expressions go away after evaluation. Same thing applies to variable expression. They have no use for listing path. Do you have tests that explicitly refer to the lack of json callback for variable and symbol expressions just like in the warning above? > /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains-0041chain_binding_0.4/rc-failed-chkdump:<<<< > > There are also other failures. e.g. > tests/shell/testcases/parsing/large_rule_pipe does not give stable > output. I need to drop that .json-nft file in v2. What does 'unstable' mean in this case?
On Thu, 2023-11-02 at 21:51 +0100, Pablo Neira Ayuso wrote: > On Thu, Nov 02, 2023 at 05:17:56PM +0100, Thomas Haller wrote: > > > Yes, chain statement is lacking a json output, that is correct, that > needs to be done. What is the correct JSON syntax for printing a chain? For example, for test "tests/shell/testcases/nft-f/sample-ruleset" I get the following from `nft -j list ruleset`: [...] { "rule": { "family": "inet", "table": "filter", "chain": "home_input", "handle": 91, "expr": [ { "match": { "op": "==", "left": { "meta": { "key": "l4proto" } }, "right": { "set": [ "tcp", "udp" ] } } }, { "match": { "op": "==", "left": { "payload": { "protocol": "th", "field": "dport" } }, "right": 53 } }, "jump {\n\t\t\tip6 saddr != { fd00::/8, fe80::/64 } counter packets 0 bytes 0 reject with icmpv6 port-unreachable\n\t\t\taccept\n\t\t}" ] } }, [...] In `man libnftables-json`, searching for "jump" only gives: { "jump": { "target": * STRING *}} Is there an example how this JSON output should look like? (or a test, after all, I want to feed this output back into `nft -j --check -f -`). > But, as for variable and symbol expressions, I do not see how those > can be found in the 'list ruleset' path. Note that symbol expressions > represent a preliminary state of the expression, these type of > expressions go away after evaluation. Same thing applies to variable > expression. They have no use for listing path. ACK about symbol_expr_ops + variable_expr_ops. I will send a minor patch about that (essentially with code comments and remove the elaborate fallback code). > > Do you have tests that explicitly refer to the lack of json callback > for variable and symbol expressions just like in the warning above? > > > /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains- > > 0041chain_binding_0.4/rc-failed-chkdump:<<<< > > > > There are also other failures. e.g. > > tests/shell/testcases/parsing/large_rule_pipe does not give stable > > output. I need to drop that .json-nft file in v2. > > What does 'unstable' mean in this case? > It seems, that the order of the elements of the list is unstable. I didn't investigate. At this point, I only want to add the .json-nft files for tests that pass, and worry about the remaining issues after the basic test infrastructure about .json-nft tests is up. Thomas
Hi Thomas, On Fri, Nov 03, 2023 at 09:45:38AM +0100, Thomas Haller wrote: > On Thu, 2023-11-02 at 21:51 +0100, Pablo Neira Ayuso wrote: > > On Thu, Nov 02, 2023 at 05:17:56PM +0100, Thomas Haller wrote: > > > > > > Yes, chain statement is lacking a json output, that is correct, that > > needs to be done. > > What is the correct JSON syntax for printing a chain? There is currently no syntax, so this needs to be defined. > For example, for test "tests/shell/testcases/nft-f/sample-ruleset" I > get the following from `nft -j list ruleset`: > > [...] > { > "rule": { > "family": "inet", > "table": "filter", > "chain": "home_input", > "handle": 91, > "expr": [ > { > "match": { > "op": "==", > "left": { > "meta": { > "key": "l4proto" > } > }, > "right": { > "set": [ > "tcp", > "udp" > ] > } > } > }, > { > "match": { > "op": "==", > "left": { > "payload": { > "protocol": "th", > "field": "dport" > } > }, > "right": 53 > } > }, > "jump {\n\t\t\tip6 saddr != { fd00::/8, fe80::/64 } counter packets 0 bytes 0 reject with icmpv6 port-unreachable\n\t\t\taccept\n\t\t}" > ] > } > }, > [...] > > > In `man libnftables-json`, searching for "jump" only gives: > > { "jump": { "target": * STRING *}} > > > Is there an example how this JSON output should look like? > > (or a test, after all, I want to feed this output back into `nft -j --check -f -`). Maybe something like: { "jump": { "chain" : [ rules here ] } but I would need to sketch some code to explore how complicate this is to reuse existing JSON code. > > But, as for variable and symbol expressions, I do not see how those > > can be found in the 'list ruleset' path. Note that symbol expressions > > represent a preliminary state of the expression, these type of > > expressions go away after evaluation. Same thing applies to variable > > expression. They have no use for listing path. > > ACK about symbol_expr_ops + variable_expr_ops. I will send a minor > patch about that (essentially with code comments and remove the > elaborate fallback code). OK, so it is chain statement that is missing the json callback. > > Do you have tests that explicitly refer to the lack of json callback > > for variable and symbol expressions just like in the warning above? > > > > > /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains- > > > 0041chain_binding_0.4/rc-failed-chkdump:<<<< > > > > > > There are also other failures. e.g. > > > tests/shell/testcases/parsing/large_rule_pipe does not give stable > > > output. I need to drop that .json-nft file in v2. > > > > What does 'unstable' mean in this case? > > It seems, that the order of the elements of the list is unstable. Ah, I see, so it is not easy to compare. Thanks for explaining. > I didn't investigate. At this point, I only want to add the > .json-nft files for tests that pass, and worry about the remaining > issues after the basic test infrastructure about .json-nft tests is > up.
Thomas Haller <thaller@redhat.com> wrote: > What is the correct JSON syntax for printing a chain? Thats the problem, the chain has no user-visible name :-) Its a shortcut syntax alias. Essentially, this: meta l4proto { tcp, udp } th dport domain jump { ip6 saddr != $private_ip6 counter reject accept } is the same as chain $RANDOM_NAME { ip6 saddr != $private_ip6 counter reject accept } meta l4proto { tcp, udp } th dport domain jump $RANDOM_NAME Except that, if you remove the rule, then $RANDOM_NAME chain is deleted as well and that $RANDOM_NAME is readonly after creation (you cannot add or remove rules from it). > For example, for test "tests/shell/testcases/nft-f/sample-ruleset" I > get the following from `nft -j list ruleset`: > > [...] > { > "rule": { > "family": "inet", > "table": "filter", > "chain": "home_input", > "handle": 91, > "expr": [ > { > "match": { > "op": "==", > "left": { > "meta": { > "key": "l4proto" > } > }, > "right": { > "set": [ > "tcp", > "udp" > ] > } > } > }, > { > "match": { > "op": "==", > "left": { > "payload": { > "protocol": "th", > "field": "dport" > } > }, > "right": 53 > } > }, > "jump {\n\t\t\tip6 saddr != { fd00::/8, fe80::/64 } counter packets 0 bytes 0 reject with icmpv6 port-unreachable\n\t\t\taccept\n\t\t}" > ] > } > }, > [...] > > > In `man libnftables-json`, searching for "jump" only gives: > > { "jump": { "target": * STRING *}} > > Is there an example how this JSON output should look like? We need to define a new syntax for this case. I think it would be best to recurse, i.e. something like: # i.e., make it clear that this is a jump ("left" :) and that the target is a complete, anonymous chain with 0 or more rules embedded within. }, "jump" : {"chain" : { "rule" : { "family" : "inet", "table": "t", "chain": "c", "handle": 5, "expr": [{"match": {"op": "!=", "left": { "payload": { "protocol": "ip6", "field": "saddr"}}, "right": { "set": [{"prefix": {"addr": "fd00::", "len": 8}}, { ....
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > Do you have tests that explicitly refer to the lack of json callback > > > for variable and symbol expressions just like in the warning above? > > > > > > > /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains- > > > > 0041chain_binding_0.4/rc-failed-chkdump:<<<< > > > > > > > > There are also other failures. e.g. > > > > tests/shell/testcases/parsing/large_rule_pipe does not give stable > > > > output. I need to drop that .json-nft file in v2. > > > > > > What does 'unstable' mean in this case? > > > > It seems, that the order of the elements of the list is unstable. > > Ah, I see, so it is not easy to compare. Thanks for explaining. If its really just that the element ordering is random, then I think we should change libnftables to sort before printing, it should not be too much work/code.
On Fri, Nov 03, 2023 at 01:19:33PM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > Do you have tests that explicitly refer to the lack of json callback > > > > for variable and symbol expressions just like in the warning above? > > > > > > > > > /tmp/nft-test.latest.thom/test-tests-shell-testcases-chains- > > > > > 0041chain_binding_0.4/rc-failed-chkdump:<<<< > > > > > > > > > > There are also other failures. e.g. > > > > > tests/shell/testcases/parsing/large_rule_pipe does not give stable > > > > > output. I need to drop that .json-nft file in v2. > > > > > > > > What does 'unstable' mean in this case? > > > > > > It seems, that the order of the elements of the list is unstable. > > > > Ah, I see, so it is not easy to compare. Thanks for explaining. > > If its really just that the element ordering is random, then I think > we should change libnftables to sort before printing, it should not > be too much work/code. There is a sort function that is already used when listing elements IIRC. Maybe this is missing the json codepath?
diff --git a/src/json.c b/src/json.c index 0fd78b763af7..cbd2abbc8b52 100644 --- a/src/json.c +++ b/src/json.c @@ -44,26 +44,7 @@ static json_t *expr_print_json(const struct expr *expr, struct output_ctx *octx) { - const struct expr_ops *ops; - char buf[1024]; - FILE *fp; - - ops = expr_ops(expr); - if (ops->json) - return ops->json(expr, octx); - - fprintf(stderr, "warning: expr ops %s have no json callback\n", - expr_name(expr)); - - fp = octx->output_fp; - octx->output_fp = fmemopen(buf, 1024, "w"); - - ops->print(expr, octx); - - fclose(octx->output_fp); - octx->output_fp = fp; - - return json_pack("s", buf); + return expr_ops(expr)->json(expr, octx); } static json_t *set_dtype_json(const struct expr *key) @@ -89,24 +70,7 @@ static json_t *set_dtype_json(const struct expr *key) static json_t *stmt_print_json(const struct stmt *stmt, struct output_ctx *octx) { - char buf[1024]; - FILE *fp; - - if (stmt->ops->json) - return stmt->ops->json(stmt, octx); - - fprintf(stderr, "warning: stmt ops %s have no json callback\n", - stmt->ops->name); - - fp = octx->output_fp; - octx->output_fp = fmemopen(buf, 1024, "w"); - - stmt->ops->print(stmt, octx); - - fclose(octx->output_fp); - octx->output_fp = fp; - - return json_pack("s", buf); + return stmt->ops->json(stmt, octx); } static json_t *set_stmt_list_json(const struct list_head *stmt_list,
By now, all "struct expr_ops" have a json() hook set. Thus, drop handling the possibility that they might not. From now on, it's a bug to create a ops without the hook. It's not clear what the code tried to do. It bothered to implement a fallback via fmemopen(), but apparently that fallback is no considered a good solution as it also printed a "warning". Either the fallback is good and does not warrant a warning. Or the condition is to be avoided to begin with, which we should do by testing the expr_ops structures. As the fallback path has an overhead to create the memory stream, the fallback path is indeed not great. That is the reason to outlaw a missing json() hook, to require that all hooks are present, and to drop the fallback path. A missing hook is very easy to cover with unit tests. Such a test shall be added soon. Signed-off-by: Thomas Haller <thaller@redhat.com> --- src/json.c | 40 ++-------------------------------------- 1 file changed, 2 insertions(+), 38 deletions(-)