[RFC] configfs_{un,}register_group() semantics

[RFC] configfs_{un,}register_group() semantics

[Al Viro]

	Folks, could you confirm if the following is correct?

1. configfs_unregister_group() callers are supposed to prevent
having it called when some items/groups had been created under it.
The original one (in iio) *does* prevent that (the call chains come
through the module_exit() of modules pinned by ->make_group() for
the added subdirectory), but I don't see that documented anywhere and
AFAICS at least in one case (drivers/pci/endpoint/pci-ep-cfs.c) that is
not guaranteed.  The same goes for symlinks created in or to those.

2. rmdir on directory added by configfs_register_group() is supposed to
fail (is it even supposed to be used inside the stuff created by mkdir?
Original use was inside a subsystem, AFAICS).

3. rmdir that would've taken out the parent group is supposed to take
the added one out (again, are they even supposed to be used inside the
stuff created by mkdir?)

4. one is *NOT* supposed to use have ->make_group() schedule creation of
subdirectories via configfs_register_group(); configfs_add_default_group()
ought to be used instead.

drivers/pci/endpoint/pci-ep-cfs.c:pci_epf_make() has this:
        INIT_DELAYED_WORK(&epf_group->cfs_work, pci_epf_cfs_work);
        queue_delayed_work(system_wq, &epf_group->cfs_work,
                           msecs_to_jiffies(1));

        return &epf_group->group;

with pci_epf_cfs_work() allocating several config_group and calling
configfs_register_group() to link those in.  I really doubt that this
kind of "let's hope that configfs_mkdir() will get through directory
creation in less than 1ms after ->make_group() returns" is the way it
is supposed to be done; at a guess, configfs_add_default_group()
should've been used (synchronously), but I might be missing something
subtle here.

Comments?

Re: [RFC] configfs_{un,}register_group() semantics

[Al Viro]

[now with hopefully correct address of Daniel Baluta]

On Sun, May 12, 2024 at 05:30:12AM +0100, Al Viro wrote:
> 	Folks, could you confirm if the following is correct?
> 
> 1. configfs_unregister_group() callers are supposed to prevent
> having it called when some items/groups had been created under it.
> The original one (in iio) *does* prevent that (the call chains come
> through the module_exit() of modules pinned by ->make_group() for
> the added subdirectory), but I don't see that documented anywhere and
> AFAICS at least in one case (drivers/pci/endpoint/pci-ep-cfs.c) that is
> not guaranteed.  The same goes for symlinks created in or to those.
> 
> 2. rmdir on directory added by configfs_register_group() is supposed to
> fail (is it even supposed to be used inside the stuff created by mkdir?
> Original use was inside a subsystem, AFAICS).
> 
> 3. rmdir that would've taken out the parent group is supposed to take
> the added one out (again, are they even supposed to be used inside the
> stuff created by mkdir?)
> 
> 4. one is *NOT* supposed to use have ->make_group() schedule creation of
> subdirectories via configfs_register_group(); configfs_add_default_group()
> ought to be used instead.
> 
> drivers/pci/endpoint/pci-ep-cfs.c:pci_epf_make() has this:
>         INIT_DELAYED_WORK(&epf_group->cfs_work, pci_epf_cfs_work);
>         queue_delayed_work(system_wq, &epf_group->cfs_work,
>                            msecs_to_jiffies(1));
> 
>         return &epf_group->group;
> 
> with pci_epf_cfs_work() allocating several config_group and calling
> configfs_register_group() to link those in.  I really doubt that this
> kind of "let's hope that configfs_mkdir() will get through directory
> creation in less than 1ms after ->make_group() returns" is the way it
> is supposed to be done; at a guess, configfs_add_default_group()
> should've been used (synchronously), but I might be missing something
> subtle here.
> 
> Comments?
> 

Re: [RFC] configfs_{un,}register_group() semantics

[Joel Becker]

On Sun, May 12, 2024 at 05:35:09AM +0100, Al Viro wrote:
> [now with hopefully correct address of Daniel Baluta]
> 
> On Sun, May 12, 2024 at 05:30:12AM +0100, Al Viro wrote:
> > 	Folks, could you confirm if the following is correct?

Recovering state on the dynamic group code...

> > 
> > 1. configfs_unregister_group() callers are supposed to prevent
> > having it called when some items/groups had been created under it.
> > The original one (in iio) *does* prevent that (the call chains come
> > through the module_exit() of modules pinned by ->make_group() for
> > the added subdirectory), but I don't see that documented anywhere and
> > AFAICS at least in one case (drivers/pci/endpoint/pci-ep-cfs.c) that is
> > not guaranteed.  The same goes for symlinks created in or to those.

I would expect configfs_unregister_group() to fail if there are
items/groups created underneath it.  Specifically, in all of the
rmdir/unregister paths, configfs_detach_prep() is responsible for
verifying that the subtree only contains default/generated entitities.
Looking at the code, I notice that configfs_detach_prep() is called
from configfs_unregister_group() without checking the return code.
This means the prep can be left in a partial state (only some parts of
the subtree have CONFIGFS_USET_DROPPING still set), and
configfs_detach_rollback() is also not called to clean that up.

Naively, I'd prefer the operations correctly fail (-ENOTEMPTY if coming
from a user's rmdir(2) on a parent that the user had initially created,
BUG_ON for kernel coding errors).

> > 
> > 2. rmdir on directory added by configfs_register_group() is supposed to
> > fail (is it even supposed to be used inside the stuff created by mkdir?
> > Original use was inside a subsystem, AFAICS).

That is correct, rmdir(2) on directories created by the machinery is
supposed to fail.  It checks for CONFIGFS_USET_DEFAULT and returns
EPERM.  This should be working -- configfs_register_group() calls
create_default_group(), which sets CONFIGFS_USET_DEFAULT.

> > 
> > 3. rmdir that would've taken out the parent group is supposed to take
> > the added one out (again, are they even supposed to be used inside the
> > stuff created by mkdir?)

Yes, if the user does `mkdir pathA`, and the default/registered group
structure automatically builds pathA/{subpathB,subpathC}, then `rmdir
pathA`

> > 
> > 4. one is *NOT* supposed to use have ->make_group() schedule creation of
> > subdirectories via configfs_register_group(); configfs_add_default_group()
> > ought to be used instead.

I presume you mean configfs_register_default_group().  My general
thoughts would be to agree, that sub-groups should be configured via
registered default groups, not added in the caller's code, because that
allows configfs to control the lifecycle.

But in that case, I don't remember exactly why configfs_register_group()
is exported at all.  Daniel, what was the distinction between using
default groups vs creating them in the client code?

> > 
> > drivers/pci/endpoint/pci-ep-cfs.c:pci_epf_make() has this:
> >         INIT_DELAYED_WORK(&epf_group->cfs_work, pci_epf_cfs_work);
> >         queue_delayed_work(system_wq, &epf_group->cfs_work,
> >                            msecs_to_jiffies(1));
> > 
> >         return &epf_group->group;
> > 
> > with pci_epf_cfs_work() allocating several config_group and calling
> > configfs_register_group() to link those in.  I really doubt that this
> > kind of "let's hope that configfs_mkdir() will get through directory
> > creation in less than 1ms after ->make_group() returns" is the way it
> > is supposed to be done; at a guess, configfs_add_default_group()
> > should've been used (synchronously), but I might be missing something
> > subtle here.

The creation and linkage needs to happen under the locking and lifecycle
of the configfs tree.  I don't see how one could safely drop it on a
workqueue and not violate either code safety (doing it outside the
correct locks or CONFIGFS_USET_XXX state markers) or atomicity (a period
when userspace can see the parent group but not the child groups).

Even if we presume the client doing an explicit
create_group()+configfs_register_group() is the right method, rather
than registering a default group and letting that machinery do the
work, the client's operation needs to be completed under the make_grup()
call.  This would imply synchronously.

Thanks,
Joel

-- 

Life's Little Instruction Book #252

	"Take good care of those you love."

			http://www.jlbec.org/
			jlbec@evilplan.org