[WIP] fix SetKeyLabel in old kernels with selinux enabled #52
Conversation
|
I think we should not be hurry to close this PR, we need a full discussion to reduce regressions which have impact to runc. |
lifubang
changed the title
lifubang
force-pushed
the
oldkernel
branch
2 times, most recently
from
743263d to
645c7cc
Compare
|
I spent a whole morning dig into this issue. And find out in this old kernel with (1) (2) When we write (3) When we read (4) After we write So I think we can use I have submit the latest commit. PTAL @cyphar @rhatdan Thanks. |
go-selinux/label/label_selinux.go
Outdated
| } | ||
| } | ||
| err := selinux.SetKeyLabel(processLabel) | ||
| if err != nil && processLabel == "" && os.IsNotExist(err) { |
There was a problem hiding this comment.
I don't think processLabel == "" should be here. really it should just be:
if os.IsNotExist(err) {
}
With the note that os.IsNotExist(nil) == false.
There was a problem hiding this comment.
Thanks for your review. I have update it.
go-selinux/label/label_selinux.go
Outdated
| return nil | ||
| } | ||
| } | ||
| err := selinux.SetKeyLabel(processLabel) |
There was a problem hiding this comment.
Why not put this check inside selinux.SetKeyLabel? Seems a bit odd to put it in this wrapper.
There was a problem hiding this comment.
Yes, it is.
If the user just use "github.com/opencontainers/selinux/go-selinux", not "github.com/opencontainers/selinux/go-selinux/label".
I have update it.
go-selinux/selinux_linux.go
Outdated
| func SetKeyLabel(label string) error { | ||
| return writeCon("/proc/self/attr/keycreate", label) | ||
| if label == "" && GetEnabled() { | ||
| if _, err := KeyLabel(); err == io.EOF { |
There was a problem hiding this comment.
Why not check ENoExists here?
There was a problem hiding this comment.
Thanks, Because we add it after write action. Anyway, I think we can add. I will add it later.
There was a problem hiding this comment.
And the ENoExists check after writeCon should also be remained.
There was a problem hiding this comment.
Thank you for your review, it will reduce one file write call, does these code change make sense?
There was a problem hiding this comment.
It will still fail on Permission Denied. If the user tries to write to a KeyLabel they are allowed to read but not allowed to write.
I am not even sure what the io.EOF does?
There was a problem hiding this comment.
I think it does make sense that Permission Denied err when write to keycreate should be throwed to runc.
There was a problem hiding this comment.
I am not even sure what the io.EOF does?
I think it is what is the older kernel really mean! It also be very very strange to me, but it really happens. I think this is a selinux bug in this old kernel.
There was a problem hiding this comment.
What OS and What Kernel?
I think I have provided it in PR description.
In some old kernels, for example, 3.10.0-862.el7.x86_64 in RedHat Enterprise Linux 7.5.
With selinux enabled.
lifubang
force-pushed
the
oldkernel
branch
3 times, most recently
from
336944d to
f8fdde7
Compare
Signed-off-by: Lifubang <lifubang@acmcoder.com>
lifubang
changed the title
|
@rhatdan Could you tell me when you submit the PR opencontainers/runc#2012, you test it in what OS version and what kernel version? |
|
I tested it in Fedora. |
|
Note there is also a regression with just the default docker setup (where |
|
The other patch got merged. |
For the sake of other lost souls reading this, "the other patch" here means #49 |
5 participants
In some old kernels, for example, 3.10.0-862.el7.x86_64 in RedHat Enterprise Linux 7.5.
With
selinux enabled.SetKeyLabelwith empty string""in the first time is not supported.But with non empty string is supported.
It will cause runc fail if
selinuxLabelis""inconfig.jsonin these kernels.Signed-off-by: Lifubang lifubang@acmcoder.com