On Tue 06-02-24 18:44:47, Amir Goldstein wrote: > On Tue, Feb 6, 2024 at 6:30 PM Sargun Dhillon <sargun@xxxxxxxxx> wrote: > > > > On Tue, Feb 6, 2024 at 6:50 AM Jan Kara <jack@xxxxxxx> wrote: > > > > > > On Tue 06-02-24 09:44:29, Amir Goldstein wrote: > > > > On Tue, Feb 6, 2024 at 1:24 AM Sargun Dhillon <sargun@xxxxxxxxx> wrote: > > > > > > > > > > One of the issues we've hit recently while using fanotify in an HSM is > > > > > racing with files that are opened for execution. > > > > > > > > > > There is a race that can result in ETXTBUSY. > > > > > Pid 1: You have a file marked with FAN_OPEN_EXEC_PERM. > > > > > Pid 2: execve(file_by_path) > > > > > Pid 1: gets notification, with file.fd > > > > > Pid 2: blocked, waiting for notification to resolve > > > > > Pid 1: Does work with FD (populates the file) > > > > > Pid 1: writes FAN_ALLOW to the fanotify file descriptor allowing the event. > > > > > Pid 2: continues, and falls through to deny_write_access (and fails) > > > > > Pid 1: closes fd > > > > > > Right, this is kind of nasty. > > > > > > > > Pid 1 can close the FD before responding, but this can result in a > > > > > race if fanotify is being handled in a multi-threaded > > > > > manner. > > > > > > Yep. > > > > > > > > I.e. if there are two threads operating on the same fanotify group, > > > > > and an event's FD has been closed, that can be reused > > > > > by another event. This is largely not a problem because the > > > > > outstanding events are added in a FIFO manner to the outstanding > > > > > event list, and as long as the earlier event is closed and responded > > > > > to without interruption, it should be okay, but it's difficult > > > > > to guarantee that this happens, unless event responses are serialized > > > > > in some fashion, with strict ordering between > > > > > responses. > > > > > > Yes, essentially you must make sure you will not read any new events from > > > the notification queue between fd close & writing of the response. Frankly, > > > I find this as quite ugly and asking for trouble (subtle bugs down the > > > line). > > > > > Is there a preference for either refactoring fanotify_event_metadata, or > > adding this new ID type as a piece of metadata? > > > > I almost feel like the FD should move to being metadata, and we should > > use ID in place of fd in fanotify_event_metadata. If we use an xarray, > > it should be reasonable to use a 32-bit identifier, so we don't need > > to modify the fanotify_event_metadata structure at all. > > I have a strong preference for FANOTIFY_METADATA_VERSION 4 > because I really would like event->key to be 64bit and in the header, > but I have a feeling that Jan may have a different opinion.. I also think 64-bit ID would be potentially more useful for userspace (mostly because of guaranteed uniqueness). I'm just still not yet sure how do you plan to use it for persistent events because there are several options how to generate the ID. I'd hate to add yet-another-id in the near future. Regarding FANOTIFY_METADATA_VERSION 4: What are your reasons to want the ID in the header? I think we'd need explicit init flag to enable event ID reporting anyway? But admittedly I can see some appeal of having ID in the header if we are going to use the ID for matching responses to permission events. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
