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

[Tom Gall]

Hi Shuah,

On Fri, Jun 23, 2017 at 2:03 PM, Shuah Khan <shuah@xxxxxxxxxx> wrote:
> On 06/22/2017 01:48 PM, Tom Gall wrote:
>> Hi
>>
>> On Thu, Jun 22, 2017 at 2:06 PM, Shuah Khan <shuah@xxxxxxxxxx> wrote:
>>> On 06/22/2017 11:50 AM, Kees Cook wrote:
>>>> On Thu, Jun 22, 2017 at 10:49 AM, Andy Lutomirski <luto@xxxxxxxxxx> wrote:
>>>>> On Thu, Jun 22, 2017 at 10:09 AM, Shuah Khan <shuah@xxxxxxxxxx> wrote:
>>>>>> On 06/22/2017 10:53 AM, Kees Cook wrote:
>>>>>>> On Thu, Jun 22, 2017 at 9:18 AM, Sumit Semwal <sumit.semwal@xxxxxxxxxx> wrote:
>>>>>>>> Hi Kees, Andy,
>>>>>>>>
>>>>>>>> On 15 June 2017 at 23:26, Sumit Semwal <sumit.semwal@xxxxxxxxxx> 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.
>
> Unfortunately adding PACKET_FANOUT_FLAG_UNIQUEID isn't correct. This is
> a new feature that went into 4.12. You don't want to define this for older
> kernel which will break on newer kernels.

My thought here wasn't to define the symbol, but to detect it.

If it's not defined, you know it's not an available feature and thus
avoid that portion of the test.

> This is one concern I have that in am attempt to fix mainline selftest to
> be able to test older kernels will cause problems.

As Greg had mentioned, if we stay focused on features and function in
kernel, many tests shouldn't care what version of the kernel they are
on.

Likewise those tests that are for new function need to be
compartmentalized to be separate and not be mixed in in such a way
that a compile time failure cause parts of a test to not be built that
would be perfectly valid should the feature not be available.

> This is a good example of a new test that has dependency on a new define
> in an include file which will be hard to check - compile will fail on
> older kernels. The rest of the net tests will compile and run.

To me seeing a bunch of preventable ugly compile errors which don't
give guidance if they are stemming from invalid test cases seems like
something we should address. That sorta suggests there's some kind of
meta that decides to either build or not build a test based on feature
existence. And I really mean existence not kernel version.

That suggests the following rule, sefltests should always either
successfully build or not be built at all.

Anyway, I'm going to shut up until I have patches.

> -- Shuah
>
>>
>>> 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