ebpf cgroupdev program: fix device permission check issue #229
Conversation
Related to: NVIDIA#227 Signed-off-by: Raphael <[email protected]>
src/nvcgo/internal/cgroup/ebpf.go
Outdated
| @@ -136,10 +136,10 @@ func (p *program) appendDevice(dev specs.LinuxDeviceCgroup, labelPrefix string) | |||
| } | |||
| if hasAccess { | |||
| p.insts = append(p.insts, | |||
| // if (R3 & bpfAccess == 0 /* use R2 as a temp var */) goto next | |||
| // if (R3 & bpfAccess != R3 /* use R2 as a temp var */) goto next | |||
| asm.Mov.Reg32(asm.R2, asm.R3), | |||
There was a problem hiding this comment.
Both runc and crun use R1 as a temporary variable here. Could it cause issues that we're using R2 since we're also using R2 in the hasAccess block above?
cc @klueska
There was a problem hiding this comment.
@elezar yes you're right ! Since we erase R2 the bpfType check in the next block will break. Interesting that the issue was never encountered before. Fixing: I'll just use a free register, R6 (I guess using R1 as you suggest would be safe as we have extracted all we needed from the struct)
There was a problem hiding this comment.
Let's rather use R1 since to align with the other implementations.
…r the next checks Signed-off-by: Raphael Glon <[email protected]>
src/nvcgo/internal/cgroup/ebpf.go
Outdated
| asm.Mov.Reg32(asm.R2, asm.R3), | ||
| asm.And.Imm32(asm.R2, bpfAccess), | ||
| asm.JEq.Imm(asm.R2, 0, nextBlockSym), | ||
| // if (R3 & bpfAccess != R3 /* use R6 as a temp var */) goto next |
There was a problem hiding this comment.
Let's use R1 to align with the crun and runc implementations
(it's ok, R1 holds the bpf ctx struct, everything has been extracted and stored in other registers) Signed-off-by: Raphael Glon <[email protected]>
Signed-off-by: Evan Lezar <[email protected]>
|
I have created https://gitlab.com/nvidia/container-toolkit/libnvidia-container/-/merge_requests/235 with these changes since we develop against GitLab. (If you prefer to create your own MR, please do so). |
can't not allowed to push code there |
No problem. Is it ok if I've pulled the commits overs, or would you prefer I reauthored this? |
|
Thanks. I have commented here: https://gitlab.com/nvidia/container-toolkit/libnvidia-container/-/merge_requests/236#note_1643699900 |
|
Closing, this pr, code has been merged, discussion logs available here: |
Related to:
#227