| Message ID | 20191016230322.24432-3-phil@nwl.cc |
|---|---|
| State | Not Applicable |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | A bunch of fixes for --echo option | expand |
Phil Sutter <phil@nwl.cc> wrote: > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754. > > While it is true that a cache exists, we still need to capture new sets > and their elements if they are anonymous. This is because the name > changes and rules will refer to them by name. > > Given that there is no easy way to identify the anonymous set in cache > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name, > just go with cache updates. Assuming that echo option is typically used > for single commands, there is not much cache updating happening anyway. Acked-by: Florian Westphal <fw@strlen.de>
On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote: > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754. > > While it is true that a cache exists, we still need to capture new sets > and their elements if they are anonymous. This is because the name > changes and rules will refer to them by name. > > Given that there is no easy way to identify the anonymous set in cache > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name, > just go with cache updates. Assuming that echo option is typically used > for single commands, there is not much cache updating happening anyway. This was fixing a real bug, if this is breaking anything, then I think we are not getting to the root cause. But reverting it does not make things any better.
On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote: > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote: > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754. > > > > While it is true that a cache exists, we still need to capture new sets > > and their elements if they are anonymous. This is because the name > > changes and rules will refer to them by name. Please, tell me how I can reproduce this here with a simple snippet and I will have a look. Thanks! > > Given that there is no easy way to identify the anonymous set in cache > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name, > > just go with cache updates. Assuming that echo option is typically used > > for single commands, there is not much cache updating happening anyway. > > This was fixing a real bug, if this is breaking anything, then I think > we are not getting to the root cause. > > But reverting it does not make things any better.
Hi Pablo, On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote: > On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote: > > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote: > > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754. > > > > > > While it is true that a cache exists, we still need to capture new sets > > > and their elements if they are anonymous. This is because the name > > > changes and rules will refer to them by name. > > Please, tell me how I can reproduce this here with a simple snippet > and I will have a look. Thanks! Just run tests/monitor testsuite, echo testing simple.t will fail. Alternatively, add a rule with anonymous set like so: | # nft --echo add rule inet t c tcp dport '{ 22, 80 }' > > > Given that there is no easy way to identify the anonymous set in cache > > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name, > > > just go with cache updates. Assuming that echo option is typically used > > > for single commands, there is not much cache updating happening anyway. > > > > This was fixing a real bug, if this is breaking anything, then I think > > we are not getting to the root cause. > > > > But reverting it does not make things any better. With all respect, this wasn't obvious. There is no test case covering it, commit message reads like it is an optimization (apart from the subject containing 'fix'). Also, I tried to find a real solution (assuming that I'm merely supposed to avoid pointless cache insertion) but ended up with code updating cache for "no cache needed" case for anonymous sets and their elements. I didn't revert out of laziness but because I deemed anything else not feasible, after having tried two alternatives already. Cheers, Phil
On Thu, Oct 17, 2019 at 12:36:49PM +0200, Phil Sutter wrote: > Hi Pablo, > > On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote: > > On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote: > > > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote: > > > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754. > > > > > > > > While it is true that a cache exists, we still need to capture new sets > > > > and their elements if they are anonymous. This is because the name > > > > changes and rules will refer to them by name. > > > > Please, tell me how I can reproduce this here with a simple snippet > > and I will have a look. Thanks! > > Just run tests/monitor testsuite, echo testing simple.t will fail. > Alternatively, add a rule with anonymous set like so: > | # nft --echo add rule inet t c tcp dport '{ 22, 80 }' let me have a look. > > > > Given that there is no easy way to identify the anonymous set in cache > > > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name, > > > > just go with cache updates. Assuming that echo option is typically used > > > > for single commands, there is not much cache updating happening anyway. > > > > > > This was fixing a real bug, if this is breaking anything, then I think > > > we are not getting to the root cause. > > > > > > But reverting it does not make things any better. > > With all respect, this wasn't obvious. There is no test case covering > it, commit message reads like it is an optimization (apart from the > subject containing 'fix'). This patch is fixing --echo with nft using a batch via -f. I started updating the test infrastructure but I never finished this.
Hi, On Thu, Oct 17, 2019 at 01:09:57PM +0200, Pablo Neira Ayuso wrote: > On Thu, Oct 17, 2019 at 12:36:49PM +0200, Phil Sutter wrote: > > Hi Pablo, > > > > On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote: > > > On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote: > > > > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote: > > > > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754. > > > > > > > > > > While it is true that a cache exists, we still need to capture new sets > > > > > and their elements if they are anonymous. This is because the name > > > > > changes and rules will refer to them by name. > > > > > > Please, tell me how I can reproduce this here with a simple snippet > > > and I will have a look. Thanks! > > > > Just run tests/monitor testsuite, echo testing simple.t will fail. > > Alternatively, add a rule with anonymous set like so: > > | # nft --echo add rule inet t c tcp dport '{ 22, 80 }' > > let me have a look. Thanks! > > > > > Given that there is no easy way to identify the anonymous set in cache > > > > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name, > > > > > just go with cache updates. Assuming that echo option is typically used > > > > > for single commands, there is not much cache updating happening anyway. > > > > > > > > This was fixing a real bug, if this is breaking anything, then I think > > > > we are not getting to the root cause. > > > > > > > > But reverting it does not make things any better. > > > > With all respect, this wasn't obvious. There is no test case covering > > it, commit message reads like it is an optimization (apart from the > > subject containing 'fix'). > > This patch is fixing --echo with nft using a batch via -f. I started > updating the test infrastructure but I never finished this. Ah, I guess extending tests/monitor is not feasible for that. Maybe just add a case to tests/shell/testcases/nft-f? There is one shell test for echo option already (cache/0007_echo_cache_init_0). Cheers, Phil
On Thu, Oct 17, 2019 at 12:36:49PM +0200, Phil Sutter wrote: > Hi Pablo, > > On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote: > > On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote: > > > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote: > > > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754. > > > > > > > > While it is true that a cache exists, we still need to capture new sets > > > > and their elements if they are anonymous. This is because the name > > > > changes and rules will refer to them by name. > > > > Please, tell me how I can reproduce this here with a simple snippet > > and I will have a look. Thanks! > > Just run tests/monitor testsuite, echo testing simple.t will fail. > Alternatively, add a rule with anonymous set like so: > | # nft --echo add rule inet t c tcp dport '{ 22, 80 }' > > > > > Given that there is no easy way to identify the anonymous set in cache > > > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name, > > > > just go with cache updates. Assuming that echo option is typically used > > > > for single commands, there is not much cache updating happening anyway. > > > > > > This was fixing a real bug, if this is breaking anything, then I think > > > we are not getting to the root cause. > > > > > > But reverting it does not make things any better. > > With all respect, this wasn't obvious. There is no test case covering > it, commit message reads like it is an optimization (apart from the > subject containing 'fix'). After reverting: # ./run-tests.sh testcases/sets/0036add_set_element_expiration_0 I: using nft binary ./../../src/nft W: [FAILED] testcases/sets/0036add_set_element_expiration_0: got 139 I: results: [OK] 0 [FAILED] 1 [TOTAL] 1 so I made the test for this fix. commit 44348edfb9fa414152d53bcf705db882899ddc4e Author: Pablo Neira Ayuso <pablo@netfilter.org> Date: Mon Jul 1 18:34:42 2019 +0200 tests: shell: restore element expiration This patch adds a test for 24f33c710e8c ("src: enable set expiration date for set elements"). This is also implicitly testing for a cache corruption bug that is fixed by 9b032cd6477b ("monitor: fix double cache update with --echo").
On Thu, Oct 17, 2019 at 01:29:17PM +0200, Pablo Neira Ayuso wrote: > On Thu, Oct 17, 2019 at 12:36:49PM +0200, Phil Sutter wrote: > > Hi Pablo, > > > > On Thu, Oct 17, 2019 at 11:07:38AM +0200, Pablo Neira Ayuso wrote: > > > On Thu, Oct 17, 2019 at 10:55:49AM +0200, Pablo Neira Ayuso wrote: > > > > On Thu, Oct 17, 2019 at 01:03:20AM +0200, Phil Sutter wrote: > > > > > This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754. > > > > > > > > > > While it is true that a cache exists, we still need to capture new sets > > > > > and their elements if they are anonymous. This is because the name > > > > > changes and rules will refer to them by name. > > > > > > Please, tell me how I can reproduce this here with a simple snippet > > > and I will have a look. Thanks! > > > > Just run tests/monitor testsuite, echo testing simple.t will fail. > > Alternatively, add a rule with anonymous set like so: > > | # nft --echo add rule inet t c tcp dport '{ 22, 80 }' > > > > > > > Given that there is no easy way to identify the anonymous set in cache > > > > > (kernel doesn't (and shouldn't) dump SET_ID value) to update its name, > > > > > just go with cache updates. Assuming that echo option is typically used > > > > > for single commands, there is not much cache updating happening anyway. > > > > > > > > This was fixing a real bug, if this is breaking anything, then I think > > > > we are not getting to the root cause. > > > > > > > > But reverting it does not make things any better. > > > > With all respect, this wasn't obvious. There is no test case covering > > it, commit message reads like it is an optimization (apart from the > > subject containing 'fix'). > > After reverting: > > # ./run-tests.sh testcases/sets/0036add_set_element_expiration_0 > I: using nft binary ./../../src/nft > > W: [FAILED] testcases/sets/0036add_set_element_expiration_0: got 139 > > I: results: [OK] 0 [FAILED] 1 [TOTAL] 1 > > so I made the test for this fix. > > commit 44348edfb9fa414152d53bcf705db882899ddc4e > Author: Pablo Neira Ayuso <pablo@netfilter.org> > Date: Mon Jul 1 18:34:42 2019 +0200 > > tests: shell: restore element expiration > > This patch adds a test for 24f33c710e8c ("src: enable set expiration > date for set elements"). > > This is also implicitly testing for a cache corruption bug that is fixed > by 9b032cd6477b ("monitor: fix double cache update with --echo"). Ah, thanks for the pointer! I'll check what's going wrong there. Cheers, Phil
diff --git a/src/monitor.c b/src/monitor.c index 20810a5de0cfb..f353c5b09cf5d 100644 --- a/src/monitor.c +++ b/src/monitor.c @@ -900,6 +900,7 @@ int netlink_echo_callback(const struct nlmsghdr *nlh, void *data) .ctx = ctx, .loc = &netlink_location, .monitor_flags = 0xffffffff, + .cache_needed = true, }; if (!nft_output_echo(&echo_monh.ctx->nft->output))
This reverts commit 9b032cd6477b847f48dc8454f0e73935e9f48754. While it is true that a cache exists, we still need to capture new sets and their elements if they are anonymous. This is because the name changes and rules will refer to them by name. Given that there is no easy way to identify the anonymous set in cache (kernel doesn't (and shouldn't) dump SET_ID value) to update its name, just go with cache updates. Assuming that echo option is typically used for single commands, there is not much cache updating happening anyway. Signed-off-by: Phil Sutter <phil@nwl.cc> --- src/monitor.c | 1 + 1 file changed, 1 insertion(+)