* Re: [RFC] configfs_unregister_group() API
2019-07-30 21:13 [RFC] configfs_unregister_group() API Al Viro
@ 2019-08-02 3:35 ` Al Viro
2019-08-02 6:52 ` Christoph Hellwig
1 sibling, 0 replies; 3+ messages in thread
From: Al Viro @ 2019-08-02 3:35 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-fsdevel, Kishon Vijay Abraham I
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?
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [RFC] configfs_unregister_group() API
2019-07-30 21:13 [RFC] configfs_unregister_group() API Al Viro
2019-08-02 3:35 ` Al Viro
@ 2019-08-02 6:52 ` Christoph Hellwig
1 sibling, 0 replies; 3+ messages in thread
From: Christoph Hellwig @ 2019-08-02 6:52 UTC (permalink / raw)
To: Al Viro; +Cc: Christoph Hellwig, linux-fsdevel, Kishon Vijay Abraham I
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.
^ permalink raw reply [flat|nested] 3+ messages in thread