On Wed, Sep 13, 2023 at 08:09:59AM -0300, Christoph Hellwig wrote:
> diff --git a/fs/super.c b/fs/super.c
> index bbe55f0651cca4..5c685b4944c2d6 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -787,7 +787,7 @@ struct super_block *sget_fc(struct fs_context *fc,
> struct super_block *s = NULL;
> struct super_block *old;
> struct user_namespace *user_ns = fc->global ? &init_user_ns : fc->user_ns;
> - int err;
> + int err = 0;
>
> retry:
> spin_lock(&sb_lock);
> @@ -806,14 +806,26 @@ struct super_block *sget_fc(struct fs_context *fc,
> }
>
> s->s_fs_info = fc->s_fs_info;
> - err = set(s, fc);
> - if (err) {
> - s->s_fs_info = NULL;
> - spin_unlock(&sb_lock);
> - destroy_unused_super(s);
> - return ERR_PTR(err);
> + if (set) {
> + err = set(s, fc);
> + if (err) {
> + s->s_fs_info = NULL;
Pointless (as the original had been); destroy_unused_super() doesn't
even look at ->s_fs_info.
> + goto unlock_and_destroy;
> + }
> }
> fc->s_fs_info = NULL;
Here we are transferring the ownership from fc to superblock; it used to sit
right next to insertion into lists and all failure exits past that point must
go through deactivate_locked_super(), so ->kill_sb() would be called on those
and it would take care of s->s_fs_info. However, your variant has that
ownership transfer done at the point before get_anon_bdev(), and that got
you a new failure exit where you are still calling destroy_unused_super():
> + if (!s->s_dev) {
> + /*
> + * If the file system didn't set a s_dev (which is usually only
> + * done for block based file systems), set an anonymous dev_t
> + * here now so that we always have a valid ->s_dev.
> + */
> + err = get_anon_bdev(&s->s_dev);
> + if (err)
> + goto unlock_and_destroy;
This. And that's a leak - fc has no reference left in it, and your
unlock_and_destroy won't even look at what's in ->s_fs_info, let alone know
what to do with it.
IOW, clearing fc->s_fs_info should've been done after that chunk.
And looking at the change in sget(),
> + if (set) {
> + err = set(s, data);
> + if (err)
> + goto unlock_and_destroy;
> }
> +
> + if (!s->s_dev) {
> + err = get_anon_bdev(&s->s_dev);
> + if (err)
> + goto unlock_and_destroy;
> + }
I'd rather expressed it (both there and in sget_fc() as well) as
if (set)
err = set(s, data);
if (!err && !s->s_dev)
err = get_anon_bdev(&s->s_dev);
if (err)
goto unlock_and_destroy;
That's really what your transformation does - you are lifting the
calls of get_anon_bdev() (in guise of set_anon_super()) from the
tails of 'set' callbacks into the caller, making them conditional
upon the lack of other errors from 'set' and upon the ->s_dev left
zero and allow NULL for the case when that was all that had been
there.
The only place where you do something different is this:
> @@ -1191,7 +1191,6 @@ static struct dentry *ceph_real_mount(struct ceph_fs_client *fsc,
> static int ceph_set_super(struct super_block *s, struct fs_context *fc)
> {
> struct ceph_fs_client *fsc = s->s_fs_info;
> - int ret;
>
> dout("set_super %p\n", s);
>
> @@ -1211,11 +1210,7 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)
> s->s_flags |= SB_NODIRATIME | SB_NOATIME;
>
> ceph_fscrypt_set_ops(s);
> -
> - ret = set_anon_super_fc(s, fc);
> - if (ret != 0)
> - fsc->sb = NULL;
> - return ret;
> + return 0;
fsc->sb = NULL has disappeared here; it *is* OK (the caller won't look at
fsc->sb after failed sget_fc()), but that's worth a mention somewhere.
A separate commit removing that clearing fsc->sb in ceph_set_super(),
perhaps?
@@ -40,7 +40,7 @@ static const struct super_operations v9fs_super_ops, v9fs_super_ops_dotl;
static int v9fs_set_super(struct super_block *s, void *data)
{
s->s_fs_info = data;
- return set_anon_super(s, data);
+ return 0;
}
/**
@@ -435,11 +435,6 @@ static int afs_dynroot_test_super(struct super_block *sb, struct fs_context *fc)
as->dyn_root);
}
-static int afs_set_super(struct super_block *sb, struct fs_context *fc)
-{
- return set_anon_super(sb, NULL);
-}
-
/*
* fill in the superblock
*/
@@ -574,9 +569,10 @@ static int afs_get_tree(struct fs_context *fc)
fc->s_fs_info = as;
/* allocate a deviceless superblock */
- sb = sget_fc(fc,
- as->dyn_root ? afs_dynroot_test_super : afs_test_super,
- afs_set_super);
+ if (as->dyn_root)
+ sb = sget_fc(fc, afs_dynroot_test_super, NULL);
+ else
+ sb = sget_fc(fc, afs_test_super, NULL);
if (IS_ERR(sb)) {
ret = PTR_ERR(sb);
goto error;
@@ -1350,10 +1350,8 @@ static int btrfs_test_super(struct super_block *s, void *data)
static int btrfs_set_super(struct super_block *s, void *data)
{
- int err = set_anon_super(s, data);
- if (!err)
- s->s_fs_info = data;
- return err;
+ s->s_fs_info = data;
+ return 0;
}
/*
@@ -1191,7 +1191,6 @@ static struct dentry *ceph_real_mount(struct ceph_fs_client *fsc,
static int ceph_set_super(struct super_block *s, struct fs_context *fc)
{
struct ceph_fs_client *fsc = s->s_fs_info;
- int ret;
dout("set_super %p\n", s);
@@ -1211,11 +1210,7 @@ static int ceph_set_super(struct super_block *s, struct fs_context *fc)
s->s_flags |= SB_NODIRATIME | SB_NOATIME;
ceph_fscrypt_set_ops(s);
-
- ret = set_anon_super_fc(s, fc);
- if (ret != 0)
- fsc->sb = NULL;
- return ret;
+ return 0;
}
/*
@@ -505,7 +505,7 @@ static struct dentry *ecryptfs_mount(struct file_system_type *fs_type, int flags
}
mount_crypt_stat = &sbi->mount_crypt_stat;
- s = sget(fs_type, NULL, set_anon_super, flags, NULL);
+ s = sget(fs_type, NULL, NULL, flags, NULL);
if (IS_ERR(s)) {
rc = PTR_ERR(s);
goto out;
@@ -1511,7 +1511,7 @@ static int fuse_get_tree_submount(struct fs_context *fsc)
fm->fc = fuse_conn_get(fc);
fsc->s_fs_info = fm;
- sb = sget_fc(fsc, NULL, set_anon_super_fc);
+ sb = sget_fc(fsc, NULL, NULL);
if (fsc->s_fs_info)
fuse_mount_destroy(fm);
if (IS_ERR(sb))
@@ -1454,7 +1454,7 @@ static int virtio_fs_get_tree(struct fs_context *fsc)
virtqueue_size - FUSE_HEADER_OVERHEAD);
fsc->s_fs_info = fm;
- sb = sget_fc(fsc, virtio_fs_test_super, set_anon_super_fc);
+ sb = sget_fc(fsc, virtio_fs_test_super, NULL);
if (fsc->s_fs_info)
fuse_mount_destroy(fm);
if (IS_ERR(sb))
@@ -300,7 +300,7 @@ static int kernfs_set_super(struct super_block *sb, struct fs_context *fc)
struct kernfs_fs_context *kfc = fc->fs_private;
kfc->ns_tag = NULL;
- return set_anon_super_fc(sb, fc);
+ return 0;
}
/**
@@ -1132,7 +1132,7 @@ static int nfs_set_super(struct super_block *s, struct fs_context *fc)
struct nfs_server *server = fc->s_fs_info;
s->s_d_op = server->nfs_client->rpc_ops->dentry_ops;
- return set_anon_super(s, server);
+ return 0;
}
static int nfs_compare_super_address(struct nfs_server *server1,
@@ -514,7 +514,7 @@ struct dentry *orangefs_mount(struct file_system_type *fst,
goto free_op;
}
- sb = sget(fst, NULL, set_anon_super, flags, NULL);
+ sb = sget(fst, NULL, NULL, flags, NULL);
if (IS_ERR(sb)) {
d = ERR_CAST(sb);
@@ -884,8 +884,9 @@ cifs_get_root(struct smb3_fs_context *ctx, struct super_block *sb)
static int cifs_set_super(struct super_block *sb, void *data)
{
struct cifs_mnt_data *mnt_data = data;
+
sb->s_fs_info = mnt_data->cifs_sb;
- return set_anon_super(sb, NULL);
+ return 0;
}
struct dentry *
@@ -787,7 +787,7 @@ struct super_block *sget_fc(struct fs_context *fc,
struct super_block *s = NULL;
struct super_block *old;
struct user_namespace *user_ns = fc->global ? &init_user_ns : fc->user_ns;
- int err;
+ int err = 0;
retry:
spin_lock(&sb_lock);
@@ -806,14 +806,26 @@ struct super_block *sget_fc(struct fs_context *fc,
}
s->s_fs_info = fc->s_fs_info;
- err = set(s, fc);
- if (err) {
- s->s_fs_info = NULL;
- spin_unlock(&sb_lock);
- destroy_unused_super(s);
- return ERR_PTR(err);
+ if (set) {
+ err = set(s, fc);
+ if (err) {
+ s->s_fs_info = NULL;
+ goto unlock_and_destroy;
+ }
}
fc->s_fs_info = NULL;
+
+ if (!s->s_dev) {
+ /*
+ * If the file system didn't set a s_dev (which is usually only
+ * done for block based file systems), set an anonymous dev_t
+ * here now so that we always have a valid ->s_dev.
+ */
+ err = get_anon_bdev(&s->s_dev);
+ if (err)
+ goto unlock_and_destroy;
+ }
+
s->s_type = fc->fs_type;
s->s_iflags |= fc->s_iflags;
strscpy(s->s_id, s->s_type->name, sizeof(s->s_id));
@@ -843,6 +855,10 @@ struct super_block *sget_fc(struct fs_context *fc,
goto retry;
destroy_unused_super(s);
return old;
+unlock_and_destroy:
+ spin_unlock(&sb_lock);
+ destroy_unused_super(s);
+ return ERR_PTR(err);
}
EXPORT_SYMBOL(sget_fc);
@@ -897,12 +913,18 @@ struct super_block *sget(struct file_system_type *type,
goto retry;
}
- err = set(s, data);
- if (err) {
- spin_unlock(&sb_lock);
- destroy_unused_super(s);
- return ERR_PTR(err);
+ if (set) {
+ err = set(s, data);
+ if (err)
+ goto unlock_and_destroy;
}
+
+ if (!s->s_dev) {
+ err = get_anon_bdev(&s->s_dev);
+ if (err)
+ goto unlock_and_destroy;
+ }
+
s->s_type = type;
strscpy(s->s_id, type->name, sizeof(s->s_id));
list_add_tail(&s->s_list, &super_blocks);
@@ -911,6 +933,10 @@ struct super_block *sget(struct file_system_type *type,
get_filesystem(type);
register_shrinker_prepared(&s->s_shrink);
return s;
+unlock_and_destroy:
+ spin_unlock(&sb_lock);
+ destroy_unused_super(s);
+ return ERR_PTR(err);
}
EXPORT_SYMBOL(sget);
@@ -1288,12 +1314,6 @@ void free_anon_bdev(dev_t dev)
}
EXPORT_SYMBOL(free_anon_bdev);
-int set_anon_super(struct super_block *s, void *data)
-{
- return get_anon_bdev(&s->s_dev);
-}
-EXPORT_SYMBOL(set_anon_super);
-
void kill_litter_super(struct super_block *sb)
{
if (sb->s_root)
@@ -1302,12 +1322,6 @@ void kill_litter_super(struct super_block *sb)
}
EXPORT_SYMBOL(kill_litter_super);
-int set_anon_super_fc(struct super_block *sb, struct fs_context *fc)
-{
- return set_anon_super(sb, NULL);
-}
-EXPORT_SYMBOL(set_anon_super_fc);
-
static int test_keyed_super(struct super_block *sb, struct fs_context *fc)
{
return sb->s_fs_info == fc->s_fs_info;
@@ -1326,7 +1340,7 @@ static int vfs_get_super(struct fs_context *fc,
struct super_block *sb;
int err;
- sb = sget_fc(fc, test, set_anon_super_fc);
+ sb = sget_fc(fc, test, NULL);
if (IS_ERR(sb))
return PTR_ERR(sb);
@@ -1657,7 +1671,7 @@ struct dentry *mount_nodev(struct file_system_type *fs_type,
int (*fill_super)(struct super_block *, void *, int))
{
int error;
- struct super_block *s = sget(fs_type, NULL, set_anon_super, flags, NULL);
+ struct super_block *s = sget(fs_type, NULL, NULL, flags, NULL);
if (IS_ERR(s))
return ERR_CAST(s);
@@ -1709,7 +1723,7 @@ struct dentry *mount_single(struct file_system_type *fs_type,
struct super_block *s;
int error;
- s = sget(fs_type, compare_single, set_anon_super, flags, NULL);
+ s = sget(fs_type, compare_single, NULL, flags, NULL);
if (IS_ERR(s))
return ERR_CAST(s);
if (!s->s_root) {
@@ -2273,7 +2273,7 @@ static int sb_test(struct super_block *sb, void *data)
static int sb_set(struct super_block *sb, void *data)
{
sb->s_fs_info = data;
- return set_anon_super(sb, NULL);
+ return 0;
}
static struct dentry *ubifs_mount(struct file_system_type *fs_type, int flags,
@@ -2385,8 +2385,6 @@ void kill_block_super(struct super_block *sb);
void kill_litter_super(struct super_block *sb);
void deactivate_super(struct super_block *sb);
void deactivate_locked_super(struct super_block *sb);
-int set_anon_super(struct super_block *s, void *data);
-int set_anon_super_fc(struct super_block *s, struct fs_context *fc);
int get_anon_bdev(dev_t *);
void free_anon_bdev(dev_t);
struct super_block *sget_fc(struct fs_context *fc,