diff --git a/kernel/src/fs/notify/inotify.rs b/kernel/src/fs/notify/inotify.rs index 459a4229f..60ec39985 100644 --- a/kernel/src/fs/notify/inotify.rs +++ b/kernel/src/fs/notify/inotify.rs @@ -169,19 +169,9 @@ impl InotifyFile { let subscriber = inotify_subscriber.clone() as Arc; let inode = path.inode(); - if !inode + inode .fs_event_publisher_or_init() - .add_subscriber(subscriber) - { - // FIXME: This can be triggered by `unlink()` race conditions or - // `inotify_add_watch("/proc/self/fd/[fd]")`, where `[fd]` is a file descriptor - // pointing to a deleted file. Although Linux allows this, we don't support it, so an - // error is returned here. - return_errno_with_message!( - Errno::ENOENT, - "adding an inotify watch to a deleted inode is not supported yet" - ); - } + .add_subscriber(subscriber); let wd = inotify_subscriber.wd(); let entry = SubscriberEntry { diff --git a/kernel/src/fs/notify/mod.rs b/kernel/src/fs/notify/mod.rs index 8409a5177..bf5dcc51e 100644 --- a/kernel/src/fs/notify/mod.rs +++ b/kernel/src/fs/notify/mod.rs @@ -3,7 +3,7 @@ use alloc::{sync::Arc, vec::Vec}; use core::{ any::Any, - sync::atomic::{AtomicBool, AtomicU32, Ordering}, + sync::atomic::{AtomicU32, Ordering}, }; use atomic_integer_wrapper::define_atomic_version_of_integer_like_type; @@ -29,8 +29,8 @@ pub struct FsEventPublisher { subscribers: RwLock>>, /// All interesting FS event types (aggregated from all subscribers). all_interesting_events: AtomicFsEvents, - /// Whether this publisher still accepts new subscribers. - accepts_new_subscribers: AtomicBool, + /// FS events to be published when dropped. + events_on_drop: AtomicFsEvents, /// A weak reference to the file system. fs: Weak, } @@ -43,26 +43,40 @@ impl Debug for FsEventPublisher { } } +impl Drop for FsEventPublisher { + fn drop(&mut self) { + let subscribers = self.subscribers.get_mut(); + + let num_subscribers = subscribers.len(); + let events_on_drop = self.events_on_drop.load(Ordering::Relaxed); + for subscriber in subscribers.drain(..) { + if !events_on_drop.is_empty() { + subscriber.deliver_event(events_on_drop, None); + } + subscriber.deliver_event(FsEvents::IN_IGNORED, None); + } + + if let Some(fs) = self.fs.upgrade() { + fs.fs_event_subscriber_stats() + .remove_subscribers(num_subscribers); + } + } +} + impl FsEventPublisher { pub fn new(fs: Weak) -> Self { Self { subscribers: RwLock::new(Vec::new()), all_interesting_events: AtomicFsEvents::new(FsEvents::empty()), - accepts_new_subscribers: AtomicBool::new(true), + events_on_drop: AtomicFsEvents::new(FsEvents::empty()), fs, } } /// Adds a subscriber to this publisher. - pub fn add_subscriber(&self, subscriber: Arc) -> bool { + pub fn add_subscriber(&self, subscriber: Arc) { let mut subscribers = self.subscribers.write(); - // This check must be done after locking `self.subscribers.write()` to avoid race - // conditions. - if !self.accepts_new_subscribers.load(Ordering::Relaxed) { - return false; - } - let current = self.all_interesting_events.load(Ordering::Relaxed); self.all_interesting_events .store(current | subscriber.interesting_events(), Ordering::Relaxed); @@ -72,8 +86,6 @@ impl FsEventPublisher { if let Some(fs) = self.fs.upgrade() { fs.fs_event_subscriber_stats().add_subscriber(); } - - true } /// Removes a subscriber from this publisher. @@ -95,38 +107,6 @@ impl FsEventPublisher { removed } - /// Removes all subscribers from this publisher. - pub fn remove_all_subscribers(&self) -> usize { - let mut subscribers = self.subscribers.write(); - - for subscriber in subscribers.iter() { - subscriber.deliver_event(FsEvents::IN_IGNORED, None); - } - - let num_subscribers = subscribers.len(); - subscribers.clear(); - - self.all_interesting_events - .store(FsEvents::empty(), Ordering::Relaxed); - - if let Some(fs) = self.fs.upgrade() { - fs.fs_event_subscriber_stats() - .remove_subscribers(num_subscribers); - } - - num_subscribers - } - - /// Forbids new subscribers from attaching to this publisher and removes all existing - /// subscribers. - pub fn disable_new_and_remove_subscribers(&self) -> usize { - // Do this before calling `remove_all_subscribers` so that the `self.subscribers.write()` - // lock will synchronize this variable. - self.accepts_new_subscribers.store(false, Ordering::Relaxed); - - self.remove_all_subscribers() - } - /// Broadcasts an event to all the subscribers of this publisher. pub fn publish_event(&self, events: FsEvents, name: Option) { let interesting = self.all_interesting_events.load(Ordering::Relaxed); @@ -179,6 +159,17 @@ impl FsEventPublisher { .store(new_events, Ordering::Relaxed); } + /// Sets the events to be published when the publisher is dropped. + /// + /// [`FsEvents::IN_IGNORED`] always follows the specified FS events. If the + /// specified events are empty, only [`FsEvents::IN_IGNORED`] will be published + /// when the publisher is dropped. + /// + /// Note that this will override any previous settings. + pub fn set_events_on_drop(&self, events: FsEvents) { + self.events_on_drop.store(events, Ordering::Relaxed); + } + /// Finds a subscriber and applies an action if found. /// /// The matcher should return `Some(T)` if the subscriber matches and processing @@ -344,7 +335,9 @@ pub fn on_inode_removed(inode: &Arc) { if !inode.fs().fs_event_subscriber_stats().has_any_subscribers() { return; } - notify_inode(inode, FsEvents::DELETE_SELF); + if let Some(publisher) = inode.fs_event_publisher() { + publisher.set_events_on_drop(FsEvents::DELETE_SELF); + } } /// Notifies that a file was linked to a directory. diff --git a/kernel/src/fs/path/dentry.rs b/kernel/src/fs/path/dentry.rs index 43d1277e8..b51a2b817 100644 --- a/kernel/src/fs/path/dentry.rs +++ b/kernel/src/fs/path/dentry.rs @@ -9,7 +9,7 @@ use super::{is_dot, is_dot_or_dotdot, is_dotdot}; use crate::{ fs::{ self, - utils::{Inode, InodeExt, InodeMode, InodeType, MknodType}, + utils::{Inode, InodeMode, InodeType, MknodType}, }, prelude::*, }; @@ -327,21 +327,9 @@ impl Dentry { let nlinks = child_inode.metadata().nlinks; fs::notify::on_link_count(&child_inode); if nlinks == 0 { - // FIXME: `DELETE_SELF` should be generated after closing the last FD. fs::notify::on_inode_removed(&child_inode); } fs::notify::on_delete(self.inode(), &child_inode, || name.to_string()); - if nlinks == 0 { - // Ideally, we would use `fs_event_publisher()` here to avoid creating a - // `FsEventPublisher` instance on a dying inode. However, it isn't possible because we - // need to disable new subscribers. - let publisher = child_inode.fs_event_publisher_or_init(); - let removed_nr_subscribers = publisher.disable_new_and_remove_subscribers(); - child_inode - .fs() - .fs_event_subscriber_stats() - .remove_subscribers(removed_nr_subscribers); - } Ok(()) } @@ -380,21 +368,9 @@ impl Dentry { let nlinks = child_inode.metadata().nlinks; if nlinks == 0 { - // FIXME: `DELETE_SELF` should be generated after closing the last FD. fs::notify::on_inode_removed(&child_inode); } fs::notify::on_delete(self.inode(), &child_inode, || name.to_string()); - if nlinks == 0 { - // Ideally, we would use `fs_event_publisher()` here to avoid creating a - // `FsEventPublisher` instance on a dying inode. However, it isn't possible because we - // need to disable new subscribers. - let publisher = child_inode.fs_event_publisher_or_init(); - let removed_nr_subscribers = publisher.disable_new_and_remove_subscribers(); - child_inode - .fs() - .fs_event_subscriber_stats() - .remove_subscribers(removed_nr_subscribers); - } Ok(()) } diff --git a/test/initramfs/src/apps/inotify/inotify_unlink.c b/test/initramfs/src/apps/inotify/inotify_unlink.c index 8806dfc47..425138e49 100644 --- a/test/initramfs/src/apps/inotify/inotify_unlink.c +++ b/test/initramfs/src/apps/inotify/inotify_unlink.c @@ -16,20 +16,79 @@ FN_TEST(unlink_add) fd = TEST_RES(open(TEST_FILE, O_CREAT | O_WRONLY, 0644), _ret == 4); TEST_SUCC(unlink(TEST_FILE)); - // FIXME: Asterinas currently does not support adding inotify watches - // to deleted inodes. -#ifdef __asterinas__ - TEST_ERRNO(inotify_add_watch(inotify_fd, "/proc/self/fd/4", - IN_DELETE_SELF), - ENOENT); - (void)wd; -#else wd = TEST_SUCC(inotify_add_watch(inotify_fd, "/proc/self/fd/4", IN_DELETE_SELF)); TEST_SUCC(inotify_rm_watch(inotify_fd, wd)); -#endif TEST_SUCC(close(fd)); TEST_SUCC(close(inotify_fd)); } END_TEST() + +FN_TEST(unlink_closed) +{ + int inotify_fd, fd, wd; + struct inotify_event ev; + + inotify_fd = TEST_SUCC(inotify_init1(O_NONBLOCK)); + + fd = TEST_SUCC(open(TEST_FILE, O_CREAT | O_WRONLY, 0644)); + wd = TEST_SUCC( + inotify_add_watch(inotify_fd, TEST_FILE, IN_DELETE_SELF)); + + TEST_SUCC(close(fd)); + TEST_SUCC(unlink(TEST_FILE)); + + // Both `IN_DELETE_SELF` and `IN_IGNORED` are generated. + TEST_RES(read(inotify_fd, &ev, sizeof(ev)), + _ret == sizeof(ev) && ev.mask == IN_DELETE_SELF); + TEST_RES(read(inotify_fd, &ev, sizeof(ev)), + _ret == sizeof(ev) && ev.mask == IN_IGNORED); + TEST_ERRNO(read(inotify_fd, &ev, sizeof(ev)), EAGAIN); + + // The watch does not exist anymore. + TEST_ERRNO(inotify_rm_watch(inotify_fd, wd), EINVAL); + + TEST_SUCC(close(inotify_fd)); +} +END_TEST() + +FN_TEST(unlink_remove) +{ + int inotify_fd1, inotify_fd2, fd, wd1, wd2; + struct inotify_event ev; + + inotify_fd1 = TEST_SUCC(inotify_init1(O_NONBLOCK)); + inotify_fd2 = TEST_SUCC(inotify_init1(O_NONBLOCK)); + + fd = TEST_SUCC(open(TEST_FILE, O_CREAT | O_WRONLY, 0644)); + wd1 = TEST_SUCC( + inotify_add_watch(inotify_fd1, TEST_FILE, IN_DELETE_SELF)); + wd2 = TEST_SUCC( + inotify_add_watch(inotify_fd2, TEST_FILE, IN_DELETE_SELF)); + + TEST_SUCC(unlink(TEST_FILE)); + TEST_ERRNO(read(inotify_fd1, &ev, sizeof(ev)), EAGAIN); + TEST_ERRNO(read(inotify_fd2, &ev, sizeof(ev)), EAGAIN); + + // Removing the watch after unlinking is fine. + TEST_SUCC(inotify_rm_watch(inotify_fd2, wd2)); + TEST_RES(read(inotify_fd2, &ev, sizeof(ev)), + _ret == sizeof(ev) && ev.mask == IN_IGNORED); + TEST_ERRNO(read(inotify_fd2, &ev, sizeof(ev)), EAGAIN); + + // `IN_DELETE_SELF` and `IN_IGNORED` will be generated once we close `fd`. + TEST_SUCC(close(fd)); + TEST_RES(read(inotify_fd1, &ev, sizeof(ev)), + _ret == sizeof(ev) && ev.mask == IN_DELETE_SELF); + TEST_RES(read(inotify_fd1, &ev, sizeof(ev)), + _ret == sizeof(ev) && ev.mask == IN_IGNORED); + TEST_ERRNO(read(inotify_fd1, &ev, sizeof(ev)), EAGAIN); + + // The watch does not exist anymore after we close `fd`. + TEST_ERRNO(inotify_rm_watch(inotify_fd1, wd1), EINVAL); + + TEST_SUCC(close(inotify_fd1)); + TEST_SUCC(close(inotify_fd2)); +} +END_TEST() diff --git a/test/initramfs/src/syscall/gvisor/blocklists/inotify_test b/test/initramfs/src/syscall/gvisor/blocklists/inotify_test index 9bd5d47fa..bf0624ff5 100644 --- a/test/initramfs/src/syscall/gvisor/blocklists/inotify_test +++ b/test/initramfs/src/syscall/gvisor/blocklists/inotify_test @@ -15,10 +15,6 @@ Inotify.ExcludeUnlinkInodeEvents_NoRandomSave Inotify.ExcludeUnlinkMultipleChildren Inotify.ExcludeUnlinkMultipleChildren_NoRandomSave -# TODO: Generate `DELETE_SELF`/`IGNORED` after closing the last FD. -Inotify.IncludeUnlinkedFile -Inotify.IncludeUnlinkedFile_NoRandomSave - # TODO: Report `MOVE_*` events at the `rename()` system call. Inotify.MoveGeneratesEvents Inotify.MoveWatchedTargetGeneratesEvents