Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

readonly volume should be bind mounted readonly on the host #3042

Merged
merged 2 commits into from Nov 4, 2020

Conversation

bergwolf
Copy link
Member

@bergwolf bergwolf commented Nov 2, 2020

So that even if the guest is compromised, we still ensure that readonly volumes cannot be modified.

backport kata-containers/kata-containers#1062

So that we get protected at the VM boundary not just the guest kernel.

Signed-off-by: Peng Tao <bergwolf@hyper.sh>
@bergwolf bergwolf added no-backport-needed Changed do not need to be applied to an older branch / repository no-forward-port-needed Changed do not need to be applied to a newer branch / repository labels Nov 2, 2020
Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bergwolf

@awprice
Copy link
Contributor

awprice commented Nov 3, 2020

Thanks for raising this @bergwolf - code works in my environment on 1.11.x and fixes the issue.

virtcontainers/mount.go Outdated Show resolved Hide resolved
Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @bergwolf - just one nit

bindmount remount events are not propagated through mount subtrees,
so we have to remount the shared dir mountpoint directly.

E.g.,
```
mkdir -p source dest foo source/foo

mount -o bind --make-shared source dest

mount -o bind foo source/foo
echo bind mount rw
mount | grep foo
echo remount ro
mount -o remount,bind,ro source/foo
mount | grep foo
```
would result in:
```
bind mount rw
/dev/xvda1 on /home/ubuntu/source/foo type ext4 (rw,relatime,discard,data=ordered)
/dev/xvda1 on /home/ubuntu/dest/foo type ext4 (rw,relatime,discard,data=ordered)
remount ro
/dev/xvda1 on /home/ubuntu/source/foo type ext4 (ro,relatime,discard,data=ordered)
/dev/xvda1 on /home/ubuntu/dest/foo type ext4 (rw,relatime,discard,data=ordered)
```

The reason is that bind mount creats new mount structs and attaches them to different mount subtrees.
However, MS_REMOUNT only looks for existing mount structs to modify and does not try to propagate the
change to mount structs in other subtrees.

Fixes: kata-containers#3041
Signed-off-by: Peng Tao <bergwolf@hyper.sh>
@bergwolf
Copy link
Member Author

bergwolf commented Nov 4, 2020

/test

@codecov
Copy link

codecov bot commented Nov 4, 2020

Codecov Report

Merging #3042 into master will increase coverage by 0.10%.
The diff coverage is 28.57%.

@@            Coverage Diff             @@
##           master    #3042      +/-   ##
==========================================
+ Coverage   50.23%   50.34%   +0.10%     
==========================================
  Files         120      120              
  Lines       15842    15844       +2     
==========================================
+ Hits         7959     7976      +17     
+ Misses       6799     6783      -16     
- Partials     1084     1085       +1     

@amshinde
Copy link
Member

amshinde commented Nov 4, 2020

@chavafg Is the jenkins-metrics-ubuntu-18-04 CI failing?
I don't see this PR causing a failure in the metrics. Can you take a look.

@chavafg
Copy link
Contributor

chavafg commented Nov 4, 2020

The job looks stable, only failing in this PR with blogbench metric going out over the limit with a percentage value of 110.7%.

@amshinde
Copy link
Member

amshinde commented Nov 4, 2020

I dont see how making volumes as read-only could impact the metrics. I'll rerun the the metrics CI to double check.

@amshinde amshinde merged commit 99a372e into kata-containers:master Nov 4, 2020
25 of 28 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no-backport-needed Changed do not need to be applied to an older branch / repository no-forward-port-needed Changed do not need to be applied to a newer branch / repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants