On 14/11/23 12:41, Al Viro wrote:
On Tue, Nov 14, 2023 at 12:25:35PM +0800, Ian Kent wrote:
root_inode = autofs_get_inode(s, S_IFDIR | 0755);
+ if (!root_inode)
+ goto fail;
Yes, I think this is the only thing it could be.
There's one small problem though, it leaks the dentry info. ino,
allocated just above. I think this should goto label fail_ino instead.
Note that once the root dentry is allocated then the ino struct will
be freed when the dentry is freed so ino doesn't need to be freed.
There's a simpler solution:
root_inode = autofs_get_inode(s, S_IFDIR | 0755);
if (root_inode) {
root_inode->i_uid = ctx->uid;
root_inode->i_gid = ctx->gid;
}
root = d_make_root(root_inode);
if (!root)
goto fail_ino;
d_make_root(NULL) will quietly return NULL, which is all you
need. FWIW, I would probably bring the rest of initialization
root_inode->i_fop = &autofs_root_operations;
root_inode->i_op = &autofs_dir_inode_operations;
in there as well.
Incidentally, why bother with separate fail_dput thing? Shove it
into ->s_root and return ret - autofs_kill_sb() will take care
of dropping it...
How about the following:
Yes, I think so, AFAICS so far it looks like everything is covered.
I'll look around a bit longer, I need to go over that mount API code
again anyway ...
I'll prepare a patch, the main thing that I was concerned about was
whether the cause really was NULL root_inode but Edward more or less
tested that.
Ian
static int autofs_fill_super(struct super_block *s, struct fs_context *fc)
{
struct autofs_fs_context *ctx = fc->fs_private;
struct autofs_sb_info *sbi = s->s_fs_info;
struct inode *root_inode;
struct autofs_info *ino;
pr_debug("starting up, sbi = %p\n", sbi);
sbi->sb = s;
s->s_blocksize = 1024;
s->s_blocksize_bits = 10;
s->s_magic = AUTOFS_SUPER_MAGIC;
s->s_op = &autofs_sops;
s->s_d_op = &autofs_dentry_operations;
s->s_time_gran = 1;
/*
* Get the root inode and dentry, but defer checking for errors.
*/
ino = autofs_new_ino(sbi);
if (!ino)
return -ENOMEM;
root_inode = autofs_get_inode(s, S_IFDIR | 0755);
if (root_inode) {
root_inode->i_uid = ctx->uid;
root_inode->i_gid = ctx->gid;
root_inode->i_fop = &autofs_root_operations;
root_inode->i_op = &autofs_dir_inode_operations;
}
s->s_root = d_make_root(root_inode);
if (unlikely(!s->s_root)) {
autofs_free_ino(ino);
return -ENOMEM;
}
s->s_root->d_fsdata = ino;
if (ctx->pgrp_set) {
sbi->oz_pgrp = find_get_pid(ctx->pgrp);
if (!sbi->oz_pgrp) {
int ret = invalf(fc, "Could not find process group %d",
ctx->pgrp);
return ret;
}
} else {
sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
}
if (autofs_type_trigger(sbi->type))
managed_dentry_set_managed(s->s_root);
pr_debug("pipe fd = %d, pgrp = %u\n",
sbi->pipefd, pid_nr(sbi->oz_pgrp));
sbi->flags &= ~AUTOFS_SBI_CATATONIC;
return 0;
}
No gotos, no labels to keep track of...
