On Fri, 2021-04-30 at 18:03 -0700, Patrick Donnelly wrote: > On Fri, Apr 30, 2021 at 8:04 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > On Fri, 2021-04-30 at 07:45 -0700, Patrick Donnelly wrote: > > > On Fri, Apr 30, 2021 at 7:33 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > We specifically need this for directories and symlinks during pathwalks > > > > too. Eventually we may also want to encrypt certain data for other inode > > > > types as well (e.g. block/char devices). That's less critical though. > > > > > > > > The problem with fetching it after the inode is first instantiated is > > > > that we can end up recursing into a separate request while encoding a > > > > path. For instance, see this stack trace that Luis reported: > > > > https://lore.kernel.org/ceph-devel/53d5bebb28c1e0cd354a336a56bf103d5e3a6344.camel@xxxxxxxxxx/T/#m0f7bbed6280623d761b8b4e70671ed568535d7fa > > > > > > > > While that implementation stored the context in an xattr, the problem > > > > isstill the same if you have to fetch the context in the middle of > > > > building a path. The best solution is just to always ensure it's > > > > available. > > > > > > Got it. Splitting the struct makes sense then. The pin cap would be > > > suitable for the immutable encryption context (if truly > > > immutable?).Otherwise maybe the Axs cap? > > > > > > > Ok. In that case, then we probably need to put the context blob under > > AUTH caps so we can ensure that it's consulted during the permission > > checks for pathwalks. The size will need to live under FILE. > > > > Now for the hard part...what do we name these fields? > > > > fscrypt_context > > fscrypt_size > > > > ...or maybe... > > > > fscrypt_auth > > fscrypt_file > > > > Since they'll be vector blobs, we can version these too so that we can > > add other fields later if the need arises (even for non-fscrypt stuff). > > Maybe we could consider: > > > > client_opaque_auth > > client_opaque_file > > An opaque blob makes sense but you'd want a sentinel indicating it's > an fscrypt blob. Don't think we'd be able to have two competing > use-cases but it'd be nice to have it generic enough for future > encryption libraries maybe. > I'm going with fscrypt_auth and fscrypt_file for now. We can rename them later though if we want. What I'll probably do is just declare a versioned format for these blobs. The MDS won't care about it, but the clients can follow that convention. I've made a bit of progress on this this week (fixing up the encoding and decoding was a bit of a hassle, fwiw). These fields are associated with the core inodes. The clients will use SETATTR calls to set them, though they will also be updated with cap flushes, etc. I need to be able to validate this feature in userland though and I don't really want to roll dedicated functions for them. What I may do is add new vxattrs (ceph.fscrypt_auth and ceph.fscrypt_file) and have those expose these fields. Doing a setxattr on them will do a SETATTR under the hood. The alternative is to declare new libcephfs routines for fetching and setting these. I'm not terribly crazy about either, but I have a slight preference for the vxattr since it's something we could replicate in the kernel for debugging purposes. Thoughts? -- Jeff Layton <jlayton@xxxxxxxxxx>
