seccomp: Use explicit DefaultErrnoRet

[rata]

Since commit "seccomp: Sync fields with runtime-spec fields"
(5d24467) we support to specify the
DefaultErrnoRet to be used.

Before that commit it was not specified and EPERM was used by default.
This commit keeps the same behaviour but just makes it explicit that the
default is EPERM.

Signed-off-by: Rodrigo Campos [email protected]

This a follow-up of #42604 (comment), as suggested by @thaJeztah. This just adds an explicit default to EPERM. Right now the default is EPERM but is implicit.

- What I did
Added the field DefaultErrnoRet to seccomp profiles

- How I did it
Added the field to default profile in profiles/seccomp/default_linux.go, then adjusted the tests to expect this new field

- How to verify it
You can run unit tests with: TESTDIRS='github.com/docker/docker/profiles/seccomp' make test-unit.
You can also run hack/validate/default-seccomp to verify nothing was missing in the changes.

- Description for the changelog

Add an explicit DefaultErrnoRet field in the default seccomp profile. No behavior change.

- A picture of a cute animal (not mandatory but encouraged)

[thaJeztah]

Thanks! Note that I also opened #42648, which might conflict with this one

[rata]

@thaJeztah oh, nice PR. I was thinking to open an issue about doing something like that. Feel free to close this and cherry-pick the commit, then!

Let me know if you prefer that or if I should polish this PR. Both options are fine for me :)

[thaJeztah]

If you don't mind the rebase (if my PR gets accepted, and merged first), then I'd say; keep this one open 👍 they're addressing different things, so it makes sense to review/merge separately.

[rata]

@thaJeztah thanks a lot! I've just rebased on latest master, completed the PR description template and addressed the comments (I think :)).

I've also run go generate ./profiles/seccomp/ to make sure the generated profile works and hack/validate/default-seccomp to validate all is fine :)

Let me know if there is anything else missing!

Thanks again

[thaJeztah]

LGTM, thanks!

[thaJeztah]

@AkihiroSuda @justincormack PTAL

[rata]

The CI failure seems unrelated to this PR, IIUC.

[rata]

@thaJeztah @justincormack PTAL. Now it is only one commit, brain fart on my end, it is not needed to change files in daemon/ as I explained in this comment: #42649 (comment)

I did add the defaultErrnoRet field to the example.json fixture, though, as I want the TestLoadProfile to excercise it with that field now that the field is there by default. We do have another function to test this field here, but I think it is worth adding it to the example.json, as that is a more "interesting" example to test the field too.

Please let me know what you think, sorry for the brain fart! :)

[cyphar]

So, switching the default to ENOSYS is probably a good thing overall (given that clone3 was missed with the seccomp update it seems the runc behaviour is suboptimal -- though in fairness we are fairly restrained by libseccomp limitations). However, there should be some inverse rules defined to return EPERM for syscalls that are explicitly being blocked (unfortunately due to libseccomp limitations some rules cannot be inverted, but most of the rules Docker uses can be -- I know some podman ones can't though).

[rata]

@cyphar thanks! The idea is to first add the DefaultErrnoRet to the default value (i.e. no behavior change), so it is explicit on the profile. And then, once this change is done, discuss changing the default or not and how. This is what @thaJeztah suggested here.

Once this is merged I was planning to open an issue to discuss this, I can cc you when creating the issue :)

[cyphar]

@rata

The idea is to first add the DefaultErrnoRet to the default value (i.e. no behavior change), so it is explicit on the profile.

Oh sorry I misread the title of the PR. Yeah that sounds good to me -- sorry for the noise!

[cpuguy83]

LGTM

[thaJeztah]

LGTM, thanks!