On 24/04/10 12:16PM, Christian Brauner wrote:
> On Tue, Apr 09, 2024 at 06:31:44PM -0500, John Groves wrote:
> > The ref vs. value logic used by sget_dev() was ungood, storing the
> > stack address of the key (dev_t) rather than the value of the key.
> > This straightens that out.
> >
> > In the sget_dev() path, the (void *)data passed to the test and set
> > helpers should be the value of the dev_t, not its address.
> >
> > Signed-off-by: John Groves <john@xxxxxxxxxx>
> > ---
>
> Afaict there's nothing wrong with the current logic so I'm missing your
> point here. It's casting to a dev_t and then dereferencing it. So I
> don't think this patch makes sense.
Hi Christian,
Apologies, I got confused myself and fubar'd this.
But I believe there is at least one actual problem; please correct
me if I'm wrong, and thanks for your patience if so.
In sget_dev() - original here:
struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
{
fc->sget_key = &dev;
return sget_fc(fc, super_s_dev_test, super_s_dev_set);
}
I don't think &dev makes sense here - it was passed by value so its
address won't make sense outside the current context, right?. It seems
like it should be:
fc->sget_key = (void *)dev;
But that assumes we're using the value of sget_key rather than what
it points to, which I now see is not the case - super_s_dev_test()
is testing for (s->s_dev == *(dev_t *)fc->sget_key), so that wants
sget_key to be a pointer to a dev_t.
But I don't see anywhere that sget_key points to something that was
allocated; the dev_t for sget_dev() appears to be the only user and it's
not something whose address can validly be stored in and dereferenced
from a pointer later (am I wrong?!).
I looked at this because I tried to use sget_dev() in famfs, for which
it seems to do the right thing (although the dev_t is character in
the famfs case). But it never found the existing superblock even when
I knew it existed - so I dug myself a little hole ;)
Although my hacks went off the rails, I was sort-of trying to make
sget_key a value rather than a pointer, because the pointer thing
didn't make sense to me (at least for the dev_t case). It seems like
that should be a value unless you have other uses in mind that need
it to actually point somewhere.
I can try again with this additional clarity, but the "key" question
is whether sget_key really needs to be a pointer - which depends on
what else you want to use it for. Type checking would certainly be
easier if it wasn't a void *...
Thank you,
John
