Date: Wed, 15 May 2024 12:07:03 -0700
From: Joel Becker <jlbec@...lplan.org>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: Christoph Hellwig <hch@....de>, Daniel Baluta <daniel.baluta@....com>,
	Kishon Vijay Abraham I <kishon@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org
Subject: Re: [RFC] configfs_{un,}register_group() semantics

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@...lplan.org

Powered by blists - more mailing lists