| Message ID | 122a3db06dbf6ac1ece5660895a69039fe45f50d.1701468306.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | btrfs: add fscrypt support | expand |
On Fri, Dec 01, 2023 at 05:10:58PM -0500, Josef Bacik wrote: > btrfs has a variety of asynchronous things we do with inodes that can > potentially last until ->put_super, when we shut everything down and > clean up all of our async work. Due to this we need to move > fscrypt_destroy_keyring() to after ->put_super, otherwise we get > warnings about still having active references on the master key. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/super.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/super.c b/fs/super.c > index 076392396e72..faf7d248145d 100644 > --- a/fs/super.c > +++ b/fs/super.c > @@ -681,12 +681,6 @@ void generic_shutdown_super(struct super_block *sb) > fsnotify_sb_delete(sb); > security_sb_delete(sb); > > - /* > - * Now that all potentially-encrypted inodes have been evicted, > - * the fscrypt keyring can be destroyed. > - */ > - fscrypt_destroy_keyring(sb); > - > if (sb->s_dio_done_wq) { > destroy_workqueue(sb->s_dio_done_wq); > sb->s_dio_done_wq = NULL; > @@ -695,6 +689,12 @@ void generic_shutdown_super(struct super_block *sb) > if (sop->put_super) > sop->put_super(sb); > > + /* > + * Now that all potentially-encrypted inodes have been evicted, > + * the fscrypt keyring can be destroyed. > + */ > + fscrypt_destroy_keyring(sb); > + This patch will cause a NULL dereference on f2fs, since f2fs_put_super() frees ->s_fs_info, and then fscrypt_destroy_keyring() can call f2fs_get_devices() (via fscrypt_operations::get_devices) which dereferences it. (ext4 also frees ->s_fs_info in its ->put_super, but ext4 doesn't implement ->get_devices.) I think we need to move the fscrypt keyring destruction into ->put_super for each filesystem. - Eric
On Mon, Dec 04, 2023 at 05:58:00PM -0800, Eric Biggers wrote: > On Fri, Dec 01, 2023 at 05:10:58PM -0500, Josef Bacik wrote: > > btrfs has a variety of asynchronous things we do with inodes that can > > potentially last until ->put_super, when we shut everything down and > > clean up all of our async work. Due to this we need to move > > fscrypt_destroy_keyring() to after ->put_super, otherwise we get > > warnings about still having active references on the master key. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/super.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/fs/super.c b/fs/super.c > > index 076392396e72..faf7d248145d 100644 > > --- a/fs/super.c > > +++ b/fs/super.c > > @@ -681,12 +681,6 @@ void generic_shutdown_super(struct super_block *sb) > > fsnotify_sb_delete(sb); > > security_sb_delete(sb); > > > > - /* > > - * Now that all potentially-encrypted inodes have been evicted, > > - * the fscrypt keyring can be destroyed. > > - */ > > - fscrypt_destroy_keyring(sb); > > - > > if (sb->s_dio_done_wq) { > > destroy_workqueue(sb->s_dio_done_wq); > > sb->s_dio_done_wq = NULL; > > @@ -695,6 +689,12 @@ void generic_shutdown_super(struct super_block *sb) > > if (sop->put_super) > > sop->put_super(sb); > > > > + /* > > + * Now that all potentially-encrypted inodes have been evicted, > > + * the fscrypt keyring can be destroyed. > > + */ > > + fscrypt_destroy_keyring(sb); > > + > > This patch will cause a NULL dereference on f2fs, since f2fs_put_super() frees > ->s_fs_info, and then fscrypt_destroy_keyring() can call f2fs_get_devices() (via > fscrypt_operations::get_devices) which dereferences it. (ext4 also frees > ->s_fs_info in its ->put_super, but ext4 doesn't implement ->get_devices.) > > I think we need to move the fscrypt keyring destruction into ->put_super for > each filesystem. I can do this, I'll send a separate series for this since this should be straightforward and we can get that part done. Thanks, Josef
On Tue, Dec 05, 2023 at 05:48:48PM -0500, Josef Bacik wrote: > On Mon, Dec 04, 2023 at 05:58:00PM -0800, Eric Biggers wrote: > > On Fri, Dec 01, 2023 at 05:10:58PM -0500, Josef Bacik wrote: > > > btrfs has a variety of asynchronous things we do with inodes that can > > > potentially last until ->put_super, when we shut everything down and > > > clean up all of our async work. Due to this we need to move > > > fscrypt_destroy_keyring() to after ->put_super, otherwise we get > > > warnings about still having active references on the master key. > > > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > > --- > > > fs/super.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/fs/super.c b/fs/super.c > > > index 076392396e72..faf7d248145d 100644 > > > --- a/fs/super.c > > > +++ b/fs/super.c > > > @@ -681,12 +681,6 @@ void generic_shutdown_super(struct super_block *sb) > > > fsnotify_sb_delete(sb); > > > security_sb_delete(sb); > > > > > > - /* > > > - * Now that all potentially-encrypted inodes have been evicted, > > > - * the fscrypt keyring can be destroyed. > > > - */ > > > - fscrypt_destroy_keyring(sb); > > > - > > > if (sb->s_dio_done_wq) { > > > destroy_workqueue(sb->s_dio_done_wq); > > > sb->s_dio_done_wq = NULL; > > > @@ -695,6 +689,12 @@ void generic_shutdown_super(struct super_block *sb) > > > if (sop->put_super) > > > sop->put_super(sb); > > > > > > + /* > > > + * Now that all potentially-encrypted inodes have been evicted, > > > + * the fscrypt keyring can be destroyed. > > > + */ > > > + fscrypt_destroy_keyring(sb); > > > + > > > > This patch will cause a NULL dereference on f2fs, since f2fs_put_super() frees > > ->s_fs_info, and then fscrypt_destroy_keyring() can call f2fs_get_devices() (via > > fscrypt_operations::get_devices) which dereferences it. (ext4 also frees > > ->s_fs_info in its ->put_super, but ext4 doesn't implement ->get_devices.) > > > > I think we need to move the fscrypt keyring destruction into ->put_super for > > each filesystem. > > I can do this, I'll send a separate series for this since this should be > straightforward and we can get that part done. Thanks, > I actually started a patch for this earlier today, just haven't had a chance to send it out yet. I'll do so in a minute. Thanks, - Eric
diff --git a/fs/super.c b/fs/super.c index 076392396e72..faf7d248145d 100644 --- a/fs/super.c +++ b/fs/super.c @@ -681,12 +681,6 @@ void generic_shutdown_super(struct super_block *sb) fsnotify_sb_delete(sb); security_sb_delete(sb); - /* - * Now that all potentially-encrypted inodes have been evicted, - * the fscrypt keyring can be destroyed. - */ - fscrypt_destroy_keyring(sb); - if (sb->s_dio_done_wq) { destroy_workqueue(sb->s_dio_done_wq); sb->s_dio_done_wq = NULL; @@ -695,6 +689,12 @@ void generic_shutdown_super(struct super_block *sb) if (sop->put_super) sop->put_super(sb); + /* + * Now that all potentially-encrypted inodes have been evicted, + * the fscrypt keyring can be destroyed. + */ + fscrypt_destroy_keyring(sb); + if (CHECK_DATA_CORRUPTION(!list_empty(&sb->s_inodes), "VFS: Busy inodes after unmount of %s (%s)", sb->s_id, sb->s_type->name)) {
btrfs has a variety of asynchronous things we do with inodes that can potentially last until ->put_super, when we shut everything down and clean up all of our async work. Due to this we need to move fscrypt_destroy_keyring() to after ->put_super, otherwise we get warnings about still having active references on the master key. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/super.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)