| Message ID | 20240204021739.1157830-11-viro@zeniv.linux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/13] fs/super.c: don't drop ->s_user_ns until we free struct super_block itself | expand |
On Sun, Feb 04, 2024 at 02:17:37AM +0000, Al Viro wrote: > ->permission(), ->get_link() and ->inode_get_acl() might dereference > ->s_fs_info (and, in case of ->permission(), ->s_fs_info->fc->user_ns > as well) when called from rcu pathwalk. > > Freeing ->s_fs_info->fc is rcu-delayed; we need to make freeing ->s_fs_info > and dropping ->user_ns rcu-delayed too. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- Reviewed-by: Christian Brauner <brauner@kernel.org>
On Mon, 5 Feb 2024 at 13:31, Christian Brauner <brauner@kernel.org> wrote: > > On Sun, Feb 04, 2024 at 02:17:37AM +0000, Al Viro wrote: > > ->permission(), ->get_link() and ->inode_get_acl() might dereference > > ->s_fs_info (and, in case of ->permission(), ->s_fs_info->fc->user_ns > > as well) when called from rcu pathwalk. > > > > Freeing ->s_fs_info->fc is rcu-delayed; we need to make freeing ->s_fs_info > > and dropping ->user_ns rcu-delayed too. > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > --- > > Reviewed-by: Christian Brauner <brauner@kernel.org> Acked-by: Miklos Szeredi <mszeredi@redhat.com>
On Mon, Feb 5, 2024 at 3:52 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Mon, 5 Feb 2024 at 13:31, Christian Brauner <brauner@kernel.org> wrote: > > > > On Sun, Feb 04, 2024 at 02:17:37AM +0000, Al Viro wrote: > > > ->permission(), ->get_link() and ->inode_get_acl() might dereference > > > ->s_fs_info (and, in case of ->permission(), ->s_fs_info->fc->user_ns > > > as well) when called from rcu pathwalk. > > > > > > Freeing ->s_fs_info->fc is rcu-delayed; we need to make freeing ->s_fs_info > > > and dropping ->user_ns rcu-delayed too. > > > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > > --- > > > > Reviewed-by: Christian Brauner <brauner@kernel.org> > > Acked-by: Miklos Szeredi <mszeredi@redhat.com> > Miklos, FYI, this is now merged and conflicts with: dc076c73b9f9 ("fuse: implement ioctls to manage backing files") from fuse/for-next: --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@@ -1373,7 -1398,9 +1405,13 @@@ EXPORT_SYMBOL_GPL(fuse_send_init) void fuse_free_conn(struct fuse_conn *fc) { WARN_ON(!list_empty(&fc->devices)); ++<<<<<<< HEAD + kfree(fc); ++======= + if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH)) + fuse_backing_files_free(fc); + kfree_rcu(fc, rcu); ++>>>>>>> fuse/for-next } EXPORT_SYMBOL_GPL(fuse_free_conn); Note that fuse_backing_files_free() calls fuse_backing_id_free() => fuse_backing_free() => kfree_rcu() Should we move fuse_backing_files_free() into fuse_conn_put() above fuse_dax_conn_free()? That will avoid the merge conflict and still be correct. no? Thanks, Amir.
On Mon, 4 Mar 2024 at 15:36, Amir Goldstein <amir73il@gmail.com> wrote: > Note that fuse_backing_files_free() calls > fuse_backing_id_free() => fuse_backing_free() => kfree_rcu() > > Should we move fuse_backing_files_free() into > fuse_conn_put() above fuse_dax_conn_free()? > > That will avoid the merge conflict and still be correct. no? Looks like a good cleanup. Force-pushed to fuse.git#for-next. Thanks, Miklos
On Tue, Mar 5, 2024 at 2:43 PM Miklos Szeredi <miklos@szeredi.hu> wrote: > > On Mon, 4 Mar 2024 at 15:36, Amir Goldstein <amir73il@gmail.com> wrote: > > > Note that fuse_backing_files_free() calls > > fuse_backing_id_free() => fuse_backing_free() => kfree_rcu() > > > > Should we move fuse_backing_files_free() into > > fuse_conn_put() above fuse_dax_conn_free()? > > > > That will avoid the merge conflict and still be correct. no? > > Looks like a good cleanup. > > Force-pushed to fuse.git#for-next. > FYI, the version that you pushed will generate a minor conflict with } - fc->release(fc); + call_rcu(&fc->rcu, delayed_release); } } EXPORT_SYMBOL_GPL(fuse_conn_put); If you move fuse_backing_files_free() to the start of the function, I think merge conflict will be avoided: void fuse_conn_put(struct fuse_conn *fc) { if (refcount_dec_and_test(&fc->count)) { struct fuse_iqueue *fiq = &fc->iq; struct fuse_sync_bucket *bucket; + if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH)) + fuse_backing_files_free(fc); if (IS_ENABLED(CONFIG_FUSE_DAX)) fuse_dax_conn_free(fc); Thanks, Amir.
On Wed, 6 Mar 2024 at 11:18, Amir Goldstein <amir73il@gmail.com> wrote: > If you move fuse_backing_files_free() to the start of the function, > I think merge conflict will be avoided: Yeah, but I don't think it's worth messing with this just to avoid a conflict. Thanks, Miklos
diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c index 91e89e68177e..b6cad106c37e 100644 --- a/fs/fuse/cuse.c +++ b/fs/fuse/cuse.c @@ -474,8 +474,7 @@ static int cuse_send_init(struct cuse_conn *cc) static void cuse_fc_release(struct fuse_conn *fc) { - struct cuse_conn *cc = fc_to_cc(fc); - kfree_rcu(cc, fc.rcu); + kfree(fc_to_cc(fc)); } /** diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index 1df83eebda92..bcbe34488862 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -888,6 +888,7 @@ struct fuse_mount { /* Entry on fc->mounts */ struct list_head fc_entry; + struct rcu_head rcu; }; static inline struct fuse_mount *get_fuse_mount_super(struct super_block *sb) diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c index 2a6d44f91729..516ea2979a90 100644 --- a/fs/fuse/inode.c +++ b/fs/fuse/inode.c @@ -930,6 +930,14 @@ void fuse_conn_init(struct fuse_conn *fc, struct fuse_mount *fm, } EXPORT_SYMBOL_GPL(fuse_conn_init); +static void delayed_release(struct rcu_head *p) +{ + struct fuse_conn *fc = container_of(p, struct fuse_conn, rcu); + + put_user_ns(fc->user_ns); + fc->release(fc); +} + void fuse_conn_put(struct fuse_conn *fc) { if (refcount_dec_and_test(&fc->count)) { @@ -941,13 +949,12 @@ void fuse_conn_put(struct fuse_conn *fc) if (fiq->ops->release) fiq->ops->release(fiq); put_pid_ns(fc->pid_ns); - put_user_ns(fc->user_ns); bucket = rcu_dereference_protected(fc->curr_bucket, 1); if (bucket) { WARN_ON(atomic_read(&bucket->count) != 1); kfree(bucket); } - fc->release(fc); + call_rcu(&fc->rcu, delayed_release); } } EXPORT_SYMBOL_GPL(fuse_conn_put); @@ -1366,7 +1373,7 @@ EXPORT_SYMBOL_GPL(fuse_send_init); void fuse_free_conn(struct fuse_conn *fc) { WARN_ON(!list_empty(&fc->devices)); - kfree_rcu(fc, rcu); + kfree(fc); } EXPORT_SYMBOL_GPL(fuse_free_conn); @@ -1902,7 +1909,7 @@ static void fuse_sb_destroy(struct super_block *sb) void fuse_mount_destroy(struct fuse_mount *fm) { fuse_conn_put(fm->fc); - kfree(fm); + kfree_rcu(fm, rcu); } EXPORT_SYMBOL(fuse_mount_destroy);
->permission(), ->get_link() and ->inode_get_acl() might dereference ->s_fs_info (and, in case of ->permission(), ->s_fs_info->fc->user_ns as well) when called from rcu pathwalk. Freeing ->s_fs_info->fc is rcu-delayed; we need to make freeing ->s_fs_info and dropping ->user_ns rcu-delayed too. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/fuse/cuse.c | 3 +-- fs/fuse/fuse_i.h | 1 + fs/fuse/inode.c | 15 +++++++++++---- 3 files changed, 13 insertions(+), 6 deletions(-)