* [PATCH 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories @ 2023-03-13 12:33 Luís Henriques 2023-03-13 12:33 ` [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() Luís Henriques ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Luís Henriques @ 2023-03-13 12:33 UTC (permalink / raw) To: Eric Biggers, Xiubo Li, Jeff Layton Cc: Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel, Luís Henriques Hi! I started seeing fstest generic/123 failing in ceph fscrypt, when running it with 'test_dummy_encryption'. This test is quite simple: 1. Creates a directory with write permissions for root only 2. Writes into a file in that directory 3. Uses 'su' to try to modify that file as a different user, and gets -EPERM All the test steps succeed, but the test fails to cleanup: 'rm -rf <dir>' will fail with -ENOTEMPTY. 'strace' shows that calling unlinkat() to remove the file got a -ENOENT and then -ENOTEMPTY for the directory. This is because 'su' does a drop_caches ('su (874): drop_caches: 2' in dmesg), and ceph's atomic open will do: if (IS_ENCRYPTED(dir)) { set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags); if (!fscrypt_has_encryption_key(dir)) { spin_lock(&dentry->d_lock); dentry->d_flags |= DCACHE_NOKEY_NAME; spin_unlock(&dentry->d_lock); } } Although 'dir' has the encryption key available, fscrypt_has_encryption_key() will return 'false' because fscrypt info isn't yet set after the cache cleanup. The first patch will add a new helper for the atomic_open that will force the fscrypt info to be loaded into an inode that has been evicted recently but for which the key is still available. The second patch switches ceph atomic_open to use the new fscrypt helper. Cheers, -- Luís Changes since initial RFC (after Eric's review): - Added kerneldoc comments to the new fscrypt helper - Dropped '__' from helper name (now fscrypt_prepare_atomic_open()) - Added IS_ENCRYPTED() check in helper - DCACHE_NOKEY_NAME is not set if fscrypt_get_encryption_info() returns an error - Fixed helper for !CONFIG_FS_ENCRYPTION (now defined 'static inline') Luís Henriques (2): fscrypt: new helper function - fscrypt_prepare_atomic_open() ceph: switch atomic open to use new fscrypt helper fs/ceph/file.c | 8 +++----- fs/crypto/hooks.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/fscrypt.h | 7 +++++++ 3 files changed, 45 insertions(+), 5 deletions(-) ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() 2023-03-13 12:33 [PATCH 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories Luís Henriques @ 2023-03-13 12:33 ` Luís Henriques 2023-03-13 18:09 ` Eric Biggers 2023-03-13 12:33 ` [PATCH 2/2] ceph: switch atomic open to use new fscrypt helper Luís Henriques 2023-03-13 17:11 ` [PATCH 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories Jeff Layton 2 siblings, 1 reply; 18+ messages in thread From: Luís Henriques @ 2023-03-13 12:33 UTC (permalink / raw) To: Eric Biggers, Xiubo Li, Jeff Layton Cc: Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel, Luís Henriques This patch introduces a new helper function which prepares an atomic_open. Because atomic open can act as a lookup if handed a dentry that is negative, we need to set DCACHE_NOKEY_NAME if the key for the parent isn't available. The reason for getting the encryption info before checking if the directory has the encryption key is because we may have the key available but the encryption info isn't yet set (maybe due to a drop_caches). The regular open path will use fscrypt_file_open for that but in the atomic open a different approach is required. Signed-off-by: Luís Henriques <lhenriques@suse.de> --- fs/crypto/hooks.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/fscrypt.h | 7 +++++++ 2 files changed, 42 insertions(+) diff --git a/fs/crypto/hooks.c b/fs/crypto/hooks.c index 7b8c5a1104b5..8be1e35984f1 100644 --- a/fs/crypto/hooks.c +++ b/fs/crypto/hooks.c @@ -117,6 +117,41 @@ int __fscrypt_prepare_readdir(struct inode *dir) } EXPORT_SYMBOL_GPL(__fscrypt_prepare_readdir); +/** + * fscrypt_prepare_atomic_open() - prepare an atomic open on an encrypted directory + * @dir: inode of parent directory + * @dentry: dentry being open + * + * Because atomic open can act as a lookup if handed a dentry that is negative, + * we need to set DCACHE_NOKEY_NAME if the key for the parent isn't available. + * + * The reason for getting the encryption info before checking if the directory + * has the encryption key is because the key may be available but the encryption + * info isn't yet set (maybe due to a drop_caches). The regular open path will + * use fscrypt_file_open for that, but in the atomic open a different approach + * is required. + * + * Return: 0 on success, or an error code if fscrypt_get_encryption_info() + * fails. + */ +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry) +{ + int err; + + if (!IS_ENCRYPTED(dir)) + return 0; + + err = fscrypt_get_encryption_info(dir, true); + if (!err && !fscrypt_has_encryption_key(dir)) { + spin_lock(&dentry->d_lock); + dentry->d_flags |= DCACHE_NOKEY_NAME; + spin_unlock(&dentry->d_lock); + } + + return err; +} +EXPORT_SYMBOL_GPL(fscrypt_prepare_atomic_open); + int __fscrypt_prepare_setattr(struct dentry *dentry, struct iattr *attr) { if (attr->ia_valid & ATTR_SIZE) diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h index 4f5f8a651213..c70acb2a737a 100644 --- a/include/linux/fscrypt.h +++ b/include/linux/fscrypt.h @@ -362,6 +362,7 @@ int __fscrypt_prepare_rename(struct inode *old_dir, struct dentry *old_dentry, int __fscrypt_prepare_lookup(struct inode *dir, struct dentry *dentry, struct fscrypt_name *fname); int __fscrypt_prepare_readdir(struct inode *dir); +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry); int __fscrypt_prepare_setattr(struct dentry *dentry, struct iattr *attr); int fscrypt_prepare_setflags(struct inode *inode, unsigned int oldflags, unsigned int flags); @@ -688,6 +689,12 @@ static inline int __fscrypt_prepare_readdir(struct inode *dir) return -EOPNOTSUPP; } +static inline int fscrypt_prepare_atomic_open(struct inode *dir, + struct dentry *dentry) +{ + return -EOPNOTSUPP; +} + static inline int __fscrypt_prepare_setattr(struct dentry *dentry, struct iattr *attr) { ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() 2023-03-13 12:33 ` [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() Luís Henriques @ 2023-03-13 18:09 ` Eric Biggers 2023-03-14 0:53 ` Xiubo Li 2023-03-14 10:15 ` Luís Henriques 0 siblings, 2 replies; 18+ messages in thread From: Eric Biggers @ 2023-03-13 18:09 UTC (permalink / raw) To: Luís Henriques Cc: Xiubo Li, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel On Mon, Mar 13, 2023 at 12:33:09PM +0000, Luís Henriques wrote: > + * The regular open path will use fscrypt_file_open for that, but in the > + * atomic open a different approach is required. This should actually be fscrypt_prepare_lookup, not fscrypt_file_open, right? > +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry) > +{ > + int err; > + > + if (!IS_ENCRYPTED(dir)) > + return 0; > + > + err = fscrypt_get_encryption_info(dir, true); > + if (!err && !fscrypt_has_encryption_key(dir)) { > + spin_lock(&dentry->d_lock); > + dentry->d_flags |= DCACHE_NOKEY_NAME; > + spin_unlock(&dentry->d_lock); > + } > + > + return err; > +} > +EXPORT_SYMBOL_GPL(fscrypt_prepare_atomic_open); [...] > +static inline int fscrypt_prepare_atomic_open(struct inode *dir, > + struct dentry *dentry) > +{ > + return -EOPNOTSUPP; > +} This has different behavior on unencrypted directories depending on whether CONFIG_FS_ENCRYPTION is enabled or not. That's bad. In patch 2, the caller you are introducing has already checked IS_ENCRYPTED(). Also, your kerneldoc comment for fscrypt_prepare_atomic_open() says it is for *encrypted* directories. So IMO, just remove the IS_ENCRYPTED() check from the CONFIG_FS_ENCRYPTION version of fscrypt_prepare_atomic_open(). - Eric ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() 2023-03-13 18:09 ` Eric Biggers @ 2023-03-14 0:53 ` Xiubo Li 2023-03-14 2:25 ` Eric Biggers 2023-03-14 10:15 ` Luís Henriques 1 sibling, 1 reply; 18+ messages in thread From: Xiubo Li @ 2023-03-14 0:53 UTC (permalink / raw) To: Eric Biggers, Luís Henriques Cc: Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel On 14/03/2023 02:09, Eric Biggers wrote: > On Mon, Mar 13, 2023 at 12:33:09PM +0000, Luís Henriques wrote: >> + * The regular open path will use fscrypt_file_open for that, but in the >> + * atomic open a different approach is required. > This should actually be fscrypt_prepare_lookup, not fscrypt_file_open, right? > >> +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry) >> +{ >> + int err; >> + >> + if (!IS_ENCRYPTED(dir)) >> + return 0; >> + >> + err = fscrypt_get_encryption_info(dir, true); >> + if (!err && !fscrypt_has_encryption_key(dir)) { >> + spin_lock(&dentry->d_lock); >> + dentry->d_flags |= DCACHE_NOKEY_NAME; >> + spin_unlock(&dentry->d_lock); >> + } >> + >> + return err; >> +} >> +EXPORT_SYMBOL_GPL(fscrypt_prepare_atomic_open); > [...] >> +static inline int fscrypt_prepare_atomic_open(struct inode *dir, >> + struct dentry *dentry) >> +{ >> + return -EOPNOTSUPP; >> +} > This has different behavior on unencrypted directories depending on whether > CONFIG_FS_ENCRYPTION is enabled or not. That's bad. > > In patch 2, the caller you are introducing has already checked IS_ENCRYPTED(). > > Also, your kerneldoc comment for fscrypt_prepare_atomic_open() says it is for > *encrypted* directories. > > So IMO, just remove the IS_ENCRYPTED() check from the CONFIG_FS_ENCRYPTION > version of fscrypt_prepare_atomic_open(). IMO we should keep this check in fscrypt_prepare_atomic_open() to make it consistent with the existing fscrypt_prepare_open(). And we can just remove the check from ceph instead. - Xiubo > - Eric > -- Best Regards, Xiubo Li (李秀波) Email: xiubli@redhat.com/xiubli@ibm.com Slack: @Xiubo Li ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() 2023-03-14 0:53 ` Xiubo Li @ 2023-03-14 2:25 ` Eric Biggers 2023-03-14 4:20 ` Xiubo Li 0 siblings, 1 reply; 18+ messages in thread From: Eric Biggers @ 2023-03-14 2:25 UTC (permalink / raw) To: Xiubo Li Cc: Luís Henriques, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel On Tue, Mar 14, 2023 at 08:53:51AM +0800, Xiubo Li wrote: > > On 14/03/2023 02:09, Eric Biggers wrote: > > On Mon, Mar 13, 2023 at 12:33:09PM +0000, Luís Henriques wrote: > > > + * The regular open path will use fscrypt_file_open for that, but in the > > > + * atomic open a different approach is required. > > This should actually be fscrypt_prepare_lookup, not fscrypt_file_open, right? > > > > > +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry) > > > +{ > > > + int err; > > > + > > > + if (!IS_ENCRYPTED(dir)) > > > + return 0; > > > + > > > + err = fscrypt_get_encryption_info(dir, true); > > > + if (!err && !fscrypt_has_encryption_key(dir)) { > > > + spin_lock(&dentry->d_lock); > > > + dentry->d_flags |= DCACHE_NOKEY_NAME; > > > + spin_unlock(&dentry->d_lock); > > > + } > > > + > > > + return err; > > > +} > > > +EXPORT_SYMBOL_GPL(fscrypt_prepare_atomic_open); > > [...] > > > +static inline int fscrypt_prepare_atomic_open(struct inode *dir, > > > + struct dentry *dentry) > > > +{ > > > + return -EOPNOTSUPP; > > > +} > > This has different behavior on unencrypted directories depending on whether > > CONFIG_FS_ENCRYPTION is enabled or not. That's bad. > > > > In patch 2, the caller you are introducing has already checked IS_ENCRYPTED(). > > > > Also, your kerneldoc comment for fscrypt_prepare_atomic_open() says it is for > > *encrypted* directories. > > > > So IMO, just remove the IS_ENCRYPTED() check from the CONFIG_FS_ENCRYPTION > > version of fscrypt_prepare_atomic_open(). > > IMO we should keep this check in fscrypt_prepare_atomic_open() to make it > consistent with the existing fscrypt_prepare_open(). And we can just remove > the check from ceph instead. > Well, then the !CONFIG_FS_ENCRYPTION version would need to return 0 if IS_ENCRYPTED() too. Either way would be okay, but please don't do a mix of both approaches within a single function, as this patch currently does. Note that there are other fscrypt_* functions, such as fscrypt_get_symlink(), that require an IS_ENCRYPTED() inode, so that pattern is not new. - Eric ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() 2023-03-14 2:25 ` Eric Biggers @ 2023-03-14 4:20 ` Xiubo Li 2023-03-14 9:25 ` Luís Henriques 0 siblings, 1 reply; 18+ messages in thread From: Xiubo Li @ 2023-03-14 4:20 UTC (permalink / raw) To: Eric Biggers Cc: Luís Henriques, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel On 14/03/2023 10:25, Eric Biggers wrote: > On Tue, Mar 14, 2023 at 08:53:51AM +0800, Xiubo Li wrote: >> On 14/03/2023 02:09, Eric Biggers wrote: >>> On Mon, Mar 13, 2023 at 12:33:09PM +0000, Luís Henriques wrote: >>>> + * The regular open path will use fscrypt_file_open for that, but in the >>>> + * atomic open a different approach is required. >>> This should actually be fscrypt_prepare_lookup, not fscrypt_file_open, right? >>> >>>> +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry) >>>> +{ >>>> + int err; >>>> + >>>> + if (!IS_ENCRYPTED(dir)) >>>> + return 0; >>>> + >>>> + err = fscrypt_get_encryption_info(dir, true); >>>> + if (!err && !fscrypt_has_encryption_key(dir)) { >>>> + spin_lock(&dentry->d_lock); >>>> + dentry->d_flags |= DCACHE_NOKEY_NAME; >>>> + spin_unlock(&dentry->d_lock); >>>> + } >>>> + >>>> + return err; >>>> +} >>>> +EXPORT_SYMBOL_GPL(fscrypt_prepare_atomic_open); >>> [...] >>>> +static inline int fscrypt_prepare_atomic_open(struct inode *dir, >>>> + struct dentry *dentry) >>>> +{ >>>> + return -EOPNOTSUPP; >>>> +} >>> This has different behavior on unencrypted directories depending on whether >>> CONFIG_FS_ENCRYPTION is enabled or not. That's bad. >>> >>> In patch 2, the caller you are introducing has already checked IS_ENCRYPTED(). >>> >>> Also, your kerneldoc comment for fscrypt_prepare_atomic_open() says it is for >>> *encrypted* directories. >>> >>> So IMO, just remove the IS_ENCRYPTED() check from the CONFIG_FS_ENCRYPTION >>> version of fscrypt_prepare_atomic_open(). >> IMO we should keep this check in fscrypt_prepare_atomic_open() to make it >> consistent with the existing fscrypt_prepare_open(). And we can just remove >> the check from ceph instead. >> > Well, then the !CONFIG_FS_ENCRYPTION version would need to return 0 if > IS_ENCRYPTED() too. For the !CONFIG_FS_ENCRYPTION version I think you mean: static inline int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry) { if (IS_ENCRYPTED(dir)) return -EOPNOTSUPP; return 0; } > Either way would be okay, but please don't do a mix of both approaches within a > single function, as this patch currently does. > > Note that there are other fscrypt_* functions, such as fscrypt_get_symlink(), > that require an IS_ENCRYPTED() inode, so that pattern is not new. Yeah, correct, I didn't notice that. - Xiubo > - Eric > -- Best Regards, Xiubo Li (李秀波) Email: xiubli@redhat.com/xiubli@ibm.com Slack: @Xiubo Li ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() 2023-03-14 4:20 ` Xiubo Li @ 2023-03-14 9:25 ` Luís Henriques 0 siblings, 0 replies; 18+ messages in thread From: Luís Henriques @ 2023-03-14 9:25 UTC (permalink / raw) To: Xiubo Li Cc: Eric Biggers, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel Xiubo Li <xiubli@redhat.com> writes: > On 14/03/2023 10:25, Eric Biggers wrote: >> On Tue, Mar 14, 2023 at 08:53:51AM +0800, Xiubo Li wrote: >>> On 14/03/2023 02:09, Eric Biggers wrote: >>>> On Mon, Mar 13, 2023 at 12:33:09PM +0000, Luís Henriques wrote: >>>>> + * The regular open path will use fscrypt_file_open for that, but in the >>>>> + * atomic open a different approach is required. >>>> This should actually be fscrypt_prepare_lookup, not fscrypt_file_open, right? >>>> >>>>> +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry) >>>>> +{ >>>>> + int err; >>>>> + >>>>> + if (!IS_ENCRYPTED(dir)) >>>>> + return 0; >>>>> + >>>>> + err = fscrypt_get_encryption_info(dir, true); >>>>> + if (!err && !fscrypt_has_encryption_key(dir)) { >>>>> + spin_lock(&dentry->d_lock); >>>>> + dentry->d_flags |= DCACHE_NOKEY_NAME; >>>>> + spin_unlock(&dentry->d_lock); >>>>> + } >>>>> + >>>>> + return err; >>>>> +} >>>>> +EXPORT_SYMBOL_GPL(fscrypt_prepare_atomic_open); >>>> [...] >>>>> +static inline int fscrypt_prepare_atomic_open(struct inode *dir, >>>>> + struct dentry *dentry) >>>>> +{ >>>>> + return -EOPNOTSUPP; >>>>> +} >>>> This has different behavior on unencrypted directories depending on whether >>>> CONFIG_FS_ENCRYPTION is enabled or not. That's bad. >>>> >>>> In patch 2, the caller you are introducing has already checked IS_ENCRYPTED(). >>>> >>>> Also, your kerneldoc comment for fscrypt_prepare_atomic_open() says it is for >>>> *encrypted* directories. >>>> >>>> So IMO, just remove the IS_ENCRYPTED() check from the CONFIG_FS_ENCRYPTION >>>> version of fscrypt_prepare_atomic_open(). >>> IMO we should keep this check in fscrypt_prepare_atomic_open() to make it >>> consistent with the existing fscrypt_prepare_open(). And we can just remove >>> the check from ceph instead. >>> >> Well, then the !CONFIG_FS_ENCRYPTION version would need to return 0 if >> IS_ENCRYPTED() too. > > For the !CONFIG_FS_ENCRYPTION version I think you mean: > > static inline int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry > *dentry) > > { > if (IS_ENCRYPTED(dir)) > return -EOPNOTSUPP; > return 0; > } > > >> Either way would be okay, but please don't do a mix of both approaches within a >> single function, as this patch currently does. >> >> Note that there are other fscrypt_* functions, such as fscrypt_get_symlink(), >> that require an IS_ENCRYPTED() inode, so that pattern is not new. > > Yeah, correct, I didn't notice that. OK, thank you both for the feedback. I'll send out v2 in a few hours. But my preference will be to drop the IS_ENCRYPTED() from fscrypt_prepare_atomic_open(). The reason is that we still need to keep it in the caller function anyway, because we need to set the MDS flags accordingly (see patch 2): if (IS_ENCRYPTED(dir)) { set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags); err = fscrypt_prepare_atomic_open(dir, dentry); if (err) goto out_req; } Cheers, -- Luís ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() 2023-03-13 18:09 ` Eric Biggers 2023-03-14 0:53 ` Xiubo Li @ 2023-03-14 10:15 ` Luís Henriques 2023-03-14 17:56 ` Eric Biggers 1 sibling, 1 reply; 18+ messages in thread From: Luís Henriques @ 2023-03-14 10:15 UTC (permalink / raw) To: Eric Biggers Cc: Xiubo Li, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel Eric Biggers <ebiggers@kernel.org> writes: > On Mon, Mar 13, 2023 at 12:33:09PM +0000, Luís Henriques wrote: >> + * The regular open path will use fscrypt_file_open for that, but in the >> + * atomic open a different approach is required. > > This should actually be fscrypt_prepare_lookup, not fscrypt_file_open, right? Ups, I missed this comment. I was comparing the regular open() with the atomic_open() paths. I think I really mean fscrypt_file_open() because that's where the encryption info is (or may be) set by calling fscrypt_require_key(). atomic_open needs something similar, but combined with a lookup. Maybe I can rephrase it to: The reason for getting the encryption info before checking if the directory has the encryption key is because the key may be available but the encryption info isn't yet set (maybe due to a drop_caches). The regular open path will call fscrypt_file_open which uses function fscrypt_require_key for setting the encryption info if needed. The atomic open needs to do something similar. Cheers, -- Luís >> +int fscrypt_prepare_atomic_open(struct inode *dir, struct dentry *dentry) >> +{ >> + int err; >> + >> + if (!IS_ENCRYPTED(dir)) >> + return 0; >> + >> + err = fscrypt_get_encryption_info(dir, true); >> + if (!err && !fscrypt_has_encryption_key(dir)) { >> + spin_lock(&dentry->d_lock); >> + dentry->d_flags |= DCACHE_NOKEY_NAME; >> + spin_unlock(&dentry->d_lock); >> + } >> + >> + return err; >> +} >> +EXPORT_SYMBOL_GPL(fscrypt_prepare_atomic_open); > [...] >> +static inline int fscrypt_prepare_atomic_open(struct inode *dir, >> + struct dentry *dentry) >> +{ >> + return -EOPNOTSUPP; >> +} > > This has different behavior on unencrypted directories depending on whether > CONFIG_FS_ENCRYPTION is enabled or not. That's bad. > > In patch 2, the caller you are introducing has already checked IS_ENCRYPTED(). > > Also, your kerneldoc comment for fscrypt_prepare_atomic_open() says it is for > *encrypted* directories. > > So IMO, just remove the IS_ENCRYPTED() check from the CONFIG_FS_ENCRYPTION > version of fscrypt_prepare_atomic_open(). > > - Eric ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() 2023-03-14 10:15 ` Luís Henriques @ 2023-03-14 17:56 ` Eric Biggers 2023-03-15 11:08 ` Luís Henriques 0 siblings, 1 reply; 18+ messages in thread From: Eric Biggers @ 2023-03-14 17:56 UTC (permalink / raw) To: Luís Henriques Cc: Xiubo Li, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel On Tue, Mar 14, 2023 at 10:15:11AM +0000, Luís Henriques wrote: > Eric Biggers <ebiggers@kernel.org> writes: > > > On Mon, Mar 13, 2023 at 12:33:09PM +0000, Luís Henriques wrote: > >> + * The regular open path will use fscrypt_file_open for that, but in the > >> + * atomic open a different approach is required. > > > > This should actually be fscrypt_prepare_lookup, not fscrypt_file_open, right? > > Ups, I missed this comment. > > I was comparing the regular open() with the atomic_open() paths. I think > I really mean fscrypt_file_open() because that's where the encryption info > is (or may be) set by calling fscrypt_require_key(). atomic_open needs > something similar, but combined with a lookup. > > Maybe I can rephrase it to: > > The reason for getting the encryption info before checking if the > directory has the encryption key is because the key may be available but > the encryption info isn't yet set (maybe due to a drop_caches). The > regular open path will call fscrypt_file_open which uses function > fscrypt_require_key for setting the encryption info if needed. The > atomic open needs to do something similar. > No, regular open is two parts: ->lookup and ->open. fscrypt_prepare_lookup() sets up the directory's key, whereas fscrypt_file_open() sets up the file's key. Your proposed fscrypt_prepare_atomic_open() sets up the directory's key. So it is really fscrypt_prepare_lookup() that is its equivalent. However, that raises the question of why doesn't ceph just use fscrypt_prepare_lookup()? It seems the answer is that ceph wants to handle the filenames encryption and no-key name encoding itself. And for that reason, its ->lookup() does the following and does *not* use fscrypt_prepare_lookup(): if (IS_ENCRYPTED(dir)) { err = ceph_fscrypt_prepare_readdir(dir); if (err < 0) return ERR_PTR(err); if (!fscrypt_has_encryption_key(dir)) { spin_lock(&dentry->d_lock); dentry->d_flags |= DCACHE_NOKEY_NAME; spin_unlock(&dentry->d_lock); } } So, actually I think this patch doesn't make sense. If ceph is doing the above in its ->lookup() anyway, then it just should do the exact same thing in its ->atomic_open() too. If you want to add a new fscrypt_* helper function which *just* sets up the given directory's key and sets the NOKEY_NAME flag on the given dentry accordingly, that could make sense. However, it should be called from *both* ->lookup() and ->atomic_open(), not just ->atomic_open(). It's also worth mentioning that setting up the filename separately from the NOKEY_NAME flag makes ceph have the same race condition that I had fixed for the other filesystems in commit b01531db6cec ("fscrypt: fix race where ->lookup() marks plaintext dentry as ciphertext"). It's not a huge deal, but it can cause some odd behavior, so it's worth thinking about whether it can be solved. - Eric ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() 2023-03-14 17:56 ` Eric Biggers @ 2023-03-15 11:08 ` Luís Henriques 2023-03-15 17:12 ` Eric Biggers 0 siblings, 1 reply; 18+ messages in thread From: Luís Henriques @ 2023-03-15 11:08 UTC (permalink / raw) To: Eric Biggers Cc: Xiubo Li, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel Eric Biggers <ebiggers@kernel.org> writes: > On Tue, Mar 14, 2023 at 10:15:11AM +0000, Luís Henriques wrote: >> Eric Biggers <ebiggers@kernel.org> writes: >> >> > On Mon, Mar 13, 2023 at 12:33:09PM +0000, Luís Henriques wrote: >> >> + * The regular open path will use fscrypt_file_open for that, but in the >> >> + * atomic open a different approach is required. >> > >> > This should actually be fscrypt_prepare_lookup, not fscrypt_file_open, right? >> >> Ups, I missed this comment. >> >> I was comparing the regular open() with the atomic_open() paths. I think >> I really mean fscrypt_file_open() because that's where the encryption info >> is (or may be) set by calling fscrypt_require_key(). atomic_open needs >> something similar, but combined with a lookup. >> >> Maybe I can rephrase it to: >> >> The reason for getting the encryption info before checking if the >> directory has the encryption key is because the key may be available but >> the encryption info isn't yet set (maybe due to a drop_caches). The >> regular open path will call fscrypt_file_open which uses function >> fscrypt_require_key for setting the encryption info if needed. The >> atomic open needs to do something similar. >> > > No, regular open is two parts: ->lookup and ->open. fscrypt_prepare_lookup() > sets up the directory's key, whereas fscrypt_file_open() sets up the file's key. > > Your proposed fscrypt_prepare_atomic_open() sets up the directory's key. So it > is really fscrypt_prepare_lookup() that is its equivalent. Oh, I see what you mean now, and you're obviously correct. Thanks for the detailed explanation. > However, that raises the question of why doesn't ceph just use > fscrypt_prepare_lookup()? It seems the answer is that ceph wants to handle the > filenames encryption and no-key name encoding itself. And for that reason, its > ->lookup() does the following and does *not* use fscrypt_prepare_lookup(): > > if (IS_ENCRYPTED(dir)) { > err = ceph_fscrypt_prepare_readdir(dir); > if (err < 0) > return ERR_PTR(err); > if (!fscrypt_has_encryption_key(dir)) { > spin_lock(&dentry->d_lock); > dentry->d_flags |= DCACHE_NOKEY_NAME; > spin_unlock(&dentry->d_lock); > } > } Ugh, I tend to forget all the details behind these decisions. If I remember correctly, we had to work around the fact that the cephfs client handle directory data in a cumbersome way. We may not have the full data for a readdir, for example, and that has to be handled by a lookup. > So, actually I think this patch doesn't make sense. If ceph is doing the above > in its ->lookup() anyway, then it just should do the exact same thing in its > ->atomic_open() too. In fact, my initial fix for the cephfs bug was doing just that. It was a single patch to ceph_atomic_open() that would simply do: if (IS_ENCRYPTED(dir)) { set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags); err = __fscrypt_prepare_readdir(dir); if (!err && !fscrypt_has_encryption_key(dir)) { spin_lock(&dentry->d_lock); dentry->d_flags |= DCACHE_NOKEY_NAME; spin_unlock(&dentry->d_lock); } } What made me want to create a new helper was that I simply needed to call fscrypt_get_encryption_info() to force the encryption info to be set in the parent directory. But this function was only accessible through __fscrypt_prepare_readdir(), which isn't really a great function name for what I need here. Since __fscrypt_prepare_readdir() doesn't seem to be used anywhere else, maybe it could be removed and fscrypt_get_encryption_info() be exported instead? > If you want to add a new fscrypt_* helper function which *just* sets up the > given directory's key and sets the NOKEY_NAME flag on the given dentry > accordingly, that could make sense. However, it should be called from *both* > ->lookup() and ->atomic_open(), not just ->atomic_open(). > > It's also worth mentioning that setting up the filename separately from the > NOKEY_NAME flag makes ceph have the same race condition that I had fixed for the > other filesystems in commit b01531db6cec ("fscrypt: fix race where ->lookup() > marks plaintext dentry as ciphertext"). It's not a huge deal, but it can cause > some odd behavior, so it's worth thinking about whether it can be solved. Hmm... OK, looks like we'll need to have a look into this. Thanks for the heads-up. Cheers, -- Luís ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() 2023-03-15 11:08 ` Luís Henriques @ 2023-03-15 17:12 ` Eric Biggers 2023-03-15 17:59 ` Luís Henriques 0 siblings, 1 reply; 18+ messages in thread From: Eric Biggers @ 2023-03-15 17:12 UTC (permalink / raw) To: Luís Henriques Cc: Xiubo Li, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel On Wed, Mar 15, 2023 at 11:08:23AM +0000, Luís Henriques wrote: > > So, actually I think this patch doesn't make sense. If ceph is doing the above > > in its ->lookup() anyway, then it just should do the exact same thing in its > > ->atomic_open() too. > > In fact, my initial fix for the cephfs bug was doing just that. It was a > single patch to ceph_atomic_open() that would simply do: > > if (IS_ENCRYPTED(dir)) { > set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags); > err = __fscrypt_prepare_readdir(dir); > if (!err && !fscrypt_has_encryption_key(dir)) { > spin_lock(&dentry->d_lock); > dentry->d_flags |= DCACHE_NOKEY_NAME; > spin_unlock(&dentry->d_lock); > } > } > > What made me want to create a new helper was that I simply needed to call > fscrypt_get_encryption_info() to force the encryption info to be set in > the parent directory. But this function was only accessible through > __fscrypt_prepare_readdir(), which isn't really a great function name for > what I need here. > > Since __fscrypt_prepare_readdir() doesn't seem to be used anywhere else, > maybe it could be removed and fscrypt_get_encryption_info() be exported > instead? Well, fscrypt_get_encryption_info() *used* to be exported, but it was hard to keep track of its use cases (some of which were not actually necessary), which is why it eventually got replaced with use-case oriented helper functions. Maybe just use fscrypt_prepare_lookup_partial() for the name of your new helper function (instead of fscrypt_prepare_atomic_open())? - Eric ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() 2023-03-15 17:12 ` Eric Biggers @ 2023-03-15 17:59 ` Luís Henriques 0 siblings, 0 replies; 18+ messages in thread From: Luís Henriques @ 2023-03-15 17:59 UTC (permalink / raw) To: Eric Biggers Cc: Xiubo Li, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel Eric Biggers <ebiggers@kernel.org> writes: > On Wed, Mar 15, 2023 at 11:08:23AM +0000, Luís Henriques wrote: >> > So, actually I think this patch doesn't make sense. If ceph is doing the above >> > in its ->lookup() anyway, then it just should do the exact same thing in its >> > ->atomic_open() too. >> >> In fact, my initial fix for the cephfs bug was doing just that. It was a >> single patch to ceph_atomic_open() that would simply do: >> >> if (IS_ENCRYPTED(dir)) { >> set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags); >> err = __fscrypt_prepare_readdir(dir); >> if (!err && !fscrypt_has_encryption_key(dir)) { >> spin_lock(&dentry->d_lock); >> dentry->d_flags |= DCACHE_NOKEY_NAME; >> spin_unlock(&dentry->d_lock); >> } >> } >> >> What made me want to create a new helper was that I simply needed to call >> fscrypt_get_encryption_info() to force the encryption info to be set in >> the parent directory. But this function was only accessible through >> __fscrypt_prepare_readdir(), which isn't really a great function name for >> what I need here. >> >> Since __fscrypt_prepare_readdir() doesn't seem to be used anywhere else, >> maybe it could be removed and fscrypt_get_encryption_info() be exported >> instead? > > Well, fscrypt_get_encryption_info() *used* to be exported, but it was hard to > keep track of its use cases (some of which were not actually necessary), which > is why it eventually got replaced with use-case oriented helper functions. > > Maybe just use fscrypt_prepare_lookup_partial() for the name of your new helper > function (instead of fscrypt_prepare_atomic_open())? OK, thanks for the name suggestion (naming is *indeed* hard). I'll go try to get a new helper that can be used in both open_atomic and lookup. That'll require a bit more of testing so that I don't end up breaking something else. Cheers, -- Luís ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] ceph: switch atomic open to use new fscrypt helper 2023-03-13 12:33 [PATCH 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories Luís Henriques 2023-03-13 12:33 ` [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() Luís Henriques @ 2023-03-13 12:33 ` Luís Henriques 2023-03-13 18:11 ` Eric Biggers 2023-03-13 17:11 ` [PATCH 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories Jeff Layton 2 siblings, 1 reply; 18+ messages in thread From: Luís Henriques @ 2023-03-13 12:33 UTC (permalink / raw) To: Eric Biggers, Xiubo Li, Jeff Layton Cc: Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel, Luís Henriques Switch ceph atomic open to use fscrypt_prepare_atomic_open(). This fixes a bug where a dentry is incorrectly set with DCACHE_NOKEY_NAME when 'dir' has been evicted but the key is still available (for example, where there's a drop_caches). Signed-off-by: Luís Henriques <lhenriques@suse.de> --- fs/ceph/file.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/fs/ceph/file.c b/fs/ceph/file.c index dee3b445f415..5ad57cc4c13b 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -795,11 +795,9 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry, ihold(dir); if (IS_ENCRYPTED(dir)) { set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags); - if (!fscrypt_has_encryption_key(dir)) { - spin_lock(&dentry->d_lock); - dentry->d_flags |= DCACHE_NOKEY_NAME; - spin_unlock(&dentry->d_lock); - } + err = fscrypt_prepare_atomic_open(dir, dentry); + if (err) + goto out_req; } if (flags & O_CREAT) { ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] ceph: switch atomic open to use new fscrypt helper 2023-03-13 12:33 ` [PATCH 2/2] ceph: switch atomic open to use new fscrypt helper Luís Henriques @ 2023-03-13 18:11 ` Eric Biggers 2023-03-13 18:42 ` Luís Henriques 0 siblings, 1 reply; 18+ messages in thread From: Eric Biggers @ 2023-03-13 18:11 UTC (permalink / raw) To: Luís Henriques Cc: Xiubo Li, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel On Mon, Mar 13, 2023 at 12:33:10PM +0000, Luís Henriques wrote: > Switch ceph atomic open to use fscrypt_prepare_atomic_open(). This fixes > a bug where a dentry is incorrectly set with DCACHE_NOKEY_NAME when 'dir' > has been evicted but the key is still available (for example, where there's > a drop_caches). > > Signed-off-by: Luís Henriques <lhenriques@suse.de> > --- > fs/ceph/file.c | 8 +++----- > 1 file changed, 3 insertions(+), 5 deletions(-) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index dee3b445f415..5ad57cc4c13b 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -795,11 +795,9 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry, > ihold(dir); > if (IS_ENCRYPTED(dir)) { > set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags); > - if (!fscrypt_has_encryption_key(dir)) { > - spin_lock(&dentry->d_lock); > - dentry->d_flags |= DCACHE_NOKEY_NAME; > - spin_unlock(&dentry->d_lock); > - } > + err = fscrypt_prepare_atomic_open(dir, dentry); > + if (err) > + goto out_req; Note that this patch does not apply to upstream or even to linux-next. I'd be glad to take patch 1 through the fscrypt tree for 6.4. But I'm wondering what the current plans are for getting ceph's fscrypt support upstream? - Eric ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] ceph: switch atomic open to use new fscrypt helper 2023-03-13 18:11 ` Eric Biggers @ 2023-03-13 18:42 ` Luís Henriques 2023-03-14 0:38 ` Xiubo Li 0 siblings, 1 reply; 18+ messages in thread From: Luís Henriques @ 2023-03-13 18:42 UTC (permalink / raw) To: Eric Biggers Cc: Xiubo Li, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel Eric Biggers <ebiggers@kernel.org> writes: > On Mon, Mar 13, 2023 at 12:33:10PM +0000, Luís Henriques wrote: >> Switch ceph atomic open to use fscrypt_prepare_atomic_open(). This fixes >> a bug where a dentry is incorrectly set with DCACHE_NOKEY_NAME when 'dir' >> has been evicted but the key is still available (for example, where there's >> a drop_caches). >> >> Signed-off-by: Luís Henriques <lhenriques@suse.de> >> --- >> fs/ceph/file.c | 8 +++----- >> 1 file changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/fs/ceph/file.c b/fs/ceph/file.c >> index dee3b445f415..5ad57cc4c13b 100644 >> --- a/fs/ceph/file.c >> +++ b/fs/ceph/file.c >> @@ -795,11 +795,9 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry, >> ihold(dir); >> if (IS_ENCRYPTED(dir)) { >> set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags); >> - if (!fscrypt_has_encryption_key(dir)) { >> - spin_lock(&dentry->d_lock); >> - dentry->d_flags |= DCACHE_NOKEY_NAME; >> - spin_unlock(&dentry->d_lock); >> - } >> + err = fscrypt_prepare_atomic_open(dir, dentry); >> + if (err) >> + goto out_req; > > Note that this patch does not apply to upstream or even to linux-next. True, I should have mentioned that in the cover-letter. This patch should be applied against the 'testing' branch in https://github.com/ceph/ceph-client, which is where the ceph fscrypt currently lives. > I'd be glad to take patch 1 through the fscrypt tree for 6.4. But I'm wondering > what the current plans are for getting ceph's fscrypt support upstream? As far as I know, the current plan is to try to merge the ceph code during the next merge window for 6.4 (but Xiubo and Ilya may correct me if I'm wrong). Also, regarding who picks which patch, I'm fine with you picking the first one. But I'll let the ceph maintainers say what they think, because it may be easier for them to keep both patches together due to the testing infrastructure being used. Anyway, I'll send out a new rev tomorrow taking your comments into account. Thanks, Eric! Cheers, -- Luís ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] ceph: switch atomic open to use new fscrypt helper 2023-03-13 18:42 ` Luís Henriques @ 2023-03-14 0:38 ` Xiubo Li 2023-03-14 9:27 ` Luís Henriques 0 siblings, 1 reply; 18+ messages in thread From: Xiubo Li @ 2023-03-14 0:38 UTC (permalink / raw) To: Luís Henriques, Eric Biggers Cc: Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel On 14/03/2023 02:42, Luís Henriques wrote: > Eric Biggers <ebiggers@kernel.org> writes: > >> On Mon, Mar 13, 2023 at 12:33:10PM +0000, Luís Henriques wrote: >>> Switch ceph atomic open to use fscrypt_prepare_atomic_open(). This fixes >>> a bug where a dentry is incorrectly set with DCACHE_NOKEY_NAME when 'dir' >>> has been evicted but the key is still available (for example, where there's >>> a drop_caches). >>> >>> Signed-off-by: Luís Henriques <lhenriques@suse.de> >>> --- >>> fs/ceph/file.c | 8 +++----- >>> 1 file changed, 3 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c >>> index dee3b445f415..5ad57cc4c13b 100644 >>> --- a/fs/ceph/file.c >>> +++ b/fs/ceph/file.c >>> @@ -795,11 +795,9 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry, >>> ihold(dir); >>> if (IS_ENCRYPTED(dir)) { >>> set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags); >>> - if (!fscrypt_has_encryption_key(dir)) { >>> - spin_lock(&dentry->d_lock); >>> - dentry->d_flags |= DCACHE_NOKEY_NAME; >>> - spin_unlock(&dentry->d_lock); >>> - } >>> + err = fscrypt_prepare_atomic_open(dir, dentry); >>> + if (err) >>> + goto out_req; >> Note that this patch does not apply to upstream or even to linux-next. > True, I should have mentioned that in the cover-letter. This patch should > be applied against the 'testing' branch in https://github.com/ceph/ceph-client, > which is where the ceph fscrypt currently lives. > >> I'd be glad to take patch 1 through the fscrypt tree for 6.4. But I'm wondering >> what the current plans are for getting ceph's fscrypt support upstream? > As far as I know, the current plan is to try to merge the ceph code during > the next merge window for 6.4 (but Xiubo and Ilya may correct me if I'm > wrong). Also, regarding who picks which patch, I'm fine with you picking > the first one. But I'll let the ceph maintainers say what they think, > because it may be easier for them to keep both patches together due to the > testing infrastructure being used. > > Anyway, I'll send out a new rev tomorrow taking your comments into > account. Thanks, Eric! Eric, Luis, It will be fine if Eric could merge patch 1 into the fscrypt tree. Then I will merge the patch 1 into the ceph-client's testing by tagging as [DO NOT MERGE] to run our tests. And locally we are still running the test, and there have several fixes followed and need more time to review. Thanks - Xiubo > Cheers, -- Best Regards, Xiubo Li (李秀波) Email: xiubli@redhat.com/xiubli@ibm.com Slack: @Xiubo Li ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] ceph: switch atomic open to use new fscrypt helper 2023-03-14 0:38 ` Xiubo Li @ 2023-03-14 9:27 ` Luís Henriques 0 siblings, 0 replies; 18+ messages in thread From: Luís Henriques @ 2023-03-14 9:27 UTC (permalink / raw) To: Xiubo Li Cc: Eric Biggers, Jeff Layton, Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel Xiubo Li <xiubli@redhat.com> writes: > On 14/03/2023 02:42, Luís Henriques wrote: >> Eric Biggers <ebiggers@kernel.org> writes: >> >>> On Mon, Mar 13, 2023 at 12:33:10PM +0000, Luís Henriques wrote: >>>> Switch ceph atomic open to use fscrypt_prepare_atomic_open(). This fixes >>>> a bug where a dentry is incorrectly set with DCACHE_NOKEY_NAME when 'dir' >>>> has been evicted but the key is still available (for example, where there's >>>> a drop_caches). >>>> >>>> Signed-off-by: Luís Henriques <lhenriques@suse.de> >>>> --- >>>> fs/ceph/file.c | 8 +++----- >>>> 1 file changed, 3 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c >>>> index dee3b445f415..5ad57cc4c13b 100644 >>>> --- a/fs/ceph/file.c >>>> +++ b/fs/ceph/file.c >>>> @@ -795,11 +795,9 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry, >>>> ihold(dir); >>>> if (IS_ENCRYPTED(dir)) { >>>> set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags); >>>> - if (!fscrypt_has_encryption_key(dir)) { >>>> - spin_lock(&dentry->d_lock); >>>> - dentry->d_flags |= DCACHE_NOKEY_NAME; >>>> - spin_unlock(&dentry->d_lock); >>>> - } >>>> + err = fscrypt_prepare_atomic_open(dir, dentry); >>>> + if (err) >>>> + goto out_req; >>> Note that this patch does not apply to upstream or even to linux-next. >> True, I should have mentioned that in the cover-letter. This patch should >> be applied against the 'testing' branch in https://github.com/ceph/ceph-client, >> which is where the ceph fscrypt currently lives. >> >>> I'd be glad to take patch 1 through the fscrypt tree for 6.4. But I'm wondering >>> what the current plans are for getting ceph's fscrypt support upstream? >> As far as I know, the current plan is to try to merge the ceph code during >> the next merge window for 6.4 (but Xiubo and Ilya may correct me if I'm >> wrong). Also, regarding who picks which patch, I'm fine with you picking >> the first one. But I'll let the ceph maintainers say what they think, >> because it may be easier for them to keep both patches together due to the >> testing infrastructure being used. >> >> Anyway, I'll send out a new rev tomorrow taking your comments into >> account. Thanks, Eric! > > Eric, Luis, > > It will be fine if Eric could merge patch 1 into the fscrypt tree. Then I will > merge the patch 1 into the ceph-client's testing by tagging as [DO NOT MERGE] to > run our tests. Awesome, so Eric can pick the first patch. Thanks. Cheers, -- Luís > And locally we are still running the test, and there have several fixes followed > and need more time to review. > > Thanks > > - Xiubo > >> Cheers, > > -- > Best Regards, > > Xiubo Li (李秀波) > > Email: xiubli@redhat.com/xiubli@ibm.com > Slack: @Xiubo Li > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories 2023-03-13 12:33 [PATCH 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories Luís Henriques 2023-03-13 12:33 ` [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() Luís Henriques 2023-03-13 12:33 ` [PATCH 2/2] ceph: switch atomic open to use new fscrypt helper Luís Henriques @ 2023-03-13 17:11 ` Jeff Layton 2 siblings, 0 replies; 18+ messages in thread From: Jeff Layton @ 2023-03-13 17:11 UTC (permalink / raw) To: Luís Henriques, Eric Biggers, Xiubo Li Cc: Theodore Y. Ts'o, Jaegeuk Kim, Ilya Dryomov, linux-fscrypt, ceph-devel, linux-kernel On Mon, 2023-03-13 at 12:33 +0000, Luís Henriques wrote: > Hi! > > I started seeing fstest generic/123 failing in ceph fscrypt, when running it > with 'test_dummy_encryption'. This test is quite simple: > > 1. Creates a directory with write permissions for root only > 2. Writes into a file in that directory > 3. Uses 'su' to try to modify that file as a different user, and > gets -EPERM > > All the test steps succeed, but the test fails to cleanup: 'rm -rf <dir>' > will fail with -ENOTEMPTY. 'strace' shows that calling unlinkat() to remove > the file got a -ENOENT and then -ENOTEMPTY for the directory. > > This is because 'su' does a drop_caches ('su (874): drop_caches: 2' in > dmesg), and ceph's atomic open will do: > > if (IS_ENCRYPTED(dir)) { > set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags); > if (!fscrypt_has_encryption_key(dir)) { > spin_lock(&dentry->d_lock); > dentry->d_flags |= DCACHE_NOKEY_NAME; > spin_unlock(&dentry->d_lock); > } > } > > Although 'dir' has the encryption key available, fscrypt_has_encryption_key() > will return 'false' because fscrypt info isn't yet set after the cache > cleanup. > > The first patch will add a new helper for the atomic_open that will force > the fscrypt info to be loaded into an inode that has been evicted recently > but for which the key is still available. > > The second patch switches ceph atomic_open to use the new fscrypt helper. > > Cheers, > -- > Luís > > Changes since initial RFC (after Eric's review): > - Added kerneldoc comments to the new fscrypt helper > - Dropped '__' from helper name (now fscrypt_prepare_atomic_open()) > - Added IS_ENCRYPTED() check in helper > - DCACHE_NOKEY_NAME is not set if fscrypt_get_encryption_info() returns an > error > - Fixed helper for !CONFIG_FS_ENCRYPTION (now defined 'static inline') > > Luís Henriques (2): > fscrypt: new helper function - fscrypt_prepare_atomic_open() > ceph: switch atomic open to use new fscrypt helper > > fs/ceph/file.c | 8 +++----- > fs/crypto/hooks.c | 35 +++++++++++++++++++++++++++++++++++ > include/linux/fscrypt.h | 7 +++++++ > 3 files changed, 45 insertions(+), 5 deletions(-) > Looks like a nice cleanup. Reviewed-by: Jeff Layton <jlayton@kernel.org> ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-03-15 17:59 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-13 12:33 [PATCH 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories Luís Henriques 2023-03-13 12:33 ` [PATCH 1/2] fscrypt: new helper function - fscrypt_prepare_atomic_open() Luís Henriques 2023-03-13 18:09 ` Eric Biggers 2023-03-14 0:53 ` Xiubo Li 2023-03-14 2:25 ` Eric Biggers 2023-03-14 4:20 ` Xiubo Li 2023-03-14 9:25 ` Luís Henriques 2023-03-14 10:15 ` Luís Henriques 2023-03-14 17:56 ` Eric Biggers 2023-03-15 11:08 ` Luís Henriques 2023-03-15 17:12 ` Eric Biggers 2023-03-15 17:59 ` Luís Henriques 2023-03-13 12:33 ` [PATCH 2/2] ceph: switch atomic open to use new fscrypt helper Luís Henriques 2023-03-13 18:11 ` Eric Biggers 2023-03-13 18:42 ` Luís Henriques 2023-03-14 0:38 ` Xiubo Li 2023-03-14 9:27 ` Luís Henriques 2023-03-13 17:11 ` [PATCH 0/2] ceph: fscrypt: fix atomic open bug for encrypted directories Jeff Layton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).