Re: seccomp ptrace selftest failures with 4.4-stable [Was: Re: LTS testing with latest kselftests - some failures]

[Tom Gall <tom.gall@linaro.org>]

Hi

On Thu, Jun 22, 2017 at 2:06 PM, Shuah Khan <shuah@kernel.org> wrote:
> On 06/22/2017 11:50 AM, Kees Cook wrote:
>> On Thu, Jun 22, 2017 at 10:49 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>> On Thu, Jun 22, 2017 at 10:09 AM, Shuah Khan <shuah@kernel.org> wrote:
>>>> On 06/22/2017 10:53 AM, Kees Cook wrote:
>>>>> On Thu, Jun 22, 2017 at 9:18 AM, Sumit Semwal <sumit.semwal@linaro.org> wrote:
>>>>>> Hi Kees, Andy,
>>>>>>
>>>>>> On 15 June 2017 at 23:26, Sumit Semwal <sumit.semwal@linaro.org> wrote:
>>>>>>> 3. 'seccomp ptrace hole closure' patches got added in 4.7 [3] -
>>>>>>> feature and test together.
>>>>>>> - This one also seems like a security hole being closed, and the
>>>>>>> 'feature' could be a candidate for stable backports, but Arnd tried
>>>>>>> that, and it was quite non-trivial. So perhaps  we'll need some help
>>>>>>> from the subsystem developers here.
>>>>>>
>>>>>> Could you please help us sort this out? Our goal is to help Greg with
>>>>>> testing stable kernels, and currently the seccomp tests fail due to
>>>>>> missing feature (seccomp ptrace hole closure) getting tested via
>>>>>> latest kselftest.
>>>>>>
>>>>>> If you feel the feature isn't a stable candidate, then could you
>>>>>> please help make the test degrade gracefully in its absence?
>
> In some cases, it is not easy to degrade and/or check for a feature.
> Probably several security features could fall in this bucket.
>
>>>>>
>>>>> I don't really want to have that change be a backport -- it's quite
>>>>> invasive across multiple architectures.
>
> Agreed. The same test for kernel applies to tests as well. If a kernel
> feature can't be back-ported, the test for that feature will fall in the
> same bucket. It shouldn't be back-ported.
>
>>>>>
>>>>> I would say just add a kernel version check to the test. This is
>>>>> probably not the only selftest that will need such things. :)
>>>>
>>>> Adding release checks to selftests is going to problematic for maintenance.
>>>> Tests should fail gracefully if feature isn't supported in older kernels.
>>>>
>>>> Several tests do that now and please find a way to check for dependencies
>>>> and feature availability and fail the test gracefully. If there is a test
>>>> that can't do that for some reason, we can discuss it, but as a general
>>>> rule, I don't want to see kselftest patches that check release.
>>>
>>> If a future kernel inadvertently loses the new feature and degrades to
>>> the behavior of old kernels, that would be a serious bug and should be
>>> caught.
>
> Agreed. If I understand you correctly, by not testing stable kernels
> with their own selftests, some serious bugs could go undetected.

Personally I'm a bit skeptical. I think the reasoning is more that the
latest selftests provide more coverage, and therefore should be better
tests, even on older kernels.

>>
>> Right. I really think stable kernels should be tested with their own
>> selftests. If some test is needed in a stable kernel it should be
>> backported to that stable kernel.
>
> Correct. This is always a safe option. There might be cases that even
> prevent tests being built, especially if a new feature adds new fields
> to an existing structure.
>
> It appears in some cases, users want to run newer tests on older kernels.
> Some tests can clearly detect feature support using module presence and/or
> Kconfig enabled or disabled. These are conditions even on a kernel that
> supports a new module or new config option. The kernel the test is running
> on might not have the feature enabled or module might not be present. In
> these cases, it would be easier to detect and skip the test.
>
> However, some features aren't so easy. For example:
>
> - a new flag is added to a syscall, and new test is added. It might not
>   be easy to detect that.
> - We might have some tests that can't detect and skip.
>
> Based on this discussion, it is probably accurate to say:
>
> 1. It is recommended that selftests from the same release be run on the
>    kernel.
> 2. Selftests from newer kernels will run on older kernels, user should
>    understand the risks such as some tests might fail and might not
>    detect feature degradation related bugs.
> 3. Selftests will fail gracefully on older releases if at all possible.

How about gracefully be skipped instead of fail?

The later suggests the test case in some situations can detect it's
pointless to run something and say as much instead of emitting a
failure that would be a waste of time to look into.

As another example take tools/testing/selftests/net/psock_fanout.c
On 4.9 it'll fail to compile (using master's selftests) because
PACKET_FANOUT_FLAG_UNIQUEID isn't defined. Add a simple #ifdef for
that symbol and the psock_fanout test will compile and run just fine.

> Sumit!
>
> 1. What are the reasons for testing older kernel with selftests from
>    newer kernels? What are the benefits you see for doing so?

I think the presumption is the latest greatest collection of selftests
are the best, most complete.

>    I am looking to understand the need/reasons for this use-case. In our
>    previous discussion on this subject, I did say, you should be able to
>    do so with some exceptions.
>
> 2. Do you test kernels with the selftests from the same release?

We have the ability to do either. The new shiny .... it calls.

> 3. Do you find testing with newer selftests to be useful?

I think it comes down to coverage and again the current perception
that latest greatest is better. Quantitatively we haven't collected
data to support that position tho it would be interesting to compare
say a 4.4-lts and it's selftests directory to a mainline, see how much
was new and then find out how much of those new selftests actually
work on the older 4.4-lts.

> thanks,
> -- Shuah


-- 
Regards,
Tom

Director, Linaro Mobile Group
Linaro.org │ Open source software for ARM SoCs
irc: tgall_foo | skype : tom_gall

"Where's the kaboom!? There was supposed to be an earth-shattering
kaboom!" Marvin Martian