DAOS-18262 common: make sure DAOS file permissions are always 0XXY by janekmi · Pull Request #17680 · daos-stack/daos

janekmi · 2026-03-10T20:32:57Z

At least:

0660 for files
0770 for directories.

So daos_server group can access DAOS files.

Guide MD-on-PMEM

daos/                           src/control/server/storage/mount/provider.go:23
├── 8ac60360-eae4-4071-86ac-a83a42dc22a3        src/mgmt/srv_target.c:699
│   ├── rdb-pool                No change
│   └── vos-X                   src/vos/vos_pool.c:1001
├── control_raft                src/control/system/raft/raft_recovery.go:93
│   ├── daos_system.db          src/control/system/raft/raft.go:269
│   └── snapshots               No change
├── daos_sys                    src/vos/sys_db.c:131
│   └── sys_db                  No change
├── NEWBORNS                    src/mgmt/srv_target.c:452
├── superblock                  src/control/common/file_utils.go:141,src/control/server/instance_superblock.go:208
└── ZOMBIES                     src/mgmt/srv_target.c:460

Guide MD-on-SSD

tree daos/
daos/                           src/control/server/storage/mount/provider.go:23
├── b4defdf5-a92f-45fe-9be0-b96122efa42d        src/mgmt/srv_target.c:699
│   ├── rdb-pool                No change
│   └── vos-X                   src/mgmt/mgmt_common.c:211
├── NEWBORNS                    src/mgmt/mgmt_common.c:134
└── ZOMBIES                     src/mgmt/mgmt_common.c:134

tree config/daos_control/
config/daos_control/            src/control/server/storage/metadata/provider.go:207
├── control_raft                src/control/system/raft/raft_recovery.go:93
│   ├── daos_system.db          src/control/system/raft/raft.go:269
│   └── snapshots               No change
│       └── 2-39-1773083440395
│           ├── meta.json
│           └── state.bin
└── engine0                     src/control/server/storage/metadata/provider.go:233
    ├── daos_nvme.conf          src/control/server/storage/bdev/backend_class.go:106
    ├── daos_sys                src/vos/sys_db.c:131
    │   └── sys_db              No change
    └── superblock              src/control/common/file_utils.go:141,src/control/server/instance_superblock.go:208

Steps for the author:

Commit message follows the guidelines.
Appropriate Features or Test-tag pragmas were used.
Appropriate Functional Test Stages were run.
At least two positive code reviews including at least one code owner from each category referenced in the PR.
Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

Gatekeeper requested (daos-gatekeeper added as a reviewer).

At least: - 0660 for files - 0770 for directories. So daos_server group can access DAOS files. Signed-off-by: Jan Michalski <jan-marian.michalski@hpe.com>

github-actions · 2026-03-10T20:42:52Z

Ticket title is 'dlck command seems to require sudo privileges. Also hit the DER_NOMEM (-1009) issue.'
Status is 'Awaiting Verification'
Labels: 'test_2.8'
https://daosio.atlassian.net/browse/DAOS-18262

src/rdb/rdb.c

liw · 2026-03-11T00:11:20Z

src/rdb/rdb.c

 	    path, (unsigned char *)uuid, params->rcp_size, 0 /* data_sz */, 0 /* meta_sz */,
 	    VOS_POF_SMALL | VOS_POF_EXCL | VOS_POF_RDB | VOS_POF_EXTERNAL_CHKPT,
 	    params->rcp_vos_df_version, &pool);
+	(void)umask(stored_mask);


Would it be safer and more appropriate to do this in dss_vos_pool_create (actually why not in server_init or daos_engine's main) instead? Is rdb special in some way?

The thing is you can put them in many different places and end up with the intended result. RDB turned out to be special because of the position I put other umask() calls so one rdb_create() call had been left unattended. But no more. I move this call a little higher up the stack so now it covers all necessary contexts. No special case for RDB. I hope you like this way. 🙂

Thank you for responding. I'd think either one approaches the file permission problem from outside of daos_engine or even daos_server (for instance, is there any systemd setting?), or one does it in main once. It's especially concerning if one changes umask to a more permissive value (e.g., 0). That said, I've not investigated into umasks deeply myself.

Since umask is per process, not per thread, changing and restoring it in target threads brings up potential questions on races among the threads---one might not want to get into such a situation.

Since umask is per process, not per thread, changing and restoring it in target threads brings up potential questions on races among the threads---one might not want to get into such a situation.

It is a really good point. I guess there are two possible ways out of it:

Set umask(002) (instead of the default 022) first thing in main() and just keep it like this. This way you get the permissions you asked every time you create a file or directory.

Always follow mkdir() and open(O_CREAT) with chmod(). So eventually you get you asked for.

I would prefer 1. Just make sure there are no 0777 used anywhere in the code. 😅

What do you think?

Thank you @grom72 for the suggestion!

grom72 · 2026-03-11T13:03:25Z

src/mgmt/srv_target.c

 			goto out;

-		rc = mkdir(tca->tca_newborn, 0700);
+		rc = mkdir(tca->tca_newborn, 0770);


Suggested change

rc = mkdir(tca->tca_newborn, 0770);

rc = mkdir(tca->tca_newborn, S_IRWXU | S_IRWXG);

src/control/server/storage/bdev/backend_class.go

grom72 · 2026-03-11T13:06:38Z

src/control/server/storage/bdev/backend_class.go

 const (
 	aioBlockSize       = humanize.KiByte * 4 // device block size hardcoded to 4096 bytes
-	defaultAioFileMode = 0600                // AIO file permissions set to owner +rw
+	defaultAioFileMode = 0660                // AIO file permissions set to owner +rw


Suggested change

defaultAioFileMode = 0660 // AIO file permissions set to owner +rw

defaultAioFileMode = 0660 // AIO file permissions set to owner and group +rw

I removed this part of the comment since the definition has been moved to a different place. Please see DefaultFilePerm.

src/control/system/raft/raft.go

grom72 · 2026-03-11T13:12:30Z

src/mgmt/mgmt_common.c

 		goto out;
 	}
-	rc = mkdir(pool_newborns_path, 0700);
+	rc = mkdir(pool_newborns_path, 0770);


Suggested change

rc = mkdir(pool_newborns_path, 0770);

rc = mkdir(pool_newborns_path, S_IRWXU | S_IRWXG);

grom72 · 2026-03-11T13:13:08Z

src/control/system/raft/raft_recovery.go


 func createRaftDir(dbPath string) error {
-	if err := os.Mkdir(dbPath, 0700); err != nil && !os.IsExist(err) {
+	var perm os.FileMode = 0770


Please be consistent: perms := os.FileMode(0775) was used above

Suggested change

var perm os.FileMode = 0770

perms := os.FileMode(0770)

Refactored. Please see Mkdir2().

src/control/server/storage/mount/provider.go

Remove from tgt_create_preallocate() and rdb_create(). Add one in ds_mgmt_create_pool(). Signed-off-by: Jan Michalski <jan-marian.michalski@hpe.com>

daosbuild3 · 2026-03-12T09:48:00Z

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-17680/2/testReport/

tanabarr

LGTM

tanabarr · 2026-03-12T11:03:05Z

src/control/server/storage/metadata/provider.go

 	}

-	if err := p.sys.Mkdir(req.DataPath, 0755); err != nil {
+	if err := p.sys.Mkdir(req.DataPath, perms); err != nil {


Given we repeat this sequence it would be nice to have a helper that performs Mkdir+Chmod to avoid the specified problem where the permissions are reduced by the umask.

Agreed with Tom. Such a function would belong in src/control/common/file_utils.go.

Done. Thank you guys for the idea and suggestions. Very much appreciated. 👍

grom72 · 2026-03-12T10:23:36Z

src/vos/sys_db.c

 static int
 db_open_create(struct sys_db *db, bool try_create)
 {
+	const mode_t       db_dir_mode = 0770;


Suggested change

const mode_t db_dir_mode = 0770;

const mode_t db_dir_mode = S_IRWXU | S_IRWXG;

kjacque · 2026-03-12T22:32:27Z

src/control/server/storage/metadata/provider.go

 }

 func (p *Provider) setupDataDir(req storage.MetadataFormatRequest) error {
+	perms := os.FileMode(0775)


Would it make sense to have a constant somewhere indicating these new default perms? Same question applies at the engine level.

If we define the permission values in a C header file for the engines, we can import it in our Go package lib/daos.

I did not go all the way. We still use a variety of values e.g. there are directories 0770 and 0775. And I do not want to change it in this PR because I do not want to remove any permissions and I do want not grant more than it was agreed to be granted.

For now I created two constants in the Go code: DefaultFilePerm and DefaultDirPerm and I used them everywhere where the permissions were either 0770 or 0660. I left other cases unchanged.

We can discuss next steps in a separate ticket. I think the way is open assuming no opposition will come up. Please note we would have to possibly remove some permissions people were used to have. It is not obvious it would be a desirable change to have one set of permissions for all files and all directories across the project.

kjacque · 2026-03-12T22:33:27Z

src/control/server/storage/metadata/provider.go

 	}

-	if err := p.sys.Mkdir(req.DataPath, 0755); err != nil {
+	if err := p.sys.Mkdir(req.DataPath, perms); err != nil {


Agreed with Tom. Such a function would belong in src/control/common/file_utils.go.

kjacque · 2026-03-12T22:36:15Z

src/control/system/raft/raft.go


+	// Boltdb file permissions on create are set to 0600.
+	// The os.Chmod ensures the final permissions for both the user and their group are the same.
+	err = os.Chmod(dbCfg.DBFilePath(), 0660)


Given how many places this needs to be added, I'd suggest adding SetDefaultFilePerm and SetDefaultDirPerm functions to keep the specifics of these defaults all in one place.

I went a slightly different route. Please see Mkdir2(). Waiting for you thought. 🙂

Signed-off-by: Jan Michalski <jan-marian.michalski@hpe.com>

Add two constants and apply where possible: - DefaultFilePerm and - DefaultDirPerm Define Mkdir2() as Mkdir() + Chmod(). Signed-off-by: Jan Michalski <jan-marian.michalski@hpe.com>

DAOS-18262 common: make sure DAOS file permissions are always 0XXY

8eece70

At least: - 0660 for files - 0770 for directories. So daos_server group can access DAOS files. Signed-off-by: Jan Michalski <jan-marian.michalski@hpe.com>

janekmi requested review from a team as code owners March 10, 2026 20:32

janekmi requested review from rpadma2 and tanabarr March 10, 2026 21:23

liw reviewed Mar 11, 2026

View reviewed changes

janekmi requested a review from grom72 March 11, 2026 09:42

grom72 requested changes Mar 11, 2026

View reviewed changes

DAOS-18262 common: improve umask() calls positions

b72f0bc

Remove from tgt_create_preallocate() and rdb_create(). Add one in ds_mgmt_create_pool(). Signed-off-by: Jan Michalski <jan-marian.michalski@hpe.com>

tanabarr previously approved these changes Mar 12, 2026

View reviewed changes

grom72 reviewed Mar 12, 2026

View reviewed changes

kjacque requested changes Mar 12, 2026

View reviewed changes

janekmi added 2 commits March 16, 2026 19:56

DAOS-18262 common: 0770 -> S_IRWXU | S_IRWXG (C only)

2b76e46

Signed-off-by: Jan Michalski <jan-marian.michalski@hpe.com>

DAOS-18262 common: introduce Default*Perm and Mkdir2

1d6435b

Add two constants and apply where possible: - DefaultFilePerm and - DefaultDirPerm Define Mkdir2() as Mkdir() + Chmod(). Signed-off-by: Jan Michalski <jan-marian.michalski@hpe.com>

janekmi dismissed tanabarr’s stale review via 1d6435b March 16, 2026 20:00

janekmi requested review from grom72, kjacque and tanabarr March 16, 2026 23:30

	rc = mkdir(tca->tca_newborn, 0770);
	rc = mkdir(tca->tca_newborn, S_IRWXU \| S_IRWXG);

	defaultAioFileMode = 0660 // AIO file permissions set to owner +rw
	defaultAioFileMode = 0660 // AIO file permissions set to owner and group +rw

	rc = mkdir(pool_newborns_path, 0770);
	rc = mkdir(pool_newborns_path, S_IRWXU \| S_IRWXG);

	const mode_t db_dir_mode = 0770;
	const mode_t db_dir_mode = S_IRWXU \| S_IRWXG;

Conversation

janekmi commented Mar 10, 2026

Guide MD-on-PMEM

Guide MD-on-SSD

Steps for the author:

After all prior steps are complete:

Uh oh!

github-actions bot commented Mar 10, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liw Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janekmi Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

daosbuild3 commented Mar 12, 2026

Uh oh!

tanabarr 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!

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!

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

liw Mar 12, 2026 •

edited

Loading

janekmi Mar 16, 2026 •

edited

Loading