On Wed, Oct 12, 2022 at 7:28 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Wed, Oct 12, 2022 at 6:44 PM Jan Kara <jack@xxxxxxx> wrote: > > > > Hi Amir! > > > > On Fri 07-10-22 16:58:21, Amir Goldstein wrote: > > > > > The other use case of automatic inode marks I was thinking about, > > > > > which are even more relevant for $SUBJECT is this: > > > > > When instantiating a dentry with an inode that has xattr > > > > > "security.fanotify.mask" (a.k.a. persistent inode mark), an inode > > > > > mark could be auto created and attached to a group with a special sb > > > > > mark (we can limit a single special mark per sb). > > > > > This could be implemented similar to get_acl(), where i_fsnotify_mask > > > > > is always initialized with a special value (i.e. FS_UNINITIALIZED) > > > > > which is set to either 0 or non-zero if "security.fanotify.mask" exists. > > > > > > > > > > The details of how such an API would look like are very unclear to me, > > > > > so I will try to focus on the recursive auto inode mark idea. > > > > > > > > Yeah, although initializing fanotify marks based on xattrs does not look > > > > completely crazy I can see a lot of open questions there so I think > > > > automatic inode mark idea has more chances for success at this point :). > > > > > > I realized that there is one sort of "persistent mark" who raises > > > less questions - one that only has an ignore mask. > > > > > > ignore masks can have a "static" namespace that is not bound to any > > > specific group, but rather a set of groups that join this namespace. > > > > > > I played with this idea and wrote some patches: > > > https://github.com/amir73il/linux/commits/fan_xattr_ignore_mask > > > > I have glanced over the patches. In general the idea looks OK to me but I > > have some concerns: > > > > 1) Technically, it may be challenging to call into filesystem xattr > > handling code on first event generated by the inode - that may generate > > some unexpected lock recursion for some filesystems and some events which > > trigger the initialization... > > That may be a correct statement in general, but please note that > - Only permission events trigger auto-init of xattr ignore mask > - Permission events are called from security hooks > - Security hooks may also call getxattr to get the security context > > Perhaps LSMs always initialize the security context in OPEN and > never in ACCESS? > > One of the earlier versions of the patch initialized xattr ignore mask > on LOOKUP permission event, if ANY object was interested in ANY > permission event even if no object was interested in LOOKUP > to mimic the LSM context initialization, > but it was complicated and I wasn't sure if this was necessary. > Also, permission events can sleep by definition so why would getxattr not be safe in the context of permission events handling? > > > > > 2) What if you set the xattr while the group is already listening to > > events? Currently the change will get ignored, won't it? But I guess this > > could be handled by clearing the "cached" flag when the xattr is set. > > > > I have created an API to update the xattr via > fanotify_mark(FAN_MARK_XATTR, ... > which updates the cached ignore mask in the connector. > > I see no reason to support "direct" modifications of this xattr. > If such changes are made directly it is fine to ignore them. > > > 3) What if multiple applications want to use the persistent mark > > functionality? I think we need some way to associate a particular > > fanotify group with a particular subset of fanotify xattrs so that > > coexistence of multiple applications is possible... > > > > Yeh, I thought about this as well. > The API in the patches is quite naive because it implements a single > global namespace for xattr ignore masks, but mostly I wanted to > see if I can get the fast path and auto-init implementation done. > > I was generally thinking of ioctl() as the API to join an xattr marks > namespace and negotiate the on-disk format of persistent marks > supported by the application. > > I would not want to allow multiple fanotify xattrs per inode - > that could have the consequence of the inode becoming a junkyard. > > I'd prefer to have a single xattr (say security.fanotify.mark) > and that mark will have > - on-disk format version > - namespace id > - ignore mask > - etc > > If multiple applications want to use persistent marks they need to figure > out how to work together without stepping on each other's toes. > I don't think it is up to fanotify to coordinate that. > > fanotify_mark() can fail with EEXIST when a group that joined namespace A > is trying to update a persistent mark when a persistent mark of namespace B > already exists and probably some FAN_MARK_REPLACE flag could be used > to force overwrite the existing persistent mark. > One thing that I feel a bit silly about is something that I only fully noticed after writing this WIP wiki entry: https://github.com/amir73il/fsnotify-utils/wiki/Hierarchical-Storage-Management-API#persistent-inode-marks Persistent marks (in xattr) with ignore mask are useful, but only a little bit more useful than Evictable marks with ignore mask. Persistent marks (in xattr) with a "positive" event mask are the real deal. Because with "positive" persistent marks, we will be able to optimize away srcu_read_lock() and marks iteration for the majority of fsnotify hooks by looking at objects interest masks (including the FS_XATTR_CACHED bit). The good thing about the POC patches [1] is that there is no technical limitation in this code for using persistent marks with positive event masks. It is a bit more challenging to document the fact that a "normal" fs/mount mark is needed in order to "activate" the persistent marks in the inodes of this fs/mount, but the changes to the code to support that would be minimal. Thanks, Amir. [1] https://github.com/amir73il/linux/commits/fan_xattr_ignore_mask
