On Tue, Jul 11, 2023 at 10:44 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > I agree. I think it was a good decision. > > > I have some followup questions though. > > > > > > I guess your use case cares about the creation of cgroups? > > > as long as the only way to create a cgroup is via vfs > > > vfs_mkdir() -> ... cgroup_mkdir() > > > fsnotify_mkdir() will be called. > > > Is that a correct statement? > > > > As far as I'm aware, this is the only way. We have the cgroups mailing > > list CC'd to confirm. > > > > I checked systemd and docker as real world consumers and both use > > mkdir and are visible in fanotify with this patch applied. > > > > > Because if not, then explicit fsnotify_mkdir() calls may be needed > > > similar to tracefs/debugfs. > > > > > > I don't think that the statement holds for dieing cgroups, > > > so explicit fsnotify_rmdir() are almost certainly needed to make > > > inotify/fanotify monitoring on cgroups complete. > > > > > > I am on the fence w.r.t making the above a prerequisite to merging > > > your patch. > > > > > > One the one hand, inotify monitoring of cgroups directory was already > > > possible (I think?) with the mentioned shortcomings for a long time. > > > > > > On the other hand, we have an opportunity to add support to fanotify > > > monitoring of cgroups directory only after the missing fsnotify hooks > > > are added, making fanotify API a much more reliable option for > > > monitoring cgroups. > > > > > > So I am leaning towards requiring the missing fsnotify hooks before > > > attaching a unique fsid to cgroups/kernfs. > > > > Unless somebody responsible for cgroups says there's a different way > > to create cgroups, I think this requirement doesn't apply. > > > > I was more concerned about the reliability of FAN_DELETE for > dieing cgroups without an explicit rmdir() from userspace. I checked the code and cgroups are destroyed in cgroup_destroy_locked, which is only called from two places: * In cgroup_mkdir (only on failure) * In cgroup_rmdir See: https://elixir.bootlin.com/linux/v6.5-rc1/A/ident/cgroup_destroy_locked We should be covered here.
