On 5/31/2021 1:49 PM, Vlastimil Babka wrote: > On 5/31/21 9:11 AM, Faiyaz Mohammed wrote: >> >> >> On 5/26/2021 5:43 PM, Vlastimil Babka wrote: >>> On 5/26/21 1:48 PM, Greg KH wrote: >>>> On Wed, May 26, 2021 at 01:38:55PM +0200, Vlastimil Babka wrote: >>>>> >>>>> alias_list a single list and both slab_sysfs_init() and slab_debugfs_init() >>>>> flush it. So only the init call that happens to be called first, does actually >>>>> find an unflushed list. I think you >>>>> need to use a separate list for debugfs (simpler) or a shared list with both >>>>> sysfs and debugfs processing (probably more complicated). >>>>> >>>>> And finally a question, perhaps also for Greg. With sysfs, we hand out the >>>>> lifecycle of struct kmem_cache to sysfs, to ensure we are not reading sysfs >>>>> files of a cache that has been removed. >>>>> >>>>> But with debugfs, what are the guarantees that things won't blow up when a >>>>> debugfs file is being read while somebody calls kmem_cache_destroy() on the cache? >>>> >>>> It's much harder, but usually the default debugfs_file_create() will >>>> handle this for you. See the debugfs_file_create_unsafe() for the >>>> "other" variant where you know you can tear things down "safely". >>> >>> Right, so IIUC debugfs will guarantee that while somebody reads the files, the >>> debugfs cleanup will block, as debugfs_file_get() comment explains. >>> >>> In that case I think we have the cleanup order wrong in this patch: >>> >>> shutdown_cache() should first do debugfs_slab_release() (which would block) and >>> only then proceed with slab_kmem_cache_release() which destroys the fundamental >>> structures such as kmem_cache_node, which are also accessed by the debugfs file >>> handlers. >>> >> If user is trying to read the data during shutdown_cache(), then I think >> it's possible user will get empty data, to avoid that we can call > > Empty data is fine, when the cache is going away anyway. > >> debugfs_slab_release() first and then do other stuff in shutdown_cache(). > > Everything above list_del(&s->list) should be OK, it's equivalent to normal > cache operations which the debugfs files must cope with anyway. > list_del(&s->list) is OK as the debugfs handlers don't go through the list. It's > slab_kmem_cache_release() that matters. > okay, I will move debugfs_slab_release() before the slab_kmem_cache_release() in next patch version. >>>> That being said, yes there are still issues in this area, be careful >>>> about what tools you expect to be constantly hitting debugfs files. >>> >>> FWIW, the files are accessible only to root. >>> >>>> thanks, >>>> >>>> greg k-h >>>> >>> >> >
