[RFC] configfs_unregister_group() API

[RFC] configfs_unregister_group() API

[Al Viro]

	AFAICS, it (and configfs_unregister_default_group())
will break if called with group non-empty (i.e. when rmdir(2)
would've failed with -ENOTEMPTY); configfs_detach_prep()
is called, but return value is completely ignored.

	Similar breakage happens in configfs_unregister_subsystem(),
but there it looks like the drivers are responsible for not calling
it that way.  It yells if configfs_detach_prep() fails and AFAICS
all callers do guarantee it never happens.

	configfs_unregister_group() is quiet; from my reading of
the callers, only pci-endpoint might end up calling it for group
that is not guaranteed to be empty.  I'm not familiar with
pci-endpoint guts, so I might very well be missing something there.

Questions to configfs API maintainers (that'd be Christoph, these
days, AFAIK)

1) should such a call be considered a driver bug?
2) should configfs_unregister_group() at least warn when that happens?

and, to pci-endpoint maintainer

3) what, if anything, prevents such calls in pci-endpoint?  Because
as it is, configfs will break badly when that happens...

Re: [RFC] configfs_unregister_group() API

[Al Viro]

On Tue, Jul 30, 2019 at 10:13:55PM +0100, Al Viro wrote:
> 	AFAICS, it (and configfs_unregister_default_group())
> will break if called with group non-empty (i.e. when rmdir(2)
> would've failed with -ENOTEMPTY); configfs_detach_prep()
> is called, but return value is completely ignored.
> 
> 	Similar breakage happens in configfs_unregister_subsystem(),
> but there it looks like the drivers are responsible for not calling
> it that way.  It yells if configfs_detach_prep() fails and AFAICS
> all callers do guarantee it never happens.
> 
> 	configfs_unregister_group() is quiet; from my reading of
> the callers, only pci-endpoint might end up calling it for group
> that is not guaranteed to be empty.  I'm not familiar with
> pci-endpoint guts, so I might very well be missing something there.
> 
> Questions to configfs API maintainers (that'd be Christoph, these
> days, AFAIK)
> 
> 1) should such a call be considered a driver bug?
> 2) should configfs_unregister_group() at least warn when that happens?
> 
> and, to pci-endpoint maintainer
> 
> 3) what, if anything, prevents such calls in pci-endpoint?  Because
> as it is, configfs will break badly when that happens...

	More specifically, consider something like pci_epf_test_init()
calling pci_epf_register_driver().  Which, in turn, calls
pci_ep_cfs_add_epf_group() and hits
        group = configfs_register_default_group(functions_group, name,
                                                &pci_epf_group_type);
in there.  OK, so we get a directory tree created, with

static const struct config_item_type pci_epf_group_type = {
        .ct_group_ops   = &pci_epf_group_ops,
        .ct_owner       = THIS_MODULE,
};

for type.  Since pci_epf_group_ops is
static struct configfs_group_operations pci_epf_group_ops = {
        .make_group     = &pci_epf_make,
        .drop_item      = &pci_epf_drop,
};

and has ->make_group(), userland can do mkdir() in there.  Now,
doing so pins ->ct_owner, preventing module_exit() until we
rmdir() the sucker.  And configfs_default_group_unregister()
*IS* triggered by module_exit(), but it's the wrong module.
THIS_MODULE here refers to pci-ep-cfs, not pci-epf-test, so
it doesn't do a damn thing to prevent rmmod pci-epf-test,
calling
static void __exit pci_epf_test_exit(void)
{
        pci_epf_unregister_driver(&test_driver);
}
which leads to pci_ep_cfs_remove_epc_group(), with
	configfs_unregister_default_group(group);
in it.  What's to prevent that call on non-empty group?

	AFAICS, pci_ep_cfs_add_epc_group()/pci_ep_cfs_remove_epc_group()
might grow a similar problem, but these have no current users.

	Folks, should that be treated as bug in driver (as in
"don't you ever call configfs_unregister_{default_,}group() on
a non-empty group") or should that be dealt with in configfs?

Re: [RFC] configfs_unregister_group() API

[Christoph Hellwig]

On Tue, Jul 30, 2019 at 10:13:55PM +0100, Al Viro wrote:
> 	AFAICS, it (and configfs_unregister_default_group())
> will break if called with group non-empty (i.e. when rmdir(2)
> would've failed with -ENOTEMPTY); configfs_detach_prep()
> is called, but return value is completely ignored.
> 
> 	Similar breakage happens in configfs_unregister_subsystem(),
> but there it looks like the drivers are responsible for not calling
> it that way.  It yells if configfs_detach_prep() fails and AFAICS
> all callers do guarantee it never happens.
> 
> 	configfs_unregister_group() is quiet; from my reading of
> the callers, only pci-endpoint might end up calling it for group
> that is not guaranteed to be empty.  I'm not familiar with
> pci-endpoint guts, so I might very well be missing something there.
> 
> Questions to configfs API maintainers (that'd be Christoph, these
> days, AFAIK)
> 
> 1) should such a call be considered a driver bug?
> 2) should configfs_unregister_group() at least warn when that happens?

Yes, I'm patch monkeying these days.  From my POV expecting it to
act recursively seems like a bug, and we should at the very least warn.