Skip to content

Commit

Permalink
libct/configs/validate: rootlessEUIDMount: speedup
Browse files Browse the repository at this point in the history
1. Fix function docs. In particular, remove the part
   which is not true ("verifies that the user isn't trying to set up any
   mounts they don't have the rights to do"), and fix the part that
   says "that doesn't resolve to root" (which is no longer true since
   commit d8b6694).

2. Replace fmt.Sscanf (which is slow and does lots of allocations)
   with strings.TrimPrefix and strconv.Atoi.

3. Add a benchmark for rootlessEUIDMount. Comparing the old and the new
   implementations:

	name                 old time/op    new time/op    delta
	RootlessEUIDMount-4    1.01µs ± 2%    0.16µs ± 1%  -84.15%  (p=0.008 n=5+5)

	name                 old alloc/op   new alloc/op   delta
	RootlessEUIDMount-4      224B ± 0%       80B ± 0%  -64.29%  (p=0.008 n=5+5)

	name                 old allocs/op  new allocs/op  delta
	RootlessEUIDMount-4      7.00 ± 0%      1.00 ± 0%  -85.71%  (p=0.008 n=5+5)

Note this code is already tested (in rootless_test.go).

Fixes: d8b6694
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
  • Loading branch information
kolyshkin committed Mar 17, 2022
1 parent 51e607f commit 48006d0
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 12 deletions.
21 changes: 9 additions & 12 deletions libcontainer/configs/validate/rootless.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package validate

import (
"errors"
"fmt"
"strconv"
"strings"

"github.com/opencontainers/runc/libcontainer/configs"
Expand Down Expand Up @@ -51,9 +51,8 @@ func rootlessEUIDMappings(config *configs.Config) error {
return nil
}

// mount verifies that the user isn't trying to set up any mounts they don't have
// the rights to do. In addition, it makes sure that no mount has a `uid=` or
// `gid=` option that doesn't resolve to root.
// rootlessEUIDMount verifies that all mounts have valid uid=/gid= options,
// i.e. their arguments has proper ID mappings.
func rootlessEUIDMount(config *configs.Config) error {
// XXX: We could whitelist allowed devices at this point, but I'm not
// convinced that's a good idea. The kernel is the best arbiter of
Expand All @@ -63,10 +62,9 @@ func rootlessEUIDMount(config *configs.Config) error {
// Check that the options list doesn't contain any uid= or gid= entries
// that don't resolve to root.
for _, opt := range strings.Split(mount.Data, ",") {
if strings.HasPrefix(opt, "uid=") {
var uid int
n, err := fmt.Sscanf(opt, "uid=%d", &uid)
if n != 1 || err != nil {
if str := strings.TrimPrefix(opt, "uid="); len(str) < len(opt) {
uid, err := strconv.Atoi(str)
if err != nil {
// Ignore unknown mount options.
continue
}
Expand All @@ -75,10 +73,9 @@ func rootlessEUIDMount(config *configs.Config) error {
}
}

if strings.HasPrefix(opt, "gid=") {
var gid int
n, err := fmt.Sscanf(opt, "gid=%d", &gid)
if n != 1 || err != nil {
if str := strings.TrimPrefix(opt, "gid="); len(str) < len(opt) {
gid, err := strconv.Atoi(str)
if err != nil {
// Ignore unknown mount options.
continue
}
Expand Down
21 changes: 21 additions & 0 deletions libcontainer/configs/validate/rootless_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,3 +141,24 @@ func TestValidateRootlessEUIDMountGid(t *testing.T) {
t.Errorf("Expected error to occur when setting gid=11 in mount options and GidMapping[0].size is 10")
}
}

func BenchmarkRootlessEUIDMount(b *testing.B) {
config := rootlessEUIDConfig()
config.GidMappings[0].Size = 10
config.Mounts = []*configs.Mount{
{
Source: "devpts",
Destination: "/dev/pts",
Device: "devpts",
Data: "newinstance,ptmxmode=0666,mode=0620,uid=0,gid=5",
},
}

b.ResetTimer()
for i := 0; i < b.N; i++ {
err := rootlessEUIDMount(config)
if err != nil {
b.Fatal(err)
}
}
}

0 comments on commit 48006d0

Please sign in to comment.