| Message ID | 20241104153404.21273-1-akumar@suse.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | crypto/af_alg0[13]: update tests for additional possible errno case | expand |
Hi Avinesh, > kernel behaviour wrt checking invalid algorithms has changed [1] [2] > updating the tests to address the additional errno case. > Related discussion on the mailing list [3] Thanks, merged! Kind regards, Petr
Hi Cyril, Jan, David, > kernel behaviour wrt checking invalid algorithms has changed [1] [2] > updating the tests to address the additional errno case. > Related discussion on the mailing list [3] Looking at 57ab2160c0 ("move_pages04: remove special-casing for kernels < 4.3") [4] recently reverting errnos for 4.3 d539a004dd ("move_pages04: fix zero page status code for kernels >= 4.3") [5] please double check this already merged change. I still believe it's a different case thus merging this is correct. Also Eric is suggesting this (I should have added Suggested-by: tag for him). Maybe we need some rules to clarify when we are ok with different errno and when not. I also thought there would be some rule "don't hide kernel bugs", but I can't find it in the docs. Kind regards, Petr [4] https://github.com/linux-test-project/ltp/commit/d539a004dde3b760f610ef7cae90a96de8489ec8 [5] https://github.com/linux-test-project/ltp/commit/57ab2160c0b002cbeeb1cba477cf8875ca9d660d [6] https://lore.kernel.org/lkml/20240924222839.GC1585@sol.localdomain/ > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e7a4142b35ce489fc8908d75596c51549711ade0 > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=795f85fca229a88543a0a706039f901106bf11c1 > [3] https://lore.kernel.org/lkml/20240924222839.GC1585@sol.localdomain/ > Signed-off-by: Avinesh Kumar <akumar@suse.de> > --- > lib/tst_af_alg.c | 1 + > testcases/kernel/crypto/af_alg01.c | 5 ++++- > testcases/kernel/crypto/af_alg03.c | 5 ++++- > 3 files changed, 9 insertions(+), 2 deletions(-) > diff --git a/lib/tst_af_alg.c b/lib/tst_af_alg.c > index f5437c5c5..a14f9865c 100644 > --- a/lib/tst_af_alg.c > +++ b/lib/tst_af_alg.c > @@ -103,6 +103,7 @@ bool tst_have_alg(const char *algtype, const char *algname) > case 0: > return true; > case ENOENT: > + case EINVAL: > tst_res(TCONF, "kernel doesn't have %s algorithm '%s'", > algtype, algname); > return false; > diff --git a/testcases/kernel/crypto/af_alg01.c b/testcases/kernel/crypto/af_alg01.c > index 7cefe5946..2100b3698 100644 > --- a/testcases/kernel/crypto/af_alg01.c > +++ b/testcases/kernel/crypto/af_alg01.c > @@ -21,6 +21,7 @@ static void test_with_hash_alg(const char *hash_algname) > { > char hmac_algname[64]; > char key[4096] = { 0 }; > + int ret; > if (!tst_have_alg("hash", hash_algname)) > return; > @@ -30,7 +31,9 @@ static void test_with_hash_alg(const char *hash_algname) > return; > sprintf(hmac_algname, "hmac(hmac(%s))", hash_algname); > - if (tst_try_alg("hash", hmac_algname) != ENOENT) { > + > + ret = tst_try_alg("hash", hmac_algname); > + if (ret != ENOENT && ret != EINVAL) { > int algfd; > tst_res(TFAIL, "instantiated nested hmac algorithm ('%s')!", > diff --git a/testcases/kernel/crypto/af_alg03.c b/testcases/kernel/crypto/af_alg03.c > index bb8d480e2..d7d385883 100644 > --- a/testcases/kernel/crypto/af_alg03.c > +++ b/testcases/kernel/crypto/af_alg03.c > @@ -15,10 +15,13 @@ > static void run(void) > { > + int ret; > + > tst_require_alg("aead", "rfc7539(chacha20,poly1305)"); > tst_require_alg("hash", "sha256"); > - if (tst_try_alg("aead", "rfc7539(chacha20,sha256)") != ENOENT) { > + ret = tst_try_alg("aead", "rfc7539(chacha20,sha256)"); > + if ( ret != ENOENT && ret != EINVAL) { > tst_res(TFAIL, > "instantiated rfc7539 template with wrong digest size"); > } else {
On 05.11.24 11:40, Petr Vorel wrote: > Hi Cyril, Jan, David, > >> kernel behaviour wrt checking invalid algorithms has changed [1] [2] >> updating the tests to address the additional errno case. >> Related discussion on the mailing list [3] > > Looking at 57ab2160c0 ("move_pages04: remove special-casing for kernels < 4.3") [4] > recently reverting errnos for 4.3 d539a004dd ("move_pages04: fix zero page > status code for kernels >= 4.3") [5] please double check this already merged > change. I still believe it's a different case thus merging this is correct. > Also Eric is suggesting this (I should have added Suggested-by: tag for him). > > Maybe we need some rules to clarify when we are ok with different errno and when not. > Right. Regarding d539a004dd, we pretty much hid kernel bugs: behaving differently than expected+documented. If the kernel starts reporting a different errno it might be a bug: user space might not be prepared to handle that. Or it might be expected, because nobody really cares about the exact error code. So if a test starts failing, it's definitely concerning and needs a closer look. > I also thought there would be some rule "don't hide kernel bugs", but I can't > find it in the docs. That rule makes sense to me.
> On 05.11.24 11:40, Petr Vorel wrote: > > Hi Cyril, Jan, David, > > > kernel behaviour wrt checking invalid algorithms has changed [1] [2] > > > updating the tests to address the additional errno case. > > > Related discussion on the mailing list [3] > > Looking at 57ab2160c0 ("move_pages04: remove special-casing for kernels < 4.3") [4] > > recently reverting errnos for 4.3 d539a004dd ("move_pages04: fix zero page > > status code for kernels >= 4.3") [5] please double check this already merged > > change. I still believe it's a different case thus merging this is correct. > > Also Eric is suggesting this (I should have added Suggested-by: tag for him). > > Maybe we need some rules to clarify when we are ok with different errno and when not. > Right. > Regarding d539a004dd, we pretty much hid kernel bugs: behaving differently > than expected+documented. > If the kernel starts reporting a different errno it might be a bug: user > space might not be prepared to handle that. Or it might be expected, because > nobody really cares about the exact error code. I don't remember last time anybody noticed, but sure it's important. FYI an example where errno change was not important enough was for bind, where commit 0fb44559ffd6 ("af_unix: move unix_mknod() out of bindlock") did some fixes and a side effect was that errno on an attempt to bind a unix socket to the same path twice changed from -EINVAL to -EADDRINUSE [1]. Bug which that kernel commit fixed was way more important than changing errno [2] (and therefore backported to all stable/LTS mainline kernels [3]). The attempt to to fix errno [1] was just considered as not needed. But this might be a special case, because both errnos were listed in the man page, it was just matter of priority which of the error checks was handled first. First attempt to fix the affected bind03.c was to detect kernel version [4], which was wrong (some versions in between did not get the fix). In the end it was possible to adjust test to get always expected errno regardless kernel got 0fb44559ffd6 backported or not [5]. Back to move_pages04. Jan wrote [6]: If people find it too noisy now for older kernels, we could add .min_kver = "" to the test. I guess nothing happen, because 4.3 is way too old (the oldest mainline LTS kernel is 4.19, IMHO nobody tests 4.3 kernel with current LTP). But having a written rules 1) when we care about errno change and when not and what to do in both cases would be useful. > So if a test starts failing, it's definitely concerning and needs a closer > look. > > I also thought there would be some rule "don't hide kernel bugs", but I can't > > find it in the docs. > That rule makes sense to me. I was not clear. We follow "don't hide kernel bugs" (at least Cyril mentioned this several times), but I haven't found if we document it somewhere in doc/ folder. Kind regards, Petr [1] https://lore.kernel.org/netdev/20170630073448.GA9546@unicorn.suse.cz/ [2] https://lore.kernel.org/netdev/CAK8P3a1q32spcF445Zhw-KMXG2VwFZuMw5C1sYFk3qLXz3HB5w@mail.gmail.com/ [3] https://lore.kernel.org/netdev/20181129123650.GI3149@kroah.com/ [4] https://lore.kernel.org/ltp/20180831132453.6461-1-junchi.chen@intel.com/ [5] https://lore.kernel.org/ltp/20180911102403.7094-1-junchi.chen@intel.com/ [6] https://lore.kernel.org/ltp/CAASaF6yLa6F9Bz9Adck=Y5RJePzYmboSAizYWJP_BHmy12cQ=g@mail.gmail.com/
On 05.11.24 19:05, Petr Vorel wrote: >> On 05.11.24 11:40, Petr Vorel wrote: >>> Hi Cyril, Jan, David, > >>>> kernel behaviour wrt checking invalid algorithms has changed [1] [2] >>>> updating the tests to address the additional errno case. >>>> Related discussion on the mailing list [3] > >>> Looking at 57ab2160c0 ("move_pages04: remove special-casing for kernels < 4.3") [4] >>> recently reverting errnos for 4.3 d539a004dd ("move_pages04: fix zero page >>> status code for kernels >= 4.3") [5] please double check this already merged >>> change. I still believe it's a different case thus merging this is correct. >>> Also Eric is suggesting this (I should have added Suggested-by: tag for him). > >>> Maybe we need some rules to clarify when we are ok with different errno and when not. > > >> Right. > >> Regarding d539a004dd, we pretty much hid kernel bugs: behaving differently >> than expected+documented. > >> If the kernel starts reporting a different errno it might be a bug: user >> space might not be prepared to handle that. Or it might be expected, because >> nobody really cares about the exact error code. > > I don't remember last time anybody noticed, but sure it's important. > > FYI an example where errno change was not important enough was for bind, where > commit 0fb44559ffd6 ("af_unix: move unix_mknod() out of bindlock") did some > fixes and a side effect was that errno on an attempt to bind a unix socket to > the same path twice changed from -EINVAL to -EADDRINUSE [1]. Bug which that > kernel commit fixed was way more important than changing errno [2] (and > therefore backported to all stable/LTS mainline kernels [3]). > > The attempt to to fix errno [1] was just considered as not needed. But this > might be a special case, because both errnos were listed in the man page, it was > just matter of priority which of the error checks was handled first. > > First attempt to fix the affected bind03.c was to detect kernel version [4], > which was wrong (some versions in between did not get the fix). In the end it > was possible to adjust test to get always expected errno regardless kernel got > 0fb44559ffd6 backported or not [5]. > > Back to move_pages04. Jan wrote [6]: > If people find it too noisy now for older kernels, we could add .min_kver = "" > to the test. > > I guess nothing happen, because 4.3 is way too old (the oldest mainline LTS kernel is 4.19, IMHO nobody tests 4.3 kernel with current LTP). Yes, likely. > > But having a written rules 1) when we care about errno change and when not and > what to do in both cases would be useful. Agreed. > >> So if a test starts failing, it's definitely concerning and needs a closer >> look. > >>> I also thought there would be some rule "don't hide kernel bugs", but I can't >>> find it in the docs. > >> That rule makes sense to me. > > I was not clear. We follow "don't hide kernel bugs" (at least Cyril mentioned > this several times), but I haven't found if we document it somewhere in doc/ > folder. Ah, yes that's what I meant: this unofficial rule makes sense to me :)
diff --git a/lib/tst_af_alg.c b/lib/tst_af_alg.c index f5437c5c5..a14f9865c 100644 --- a/lib/tst_af_alg.c +++ b/lib/tst_af_alg.c @@ -103,6 +103,7 @@ bool tst_have_alg(const char *algtype, const char *algname) case 0: return true; case ENOENT: + case EINVAL: tst_res(TCONF, "kernel doesn't have %s algorithm '%s'", algtype, algname); return false; diff --git a/testcases/kernel/crypto/af_alg01.c b/testcases/kernel/crypto/af_alg01.c index 7cefe5946..2100b3698 100644 --- a/testcases/kernel/crypto/af_alg01.c +++ b/testcases/kernel/crypto/af_alg01.c @@ -21,6 +21,7 @@ static void test_with_hash_alg(const char *hash_algname) { char hmac_algname[64]; char key[4096] = { 0 }; + int ret; if (!tst_have_alg("hash", hash_algname)) return; @@ -30,7 +31,9 @@ static void test_with_hash_alg(const char *hash_algname) return; sprintf(hmac_algname, "hmac(hmac(%s))", hash_algname); - if (tst_try_alg("hash", hmac_algname) != ENOENT) { + + ret = tst_try_alg("hash", hmac_algname); + if (ret != ENOENT && ret != EINVAL) { int algfd; tst_res(TFAIL, "instantiated nested hmac algorithm ('%s')!", diff --git a/testcases/kernel/crypto/af_alg03.c b/testcases/kernel/crypto/af_alg03.c index bb8d480e2..d7d385883 100644 --- a/testcases/kernel/crypto/af_alg03.c +++ b/testcases/kernel/crypto/af_alg03.c @@ -15,10 +15,13 @@ static void run(void) { + int ret; + tst_require_alg("aead", "rfc7539(chacha20,poly1305)"); tst_require_alg("hash", "sha256"); - if (tst_try_alg("aead", "rfc7539(chacha20,sha256)") != ENOENT) { + ret = tst_try_alg("aead", "rfc7539(chacha20,sha256)"); + if ( ret != ENOENT && ret != EINVAL) { tst_res(TFAIL, "instantiated rfc7539 template with wrong digest size"); } else {
kernel behaviour wrt checking invalid algorithms has changed [1] [2] updating the tests to address the additional errno case. Related discussion on the mailing list [3] [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e7a4142b35ce489fc8908d75596c51549711ade0 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=795f85fca229a88543a0a706039f901106bf11c1 [3] https://lore.kernel.org/lkml/20240924222839.GC1585@sol.localdomain/ Signed-off-by: Avinesh Kumar <akumar@suse.de> --- lib/tst_af_alg.c | 1 + testcases/kernel/crypto/af_alg01.c | 5 ++++- testcases/kernel/crypto/af_alg03.c | 5 ++++- 3 files changed, 9 insertions(+), 2 deletions(-)