Re: [PATCH] crypto: tcrypt - fix return value for multiple subtests

[Anirudh Venkataramanan]

On 9/30/2022 3:42 PM, Elliott, Robert (Servers) wrote:
> > -----Original Message-----
> > From: Anirudh Venkataramanan <anirudh.venkataramanan@xxxxxxxxx>
> > Sent: Friday, September 30, 2022 5:10 PM
> > To: Elliott, Robert (Servers) <elliott@xxxxxxx>; herbert@xxxxxxxxxxxxxxxxxxx;
> > davem@xxxxxxxxxxxxx; jarod@xxxxxxxxxx; linux-crypto@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH] crypto: tcrypt - fix return value for multiple subtests
> >
> > On 9/30/2022 2:40 PM, Robert Elliott wrote:
> > > When a test mode invokes multiple tests (e.g., mode 0 invokes modes
> > > 1 through 199, and mode 3 tests three block cipher modes with des),
> > > don't keep accumulating the return values with ret += tcrypt_test(),
> > > which results in a bogus value if more than one report a nonzero
> > > value (e.g., two reporting -2 (-ENOENT) end up reporting -4 (-EINTR)).
> > > Instead, keep track of the minimum return value reported by any
> > > subtest.
> >
> > I am assuming this is for the case when fips_enabled is true?
>
> I have some other unposted patches that print more info on the
> test progress including the return values at various levels.

To what end? What is the problem you're trying to solve?

> The Fedora 36 .config on x86 yields 23 -2 (ENOENT) errors, so
> the overall result is -46 (which is defined as EPFNOSUPPORT).

yeah, but the return value to userspace would always be -EAGAIN (-11) 
unless fips_enabled is true.

> > I agree that returning the cumulative sum or errors isn't particularly
> > useful, but how is returning the minimum error value useful? Wouldn't it
> > be more useful to return the first error return?
>
> The first error would be more useful, but would require more complex
> changes. Is there any kernel macro that would handle this in one line?

Actually, thinking about this some more, the first error isn't 
particularly useful either, because to find out what failed you have to 
do more digging anyway. Userspace just gets a 0/negative return value, 
and the negative value doesn't mean anything other than "there was an 
error", and that too only if fips_enabled is true.

>    tmp = tcrypt_test();
>    if (tmp && !ret)
>      ret = tmp;
>
> Since do_test() and tcrypt_test() are static inline functions
> only used within this file, a new argument containing a pointer
> to the return value could be added that lets them handle
> updating it while keeping the callers simple.
>
>     ret += tcrypt_test("md5");
> could become
>     tcrypt_test("md5", &ret);