| Message ID | 20231201040020.161143-1-denkenz@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] handshake: Add cleanup function for handshake_state | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-alpine-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-fetch | success | Fetch PR |
| prestwoj/iwd-ci-gitlint | success | GitLint |
| prestwoj/iwd-alpine-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-alpine-ci-build | success | Build - Configure |
| prestwoj/iwd-ci-makedistcheck | success | Make Distcheck |
| prestwoj/iwd-ci-build | success | Build - Configure |
| prestwoj/iwd-alpine-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-alpine-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-makecheckvalgrind | success | Make Check w/Valgrind |
| prestwoj/iwd-ci-makecheck | success | Make Check |
| prestwoj/iwd-ci-clang | success | clang PASS |
| prestwoj/iwd-ci-incremental_build | success | Incremental Build with patches |
| prestwoj/iwd-alpine-ci-incremental_build | success | Incremental Build with patches |
| prestwoj/iwd-ci-testrunner | fail | test-runner - FAIL: testPSK-roam |
Hi Denis, On 11/30/23 20:00, Denis Kenzior wrote: > To allow _auto_(handshake_state_free) variables to be used. > --- > src/handshake.c | 7 ++++++- > src/handshake.h | 3 +++ > 2 files changed, 9 insertions(+), 1 deletion(-) All LGTM Thanks, James
Hi James, On 12/1/23 06:42, James Prestwood wrote: > Hi Denis, > > On 11/30/23 20:00, Denis Kenzior wrote: >> To allow _auto_(handshake_state_free) variables to be used. >> --- >> src/handshake.c | 7 ++++++- >> src/handshake.h | 3 +++ >> 2 files changed, 9 insertions(+), 1 deletion(-) > All LGTM > Any ideas why testPSK-roam tests are failing? They seem to be quite flaky on my machine as well, passing only 4/9? Doesn't seem related to this series though? | testPSK-roam | 4 | 5 | 0 | 256.84 | Regards, -Denis
Hi Denis, On 12/1/23 07:08, Denis Kenzior wrote: > Hi James, > > On 12/1/23 06:42, James Prestwood wrote: >> Hi Denis, >> >> On 11/30/23 20:00, Denis Kenzior wrote: >>> To allow _auto_(handshake_state_free) variables to be used. >>> --- >>> src/handshake.c | 7 ++++++- >>> src/handshake.h | 3 +++ >>> 2 files changed, 9 insertions(+), 1 deletion(-) >> All LGTM >> > > Any ideas why testPSK-roam tests are failing? They seem to be quite > flaky on my machine as well, passing only 4/9? Doesn't seem related > to this series though? > > | testPSK-roam | 4 | 5 | 0 | 256.84 | Not seeing that on my end (with and without your patches). The packet-loss roam tests is always flaky on the CI, but I've never seen that many failures. What UML kernel are you running so I can take that out of the equation? > > > Regards, > -Denis >
On 12/1/23 07:22, James Prestwood wrote: > Hi Denis, > > On 12/1/23 07:08, Denis Kenzior wrote: >> Hi James, >> >> On 12/1/23 06:42, James Prestwood wrote: >>> Hi Denis, >>> >>> On 11/30/23 20:00, Denis Kenzior wrote: >>>> To allow _auto_(handshake_state_free) variables to be used. >>>> --- >>>> src/handshake.c | 7 ++++++- >>>> src/handshake.h | 3 +++ >>>> 2 files changed, 9 insertions(+), 1 deletion(-) >>> All LGTM >>> >> >> Any ideas why testPSK-roam tests are failing? They seem to be quite >> flaky on my machine as well, passing only 4/9? Doesn't seem related >> to this series though? >> >> | testPSK-roam | 4 | 5 | 0 | 256.84 | > > Not seeing that on my end (with and without your patches). The > packet-loss roam tests is always flaky on the CI, but I've never seen > that many failures. > > What UML kernel are you running so I can take that out of the equation? Nvm, I'm getting the 4 failures on a 6.6 kernel now. Something must have changed so I'll look into it. > >> >> >> Regards, >> -Denis >>
Hi Denis, On 12/1/23 07:32, James Prestwood wrote: > > On 12/1/23 07:22, James Prestwood wrote: >> Hi Denis, >> >> On 12/1/23 07:08, Denis Kenzior wrote: >>> Hi James, >>> >>> On 12/1/23 06:42, James Prestwood wrote: >>>> Hi Denis, >>>> >>>> On 11/30/23 20:00, Denis Kenzior wrote: >>>>> To allow _auto_(handshake_state_free) variables to be used. >>>>> --- >>>>> src/handshake.c | 7 ++++++- >>>>> src/handshake.h | 3 +++ >>>>> 2 files changed, 9 insertions(+), 1 deletion(-) >>>> All LGTM >>>> >>> >>> Any ideas why testPSK-roam tests are failing? They seem to be quite >>> flaky on my machine as well, passing only 4/9? Doesn't seem related >>> to this series though? >>> >>> | testPSK-roam | 4 | 5 | 0 | 256.84 | >> >> Not seeing that on my end (with and without your patches). The >> packet-loss roam tests is always flaky on the CI, but I've never seen >> that many failures. >> >> What UML kernel are you running so I can take that out of the equation? > Nvm, I'm getting the 4 failures on a 6.6 kernel now. Something must > have changed so I'll look into it. 6.2 - works 6.5 - works 6.6 - broken I tried applying that patch [1] from Johannes onto 6.6 but it failed to apply. So anyways it appears CQM events are broken in at least 6.6. [1] https://lore.kernel.org/linux-wireless/bbfd6f959e7ff4b567084ef3d962bf255aa25c85.camel@sipsolutions.net/T/#t >> >>> >>> >>> Regards, >>> -Denis >>>
On 11/30/23 22:00, Denis Kenzior wrote: > To allow _auto_(handshake_state_free) variables to be used. > --- > src/handshake.c | 7 ++++++- > src/handshake.h | 3 +++ > 2 files changed, 9 insertions(+), 1 deletion(-) > Applied.
Hi James, >> Nvm, I'm getting the 4 failures on a 6.6 kernel now. Something must have >> changed so I'll look into it. > > 6.2 - works > > 6.5 - works > > 6.6 - broken Sigh, figures. I doubt the patch from Johannes would fix anything for us since we don't query carrier state directly if I recall correctly. We only react to events, at least on the connect path. Regards, -Denis
Hi Denis, On 12/1/23 08:32, Denis Kenzior wrote: > Hi James, > >>> Nvm, I'm getting the 4 failures on a 6.6 kernel now. Something must >>> have changed so I'll look into it. >> >> 6.2 - works >> >> 6.5 - works >> >> 6.6 - broken > > Sigh, figures. I doubt the patch from Johannes would fix anything for > us since we don't query carrier state directly if I recall correctly. > We only react to events, at least on the connect path. Two separate things here: - Confirmed that CQM events are broken at least in 6.6, this was the patch I linked in this thread. - Earlier email I sent about the carrier state was more of a suspected problem for flaky tests, before I even saw the roaming tests were failing. I haven't tried applying those kernel patches, but it would also require userspace changes checking that extra attribute. I just need to look more into this. Anyways, yes, unfortunate. At it's also broken in wpa_supplicant so its going to get a lot of visibility. Thanks, James > > Regards, > -Denis
diff --git a/src/handshake.c b/src/handshake.c index 6b93774ab137..1c5ed2c9bc12 100644 --- a/src/handshake.c +++ b/src/handshake.c @@ -105,7 +105,12 @@ void __handshake_set_install_ext_tk_func(handshake_install_ext_tk_func_t func) void handshake_state_free(struct handshake_state *s) { - __typeof__(s->free) destroy = s->free; + __typeof__(s->free) destroy; + + if (!s) + return; + + destroy = s->free; if (s->in_event) { s->in_event = false; diff --git a/src/handshake.h b/src/handshake.h index 7200c3617b8e..815eb44ff6ba 100644 --- a/src/handshake.h +++ b/src/handshake.h @@ -24,6 +24,7 @@ #include <stdbool.h> #include <asm/byteorder.h> #include <linux/types.h> +#include <ell/cleanup.h> struct handshake_state; enum crypto_cipher; @@ -298,3 +299,5 @@ const uint8_t *handshake_util_find_pmkid_kde(const uint8_t *data, size_t data_len); void handshake_util_build_gtk_kde(enum crypto_cipher cipher, const uint8_t *key, unsigned int key_index, uint8_t *to); + +DEFINE_CLEANUP_FUNC(handshake_state_free);