Re: [PATCH v4 1/6] io_uring/mock: add basic infra for test mock files

[Jens Axboe <axboe@kernel.dk>]

On 5/30/25 9:11 AM, Pavel Begunkov wrote:
> On 5/30/25 15:41, Jens Axboe wrote:
>> On 5/30/25 8:26 AM, Pavel Begunkov wrote:
>>> On 5/30/25 15:12, Pavel Begunkov wrote:
>>>> On 5/30/25 15:09, Pavel Begunkov wrote:
>>>>> On 5/30/25 14:28, Jens Axboe wrote:
>>>>>> On 5/30/25 6:51 AM, Pavel Begunkov wrote:
>>>>>>> diff --git a/init/Kconfig b/init/Kconfig
>>>>>>> index 63f5974b9fa6..9e8a5b810804 100644
>>>>>>> --- a/init/Kconfig
>>>>>>> +++ b/init/Kconfig
>>>>>>> @@ -1774,6 +1774,17 @@ config GCOV_PROFILE_URING
>>>>>>>          the io_uring subsystem, hence this should only be enabled for
>>>>>>>          specific test purposes.
>>>>>>> +config IO_URING_MOCK_FILE
>>>>>>> +    tristate "Enable io_uring mock files (Experimental)" if EXPERT
>>>>>>> +    default n
>>>>>>> +    depends on IO_URING && KASAN
>>>>>>> +    help
>>>>>>> +      Enable mock files for io_uring subststem testing. The ABI might
>>>>>>> +      still change, so it's still experimental and should only be enabled
>>>>>>> +      for specific test purposes.
>>>>>>> +
>>>>>>> +      If unsure, say N.
>>>>>>
>>>>>> As mentioned in the other email, I don't think we should include KASAN
>>>>>> here.
>>>>>
>>>>> I disagree. It's supposed to give a superset of coverage, if not,
>>>>> mocking should be improved. It might be seen as a nuisance that you
>>>>> can't run it with a stock kernel, but that desire is already half
>>>>> step from "let's enable it for prod kernels for testing", and then
>>>>> distributions will start forcing it on, because as you said "People
>>>>> do all sorts of weird stuff".
>>>>
>>>> The purpose is to get the project even more hardened / secure through
>>>> elaborate testing, that would defeat the purpose if non test systems
>>>> will start getting errors because of some mess up, let's say in the
>>>> driver.
>>>
>>> Alternatively, it doesn't help with bloating, but tainting the kernel
>>> might be enough to serve the purpose.
>>
>> I think taint or KASAN dependencies is over-reaching. It has nothing to
>> do with KASAN, and there's absolutely zero reason for it to be gated on
>> KASAN (or lockdep, or whatever). You're never going to prevent people
>> from running this in odd cases, and I think it's a mistake to try and do
>> that. If the thing is gated on CAP_SYS_ADMIN, then that's Good Enough
>> imho.
>>
>> It'll make my life harder for coverage testing, which I think is reason
>> enough alone to not have a KASAN dependency. No other test code in the
>> kernel has unrelated dependencies like KASAN, unless they are related to
>> KASAN. We should not add one here for some notion of preventing people
>> from running it on prod stuff, in fact it should be totally fine to run
>> on a prod kernel. Might actually be useful in some cases, to verify or
>> test some behavior on that specific kernel, without needing to build a
>> new kernel for it.
> 
> commit 2852ca7fba9f77b204f0fe953b31fadd0057c936
> Author: David Gow <davidgow@google.com>
> Date:   Fri Jul 1 16:47:41 2022 +0800
> 
>     panic: Taint kernel if tests are run
>         Most in-kernel tests (such as KUnit tests) are not supposed to run on
>     production systems: they may do deliberately illegal things to trigger
>     errors, and have security implications (for example, KUnit assertions
>     will often deliberately leak kernel addresses).
>         Add a new taint type, TAINT_TEST to signal that a test has been run.
>     This will be printed as 'N' (originally for kuNit, as every other
>     sensible letter was taken.)
>         This should discourage people from running these tests on production
>     systems, and to make it easier to tell if tests have been run
>     accidentally (by loading the wrong configuration, etc.)
>         Acked-by: Luis Chamberlain <mcgrof@kernel.org>
>     Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
>     Signed-off-by: David Gow <davidgow@google.com>
>     Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> 
> The same situation, it's a special TAINT_TEST, and set for a good
> reason. And there is also a case of TAINT_CRAP for staging.

TAINT is fine, I don't care about that. So we can certainly do that. My
main objection is just to gating it on lockdep/kasan or something like
that.

-- 
Jens Axboe