I'm still trying to wrap my head around what this part involves exactly. Thisis a pretty big change in semantics.Could this be moved to the end of the patchset, or is this a fundamental part of the btrfs fscrypt support that the rest of your patchset depends on? I'd think it would be a lot easier to review if this change was an add-on at the end.
Definitely.
One thing to keep in mind is that FS_IOC_SET_ENCRYPTION_POLICY failing on nonempty directories can actually be very useful, since it makes it possible to detect bugs where people create files in encrypted directories (expecting that they are encrypted) before an encryption policy actually gets assigned. Since FS_IOC_SET_ENCRYPTION_POLICY fails in that case, such bugs can be detected andfixed.
I agree that this has risks of inadvertent misuse in that fashion.The usecase I'm oriented towards is: someone builds an unencrypted subvolume with a container base filesystem, takes several snapshots of the subvolume, starts a container on each subvolume, and has each container encrypt its designated subvolume going forward with a different key. This usecase needs some way to mark a subvolume/directory already containing files as encrypted going forward; I've had a hard time coming up with a way to both protect users against such accidental misuse, but also allow this container usecase.
It might be warranted to use an encryption policy flag to explicitly indicatethat mixing encrypted and unencrypted files is being allowed.
Could it be sufficient to check either empty or read-only, something like (is_empty_dir(inode) || (FS_CFLG_PARTIAL && !inode_permission(..., inode, MAY_WRITE)))? Then the user is unable to accidentally write unencrypted data, since they've taken an action to make the directory read-only, until they've set a policy and key and turned the directory read-write again.
Thanks!
|