Message-ID: <aNSmO8iou5dJ1VHq@codewreck.org>
Date: Thu, 25 Sep 2025 11:17:31 +0900
From: Dominique Martinet <asmadeus@...ewreck.org>
To: Eric Sandeen <sandeen@...hat.com>
Cc: Edward Adam Davis <eadavis@...com>, ericvh@...nel.org,
	linux-kernel@...r.kernel.org, linux_oss@...debyte.com,
	lucho@...kov.net,
	syzbot+30c83da54e948f6e9436@...kaller.appspotmail.com,
	syzkaller-bugs@...glegroups.com, v9fs@...ts.linux.dev
Subject: Re: [PATCH next V2] 9p: Correct the session info

Eric Sandeen wrote on Wed, Sep 24, 2025 at 05:19:11PM -0500:
> > What I'm not following now is how the v9ses is created/handled around
> > the new mount API:
> > - in v9fs_get_tree a v9ses is allocated and passed along in
> > fc->s_fs_info (that this patches now uses)
> 
> This one should exist for the lifetime of the mount, and is the real,
> live session info.
> 
> > - but in v9fs_init_fs_context then a `v9fs_context` is created that
> > also embeds (not a pointer) a v9ses struct, which is accessed through
> > fc->fs_private as the code before this patch.
> 
> This one should be ephemeral, used to hold options during mount/remount.
> (see the patch description for
> "9p: create a v9fs_context structure to hold parsed options")
> 
> The idea is that the v9ses within the v9fs_context is used to hold
> state/options while parsing or reconfiguring. Its lifetime is only during
> mount/remount, and freed in v9fs_free_fc when that is done.
> 
> Before it is freed, at the end of mount/remount, the values stored in it
> are copied into the "live" v9ses in v9fs_apply_options().

Thanks for clarifying this;

I had which is a pointer and which is embedded wrong on the first
reading so I first wrote that this should be ok if we could just null
the pointer in fs_private to avoid access after the copy, but the
temporary values are the embedded ones we can't "unaccess"...
Hmm, do we need anything else in fs_private after v9fs_apply_options()?
Would it make sense to call v9fs_free_fc() early on and null the full
fs_private in or after v9fs_apply_options()?

Using a different struct tailored for mount options is certainly more
appropriate if you have the time to do it, but as long as the risk of
accessing "the wrong one" goes away I'm fine either way, so if you think
nulling fs_private is possible without too much churn I think that's
good enough.

What do you think?



> >> diff --git a/fs/9p/vfs_super.c b/fs/9p/vfs_super.c
> >> index f6065b5e0e5d..bcb6ebdb8037 100644
> >> --- a/fs/9p/vfs_super.c
> >> +++ b/fs/9p/vfs_super.c
> >> @@ -49,8 +49,7 @@ static int v9fs_set_super(struct super_block *s, struct fs_context *fc)
> >>  static int v9fs_fill_super(struct super_block *sb, struct fs_context *fc)
> >>  {
> >>  	int ret;
> >> -	struct v9fs_context	*ctx = fc->fs_private;
> >> -	struct v9fs_session_info *v9ses = &ctx->v9ses;
> >> +	struct v9fs_session_info *v9ses = sb->s_fs_info;
> 
> This seems correct (though we can get rid of *fc too right?) 

(Sounds right as nothing else seems to use fc)

> To get here (v9fs_fill_super), we've gone through:
> 
> v9fs_get_tree(fc)
> 	// this will be our real live v9ses for the mount
> 	v9ses = kzalloc(sizeof(struct v9fs_session_info), GFP_KERNEL);
> 
>         v9fs_session_init(v9ses, fc)
> 		// copies fc/context options in context v9ses
> 		// into the "real" v9ses
>                 v9fs_apply_options(v9ses, fc)
> 		// inits maxdata on the real, live v9ses
> 		v9ses->maxdata = v9ses->clnt->msize - P9_IOHDRSZ;
> 
> 	fc->s_fs_info = v9ses;	// real, live v9ses on fc->s_fs_info now.
>         sb = sget_fc(fc, NULL, v9fs_set_super);
> 		s->s_fs_info = fc->s_fs_info; // live v9ses on s->fs_info now
>                 err = set(s, fc)
> 			v9fs_set_super(s, fc)
> 				// assigns s->s_fs_info to fc->s_fs_info
> 				// but I think this is redundant
> 				// and should be removed

My reading is that we need it, because the super block isn't the fs
context, and we need it for v9fs_umount_begin (it doesn't help that the
field name is the same between both structs, and that some super_block
variables are just 's' and others are 'sb', but I think that's the only
place where it's used)

At this point these are both the "real live" v9ses so that should be
fine as far as I can see.

> 				*v9ses = fc->s_fs_info;
> 	                        s->s_fs_info = v9ses;
> 				return set_anon_super(s, NULL);
> 
>         v9fs_fill_super(sb, fc)
> 
> and yes, with my patch were were using the option-holding "v9ses" here,
> not the actual, fully initialized, legitimate v9ses for this mount, and
> because v9ses->maxdata is not something that gets options parsed into it,
> it was unset, and it so hit this bug. :(
> 
> Going back to the prior explanation of the fc context, it's probably
> a better idea to not re-use the v9ses struct "for convenience" and
> make a new dedicated structure just for option parsing, so unset
> structure members aren't even present and therefore inaccessible.
> I'm sorry for the mistake.
> 
> I should spin up a new version of the series fixing this and the
> uint32/int32 difference you spotted as well

That would be great!

I'm not sure the unsigned makes much different ultimately, but might as
well make it a fsparam_s32 as you suggested...
Now I understand the lifecycle a bit better I can have another read with
that in mind before merging, and we can do the nulling fs_private or
other "make sure this bug doesn't come back" later if you don't have
time, I'm leaving this up to you.

> and credit Edward for his fix in the new commit log somehow?

I've never really been sure what kind of tag is appropriate in this
case, but I assume any mention would be welcome :)


Thanks,
-- 
Dominique Martinet | Asmadeus

Powered by blists - more mailing lists