stage1/kvm: correctly bind-mount read-only volumes #3304
Conversation
|
LGTM |
lucab
left a comment
There was a problem hiding this comment.
A couple of change requests. Can you please also add the missing testcase?
stage1/init/kvm.go
Outdated
|
|
||
| // Linux can't bind-mount with readonly in a single operation, so remount +ro | ||
| if readOnly { | ||
| if err := syscall.Mount("", destination, "bind", syscall.MS_REMOUNT|syscall.MS_RDONLY|syscall.MS_BIND, ""); err != nil { |
There was a problem hiding this comment.
I think you should use flags here, otherwise you may be dropping the MS_REC flag.
There was a problem hiding this comment.
MS_REC is not needed here. For whatever reason, you can't MS_REC a MS_REMOUNT.
stage1/init/kvm.go
Outdated
| // Linux can't bind-mount with readonly in a single operation, so remount +ro | ||
| if readOnly { | ||
| if err := syscall.Mount("", destination, "bind", syscall.MS_REMOUNT|syscall.MS_RDONLY|syscall.MS_BIND, ""); err != nil { | ||
| syscall.Unmount(destination, 0) // If err, try to unmount, fail |
There was a problem hiding this comment.
I'm not sure we want to unmount on err. I guess this will just terminate the guest, automatically killing the leftover mounts, right?
|
great catch! Is there an issue somewhere about sharing this logic between the stage1s..? |
|
@jonboulle, No, no issue. There is a snarky TODO I added though :-). (I'll create one) |
|
filed #3311 for cleanup. |
|
@lucab updated the tests too, PTAL. Thanks! |
lucab
left a comment
There was a problem hiding this comment.
LGTM. Even though I'm still not completely sure this is 100% correct, it is at least saner than before.
Fixes #3302