Skip to content

graphdriver/fuse-overlayfs: Remove redundant dir chown#53134

Merged
thaJeztah merged 1 commit into
moby:masterfrom
vvoland:gd-fuse-norechown
Jul 23, 2026
Merged

graphdriver/fuse-overlayfs: Remove redundant dir chown#53134
thaJeztah merged 1 commit into
moby:masterfrom
vvoland:gd-fuse-norechown

Conversation

@vvoland

@vvoland vvoland commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Init already creates the driver home with the required ownership. Avoid calling MkdirAllAndChown on it again during every layer creation.

Summary

Release notes (optional)

A picture of a cute animal (not mandatory but encouraged)

Init already creates the driver home with the required ownership.
Avoid calling MkdirAllAndChown on it again during every layer creation.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@vvoland vvoland self-assigned this Jul 22, 2026
Copilot AI review requested due to automatic review settings July 22, 2026 14:10
@vvoland vvoland added this to the 29.7.0 milestone Jul 22, 2026
@github-actions github-actions Bot added the area/daemon Core Engine label Jul 22, 2026
@vvoland vvoland added kind/bugfix PR's that fix bugs area/daemon Core Engine and removed area/daemon Core Engine labels Jul 22, 2026
@vvoland vvoland added the area/storage Image Storage label Jul 22, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR removes a per-layer MkdirAllAndChown(path.Dir(dir), ...) call in the fuse-overlayfs graphdriver’s create() path, relying on Init() to have already created and chowned the driver home directory. The intent is to avoid repeated ownership work on every layer creation.

Changes:

  • Remove redundant MkdirAllAndChown() of the driver home directory from (*Driver).create().
  • Keep per-layer directory creation (MkdirAndChown(dir, ...)) unchanged.

Comment on lines 172 to 176
dir := d.dir(id)
uid, gid := d.idMap.RootPair()

if err := user.MkdirAllAndChown(path.Dir(dir), 0o710, uid, gid); err != nil {
return err
}
if err := user.MkdirAndChown(dir, 0o710, uid, gid); err != nil {
return err

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The path is created in the driver's init function

func Init(home string, options []string, idMap user.IdentityMapping) (graphdriver.Driver, error) {
if _, err := exec.LookPath(binary); err != nil {
logger.Error(err)
return nil, graphdriver.ErrNotSupported
}
if !kernel.CheckKernelVersion(4, 18, 0) {
return nil, graphdriver.ErrNotSupported
}
cuid := os.Getuid()
_, gid := idMap.RootPair()
if err := user.MkdirAllAndChown(home, 0o710, cuid, gid); err != nil {
return nil, err
}
if err := user.MkdirAllAndChown(path.Join(home, linkDir), 0o700, cuid, os.Getegid()); err != nil {
return nil, err
}

Which is registered as the init function for the driver, so executed at startup;

graphdriver.Register(driverName, Init)

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
thaJeztah merged commit c77c96e into moby:master Jul 23, 2026
257 of 264 checks passed
@thaJeztah

Copy link
Copy Markdown
Member

Had another look at this, and probably needs a follow-up; looks like the same is the case for overlay2 (maybe others)

func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts) (retErr error) {
dir := d.dir(id)
cuid := os.Getuid()
uid, gid := d.idMap.RootPair()
if err := user.MkdirAllAndChown(path.Dir(dir), 0o710, cuid, gid); err != nil {
return err
}
if err := user.MkdirAndChown(dir, 0o710, cuid, gid); err != nil {
return err
}

Some notes there;

Looks like this MkdirAllAndChown for the drivers' home because redundant after 03f1c3d

That was a security fix, to make sure the driver's home had the right permissions, and that added the MkdirAllAndChown in the Init function, but it left the one in Create;

03f1c3d#diff-ed0b7987eccdde1bd70dc1ca9191f0ac2601e8f187024db5e3f968d755b9e47c

From the above, it looks to be intentional to overwrite existing mode for the directory BUT it may be too wide; MkdirAllAndChown is called without OnlyNew

// WithOnlyNew is an option for MkdirAllAndChown that will only change ownership and permissions
// on newly created directories. If the directory already exists, it will not be modified
func WithOnlyNew(o *mkdirOptions) {

MkdirAllAndChown calls mkDirAs with mkAll =true

return mkdirAs(path, mode, uid, gid, true, options.onlyNew)

Which means that any missing parent path would also be chown-ed and chmod-ed (the comment looks wrong because existing paths are handled earlier);

// even if it existed, we will chown the requested path + any subpaths that
// didn't exist when we called MkdirAll
for _, pathComponent := range paths {
if err = setPermissions(pathComponent, mode, uid, gid, nil); err != nil {
return err
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants