projectquota: protect concurrent map access (ENGCORE-920) by kolyshkin · Pull Request #39644 · moby/moby

kolyshkin · 2019-07-31T18:06:40Z

The bug was always there, hidden by the global lock (removed in #39135, which opened a can of worms fixed in a few followup PRs, such as #39209)

kolyshkin · 2019-07-31T18:17:23Z

@amir73il PTAL

cpuguy83 · 2019-07-31T18:20:55Z

daemon/graphdriver/quota/projectquota.go

Does this need a lock too?

Looks like it... the code was written without concurrency in mind.

maybe we could use atomics for this one

yeah, it's easier to reuse the same lock

cpuguy83 · 2019-07-31T18:24:36Z

Seems like there's some other potential for data races around project ID's.
I also wonder if we need some locking around the syscalls at least per ID or target path?

thaJeztah · 2019-07-31T19:13:21Z

Test is failing;

18:23:59 fatal error: sync: Unlock of unlocked RWMutex

thaJeztah · 2019-07-31T19:14:22Z

daemon/graphdriver/quota/types.go

can you move this above quotas ? I think the convention is to put the mutex above the fields that it is protecting

thaJeztah · 2019-07-31T19:15:23Z

daemon/graphdriver/quota/projectquota.go

should nextProjectID also be inside the lock?

oh, never mind, I see brian's comment above

kolyshkin · 2019-07-31T21:07:52Z

Seems like there's some other potential for data races around project ID's.

@cpuguy83 I think I have handled it (now in the latest version)

I also wonder if we need some locking around the syscalls at least per ID or target path?

Are you assuming it might be unsafe? I took a look at quotactl(2) man page and can't find anything that hints it might be the case. Perhaps @amir73il can chime in?

thaJeztah

SGTM

cpuguy83 · 2019-08-01T03:51:22Z

daemon/graphdriver/quota/types.go

I'm surprised this passed the gofmt check.
Can we move add a whitespace line above import?

Sure.

I am surprised as well -- it was auto-inserted by goimports which is a superset of gofmt -- maybe it's a corner case which they don't handle, I'll take a look.

~~golang/go#33413~~ -> golang/go#22631

Protect access to q.quotas map, and lock around changing nextProjectID. Techinically, the lock in findNextProjectID() is not needed as it is only called during initialization, but one can never be too careful. Fixes: 52897d1 ("projectquota: utility class for project quota controls") Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

andrewhsu · 2019-08-01T18:24:06Z

I see an old test here related to --storage-opt:

moby/integration-cli/docker_cli_create_test.go

Lines 60 to 72 in 6345208

    
           // Make sure we can grow the container's rootfs at creation time. 
        
           func (s *DockerSuite) TestCreateGrowRootfs(c *check.C) { 
        
           	// Windows and Devicemapper support growing the rootfs 
        
           	if testEnv.OSType != "windows" { 
        
           		testRequires(c, Devicemapper) 
        
           	} 
        
           	out, _ := dockerCmd(c, "create", "--storage-opt", "size=120G", "busybox") 
        
           	cleanedContainerID := strings.TrimSpace(out) 
        
           	inspectOut := inspectField(c, cleanedContainerID, "HostConfig.StorageOpt") 
        
           	c.Assert(inspectOut, checker.Equals, "map[size:120G]") 
        
           }

But test is only run on devicemapper. Perhaps we should change it to allow it to run on overlay2 with XFS?

cpuguy83

LGTM

kolyshkin · 2019-08-02T00:45:26Z

Some followups

Tried to repro this, and apparently projectquota is not working on latest CentOS 7, due to EACCES from the kernel -- apparently it does not like we're using a self-made device. I did some experiments and here are the gist of results:

[root@kir-ce75-gd quota]# strace -e quotactl ./quota.test -test.run M
quotactl(Q_XGETQSTAT|PRJQUOTA, "/dev/mapper/docker-lvdocker", 0, {version=1, ...}) = 0
quotactl(Q_XGETQSTAT|PRJQUOTA, "/var/lib/docker/overlay2/backingFsBlockDev", 0, 0xc000054ee8) = -1 EACCES (Permission denied)
PASS
+++ exited with 0 +++
[root@kir-ce75-gd quota]# ls -l /var/lib/docker/overlay2/backingFsBlockDev
brw-------. 1 root root 253, 0 Aug  1 20:13 /var/lib/docker/overlay2/backingFsBlockDev
[root@kir-ce75-gd quota]# ls -l /dev/mapper/docker-lvdocker
lrwxrwxrwx. 1 root root 7 Aug  1 20:03 /dev/mapper/docker-lvdocker -> ../dm-0
[root@kir-ce75-gd quota]# ls -l /dev/dm-0 
brw-rw----. 1 root disk 253, 0 Aug  1 20:03 /dev/dm-0

So, /var/lib/docker/overlay2/backingFsBlockDev is identical to /dev/dm-0 but the kernel returns an error for the former and success for the latter.

It's all working just fine with Debian 9, though it's not reproducible there since it's a UP machine.

Reproduced on a SLES 15 system with 6x Xeon and XFS with pquota, using the following config:

# cat /etc/docker/daemon.json 
{
	"debug": true,
	"storage-driver": "overlay2",
	"storage-opts": ["overlay2.size=1G"],
	"experimental": true
}

and a stress-testing script from #39135 (took two runs with default values).

Compiled a fixed version; so far no bug. Left it to run overnight to fully confirm.

thaJeztah · 2019-08-02T07:43:27Z

@kolyshkin can you create a tracking issue for project quota not working on CentOS 7 ?

kolyshkin · 2019-08-02T16:24:08Z

Ran 140 iterations of the following script overnight (created/removed 1400000 containers total) using dockerd compiled from moby/moby master on SLES-15 system, no issues.

#!/bin/bash

NUMCTS=10000
PARALLEL=100

create() {
	# -v 'vol_{}:/vol'
	time ( seq $NUMCTS | parallel -j$PARALLEL docker create alpine top > /dev/null )
	echo "^^^ CREATE TIME ^^^"
}

remove() {
	time ( docker ps -qa | shuf | tail -n $NUMCTS | parallel -j$PARALLEL docker rm -f '{}' > /dev/null )
	echo "^^^ REMOVE TIME ^^^"
}

# precreate $NUMCTS containers
create
echo

# create another $NUMCTS while removing $NUMCTS containers
create &
remove &
wait

# remove the rest of it
remove

GordonTheTurtle added the status/0-triage label Jul 31, 2019

kolyshkin requested review from thaJeztah and tonistiigi and removed request for tonistiigi July 31, 2019 18:06

kolyshkin force-pushed the quota-map branch from b845a74 to 5c260fa Compare July 31, 2019 18:15

cpuguy83 reviewed Jul 31, 2019

View reviewed changes

thaJeztah added area/storage/overlay process/cherry-pick status/2-code-review and removed status/0-triage labels Jul 31, 2019

thaJeztah requested changes Jul 31, 2019

View reviewed changes

thaJeztah reviewed Jul 31, 2019

View reviewed changes

kolyshkin force-pushed the quota-map branch 4 times, most recently from b6421df to 4fbe160 Compare July 31, 2019 21:01

thaJeztah approved these changes Jul 31, 2019

View reviewed changes

cpuguy83 reviewed Aug 1, 2019

View reviewed changes

kolyshkin force-pushed the quota-map branch from 4fbe160 to 1ac0a66 Compare August 1, 2019 16:59

cpuguy83 approved these changes Aug 1, 2019

View reviewed changes

cpuguy83 merged commit 41040b7 into moby:master Aug 1, 2019

kolyshkin changed the title ~~projectquota: protect concurrent map access~~ projectquota: protect concurrent map access (ENGCORE-920) Aug 1, 2019

This was referenced Aug 1, 2019

[19.03] backport projectquota: protect concurrent map access (ENGCORE-920) docker-archive/engine#310

Merged

[18.09 backport] projectquota: protect concurrent map access (ENGCORE-920) docker-archive/engine#311

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Oct 8, 2019

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

Conversation

kolyshkin commented Jul 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Jul 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jul 31, 2019

Uh oh!

thaJeztah commented Jul 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Jul 31, 2019

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin Aug 1, 2019 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewhsu commented Aug 1, 2019

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Aug 2, 2019

Uh oh!

thaJeztah commented Aug 2, 2019

Uh oh!

kolyshkin commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kolyshkin commented Jul 31, 2019 •

edited

Loading

kolyshkin Aug 1, 2019 •

edited by thaJeztah

Loading