Message ID | 20180927220112.25123-1-amir73il@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [RFC] fanotify: deprecate uapi FAN_ALL_* constants | expand |
On Fri 28-09-18 01:01:12, Amir Goldstein wrote: > We do not want to add new bits to the FAN_ALL_* uapi constants > because they have been exposed to userspace. If there are programs > out there using these constants, those programs could break if > re-compiled with modified FAN_ALL_* constants and run on an old kernel. > > We deprecate the uapi constants FAN_ALL_* and define new FAN_USER_* > constants for internal use to replace them. New feature bits will be > added only to the new constants. > > Use high bits for kernel internal flag FAN_MARK_ONDIR and add > BUILD_BUG_ON to avoid collision between uapi and kernel internal > mark flags. > > Cc: <linux-api@vger.kernel.org> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > Jan, > > I have rebased the API changes (FAN_MARK_FILESYSTEM and > FAN_EVENT_INFO_TID) on top of commit 60f7ed8c7c4d ("fsnotify: send path > type events to group with super block marks") from your 'fsnotify' > branch starting with this change. The work is available on my branch > fanotify_api-v3 [1]. > > The end result is that no existing uapi constant are modified and > new bit group definitions (FAN_MARK_TYPE_MASK, FAN_EVENT_INFO_FLAGS) > are not repeating past mistake and not exposed in uapi. > > If you agree with this approach and I will post the rest of the series. > > Thanks, > Amir. > > [1] https://github.com/amir73il/linux/commits/fanotify_api-v3 WRT all stuff in your tree I'd prefer if we had to rebase less stuff. What about following: 1) AFAIU "fanotify: store fanotify_init() flags in group's fanotify_data" needs no change so I keep it. 2) I'll drop "fanotify: support reporting thread id instead of process id" from fsnotify for now and merge your new version once you post it together with your cleanup of mask constants. 3) I will keep "fanotify: add API to attach/detach super block mark" as is, just please write a separate small patch that resolves the clash between FAN_MARK_ONDIR and FAN_MARK_FILESYSTEM. Regarding to this patch I have just two nits: > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 94b52157bf8d..e5a3c69848e4 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -131,8 +131,8 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info, > !(marks_mask & FS_ISDIR & ~marks_ignored_mask)) > return false; > > - if (event_mask & FAN_ALL_OUTGOING_EVENTS & marks_mask & > - ~marks_ignored_mask) > + if (event_mask & FAN_USER_OUTGOING_EVENTS & > + marks_mask & ~marks_ignored_mask) > return true; I don't like the _USER_ part of the constant name. How about _KNOWN_? I.e., FAN_KNOWN_OUTGOING_EVENTS sounds about like what it should? ... > + BUILD_BUG_ON(FAN_USER_MARK_FLAGS & FAN_KERN_MARK_FLAGS); > + We have in fsnotify_init(): BUG_ON(hweight32(ALL_FSNOTIFY_EVENTS) != 23) Maybe we should have in fanotify_user_setup() something similar for fanotify flags including the internal ones? > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > index 096c96f4f16a..a67430811006 100644 > --- a/include/linux/fanotify.h > +++ b/include/linux/fanotify.h > @@ -4,6 +4,56 @@ > > #include <uapi/linux/fanotify.h> > > -/* not valid from userspace, only kernel internal */ > -#define FAN_MARK_ONDIR 0x00000100 > +/* > + * Flags not valid from userspace, only kernel internal. > + * Use high bits so we won't collide with userspace flags. > + */ > +#define FAN_MARK_ONDIR 0x80000000 This ought to be a separate change as I wrote above. Thanks! Honza
On Tue, Oct 2, 2018 at 6:56 PM Jan Kara <jack@suse.cz> wrote: > > On Fri 28-09-18 01:01:12, Amir Goldstein wrote: > > We do not want to add new bits to the FAN_ALL_* uapi constants > > because they have been exposed to userspace. If there are programs > > out there using these constants, those programs could break if > > re-compiled with modified FAN_ALL_* constants and run on an old kernel. > > > > We deprecate the uapi constants FAN_ALL_* and define new FAN_USER_* > > constants for internal use to replace them. New feature bits will be > > added only to the new constants. > > > > Use high bits for kernel internal flag FAN_MARK_ONDIR and add > > BUILD_BUG_ON to avoid collision between uapi and kernel internal > > mark flags. > > > > Cc: <linux-api@vger.kernel.org> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > Jan, > > > > I have rebased the API changes (FAN_MARK_FILESYSTEM and > > FAN_EVENT_INFO_TID) on top of commit 60f7ed8c7c4d ("fsnotify: send path > > type events to group with super block marks") from your 'fsnotify' > > branch starting with this change. The work is available on my branch > > fanotify_api-v3 [1]. > > > > The end result is that no existing uapi constant are modified and > > new bit group definitions (FAN_MARK_TYPE_MASK, FAN_EVENT_INFO_FLAGS) > > are not repeating past mistake and not exposed in uapi. > > > > If you agree with this approach and I will post the rest of the series. > > > > Thanks, > > Amir. > > > > [1] https://github.com/amir73il/linux/commits/fanotify_api-v3 > > WRT all stuff in your tree I'd prefer if we had to rebase less stuff. What > about following: > > 1) AFAIU "fanotify: store fanotify_init() flags in group's fanotify_data" > needs no change so I keep it. > > 2) I'll drop "fanotify: support reporting thread id instead of process id" > from fsnotify for now and merge your new version once you post it together > with your cleanup of mask constants. > > 3) I will keep "fanotify: add API to attach/detach super block mark" as is, > just please write a separate small patch that resolves the clash between > FAN_MARK_ONDIR and FAN_MARK_FILESYSTEM. > OK. > Regarding to this patch I have just two nits: > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > index 94b52157bf8d..e5a3c69848e4 100644 > > --- a/fs/notify/fanotify/fanotify.c > > +++ b/fs/notify/fanotify/fanotify.c > > @@ -131,8 +131,8 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info, > > !(marks_mask & FS_ISDIR & ~marks_ignored_mask)) > > return false; > > > > - if (event_mask & FAN_ALL_OUTGOING_EVENTS & marks_mask & > > - ~marks_ignored_mask) > > + if (event_mask & FAN_USER_OUTGOING_EVENTS & > > + marks_mask & ~marks_ignored_mask) > > return true; > > I don't like the _USER_ part of the constant name. How about _KNOWN_? > I.e., FAN_KNOWN_OUTGOING_EVENTS sounds about like what it should? > Do you mean just for *this* constant? I would rather have some uniformity across constants if possible. Errr... Maybe FAN_VALID_EVENTS/FAN_VALID_OUTGOING_EVENTS Although _VALID_ is more appropriate for user inputs, so maybe just FAN_KNOWN_OUTGOING_EVENTS as an exception to the convention is fine. Or simply FAN_EVENTS_MASK/FAN_OUTGOING_EVENTS_MASK, like the recent new constant FAN_MARK_TYPE_MASK? > ... > > + BUILD_BUG_ON(FAN_USER_MARK_FLAGS & FAN_KERN_MARK_FLAGS); > > + > > We have in fsnotify_init(): > > BUG_ON(hweight32(ALL_FSNOTIFY_EVENTS) != 23) > > Maybe we should have in fanotify_user_setup() something similar for > fanotify flags including the internal ones? > OK. > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > > index 096c96f4f16a..a67430811006 100644 > > --- a/include/linux/fanotify.h > > +++ b/include/linux/fanotify.h > > @@ -4,6 +4,56 @@ > > > > #include <uapi/linux/fanotify.h> > > > > -/* not valid from userspace, only kernel internal */ > > -#define FAN_MARK_ONDIR 0x00000100 > > +/* > > + * Flags not valid from userspace, only kernel internal. > > + * Use high bits so we won't collide with userspace flags. > > + */ > > +#define FAN_MARK_ONDIR 0x80000000 > > This ought to be a separate change as I wrote above. > Sure and maybe I'll name it FAN_KERN_MARK_ONDIR. Thanks, Amir.
On Tue 02-10-18 19:23:53, Amir Goldstein wrote: > On Tue, Oct 2, 2018 at 6:56 PM Jan Kara <jack@suse.cz> wrote: > > Regarding to this patch I have just two nits: > > > > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > > > index 94b52157bf8d..e5a3c69848e4 100644 > > > --- a/fs/notify/fanotify/fanotify.c > > > +++ b/fs/notify/fanotify/fanotify.c > > > @@ -131,8 +131,8 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info, > > > !(marks_mask & FS_ISDIR & ~marks_ignored_mask)) > > > return false; > > > > > > - if (event_mask & FAN_ALL_OUTGOING_EVENTS & marks_mask & > > > - ~marks_ignored_mask) > > > + if (event_mask & FAN_USER_OUTGOING_EVENTS & > > > + marks_mask & ~marks_ignored_mask) > > > return true; > > > > I don't like the _USER_ part of the constant name. How about _KNOWN_? > > I.e., FAN_KNOWN_OUTGOING_EVENTS sounds about like what it should? > > > > Do you mean just for *this* constant? I would rather have some uniformity > across constants if possible. No, I meant for all. Sorry for the confusion. > Errr... Maybe FAN_VALID_EVENTS/FAN_VALID_OUTGOING_EVENTS > Although _VALID_ is more appropriate for user inputs, so maybe just > FAN_KNOWN_OUTGOING_EVENTS as an exception to the convention > is fine. > > Or simply FAN_EVENTS_MASK/FAN_OUTGOING_EVENTS_MASK, like > the recent new constant FAN_MARK_TYPE_MASK? Yeah, MASK is fine with me as well. So are the names with VALID. Just pick one you like the most. > > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > > > index 096c96f4f16a..a67430811006 100644 > > > --- a/include/linux/fanotify.h > > > +++ b/include/linux/fanotify.h > > > @@ -4,6 +4,56 @@ > > > > > > #include <uapi/linux/fanotify.h> > > > > > > -/* not valid from userspace, only kernel internal */ > > > -#define FAN_MARK_ONDIR 0x00000100 > > > +/* > > > + * Flags not valid from userspace, only kernel internal. > > > + * Use high bits so we won't collide with userspace flags. > > > + */ > > > +#define FAN_MARK_ONDIR 0x80000000 > > > > This ought to be a separate change as I wrote above. > > > > Sure and maybe I'll name it FAN_KERN_MARK_ONDIR. Fine by me. Honza
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 94b52157bf8d..e5a3c69848e4 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -131,8 +131,8 @@ static bool fanotify_should_send_event(struct fsnotify_iter_info *iter_info, !(marks_mask & FS_ISDIR & ~marks_ignored_mask)) return false; - if (event_mask & FAN_ALL_OUTGOING_EVENTS & marks_mask & - ~marks_ignored_mask) + if (event_mask & FAN_USER_OUTGOING_EVENTS & + marks_mask & ~marks_ignored_mask) return true; return false; @@ -236,7 +236,7 @@ static int fanotify_handle_event(struct fsnotify_group *group, ret = fsnotify_add_event(group, fsn_event, fanotify_merge); if (ret) { /* Permission events shouldn't be merged */ - BUG_ON(ret == 1 && mask & FAN_ALL_PERM_EVENTS); + BUG_ON(ret == 1 && mask & FAN_USER_PERM_EVENTS); /* Our event wasn't used in the end. Free it. */ fsnotify_destroy_event(group, fsn_event); diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index 8609ba06f474..2686a34319ca 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -44,7 +44,7 @@ FANOTIFY_PE(struct fsnotify_event *fse) static inline bool fanotify_is_perm_event(u32 mask) { return IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS) && - mask & FAN_ALL_PERM_EVENTS; + mask & FAN_USER_PERM_EVENTS; } static inline struct fanotify_event_info *FANOTIFY_E(struct fsnotify_event *fse) diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 69054886915b..5ede3488ec93 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -131,7 +131,7 @@ static int fill_event_metadata(struct fsnotify_group *group, metadata->metadata_len = FAN_EVENT_METADATA_LEN; metadata->vers = FANOTIFY_METADATA_VERSION; metadata->reserved = 0; - metadata->mask = fsn_event->mask & FAN_ALL_OUTGOING_EVENTS; + metadata->mask = fsn_event->mask & FAN_USER_OUTGOING_EVENTS; metadata->pid = pid_vnr(event->tgid); if (unlikely(fsn_event->mask & FAN_Q_OVERFLOW)) metadata->fd = FAN_NOFD; @@ -395,7 +395,7 @@ static int fanotify_release(struct inode *ignored, struct file *file) */ while (!fsnotify_notify_queue_is_empty(group)) { fsn_event = fsnotify_remove_first_event(group); - if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS)) { + if (!(fsn_event->mask & FAN_USER_PERM_EVENTS)) { spin_unlock(&group->notification_lock); fsnotify_destroy_event(group, fsn_event); spin_lock(&group->notification_lock); @@ -693,9 +693,9 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags) return -EPERM; #ifdef CONFIG_AUDITSYSCALL - if (flags & ~(FAN_ALL_INIT_FLAGS | FAN_ENABLE_AUDIT)) + if (flags & ~(FAN_USER_INIT_FLAGS | FAN_ENABLE_AUDIT)) #else - if (flags & ~FAN_ALL_INIT_FLAGS) + if (flags & ~FAN_USER_INIT_FLAGS) #endif return -EINVAL; @@ -746,7 +746,7 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags) group->fanotify_data.f_flags = event_f_flags; init_waitqueue_head(&group->fanotify_data.access_waitq); INIT_LIST_HEAD(&group->fanotify_data.access_list); - switch (flags & FAN_ALL_CLASS_BITS) { + switch (flags & FAN_USER_CLASS_BITS) { case FAN_CLASS_NOTIF: group->priority = FS_PRIO_0; break; @@ -805,9 +805,11 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, struct fsnotify_group *group; struct fd f; struct path path; - u32 valid_mask = FAN_ALL_EVENTS | FAN_EVENT_ON_CHILD; + u32 valid_mask = FAN_USER_EVENTS | FAN_EVENT_ON_CHILD; int ret; + BUILD_BUG_ON(FAN_USER_MARK_FLAGS & FAN_KERN_MARK_FLAGS); + pr_debug("%s: fanotify_fd=%d flags=%x dfd=%d pathname=%p mask=%llx\n", __func__, fanotify_fd, flags, dfd, pathname, mask); @@ -815,7 +817,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, if (mask & ((__u64)0xffffffff << 32)) return -EINVAL; - if (flags & ~FAN_ALL_MARK_FLAGS) + if (flags & ~FAN_USER_MARK_FLAGS) return -EINVAL; switch (flags & (FAN_MARK_ADD | FAN_MARK_REMOVE | FAN_MARK_FLUSH)) { case FAN_MARK_ADD: /* fallthrough */ @@ -837,7 +839,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, } if (IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) - valid_mask |= FAN_ALL_PERM_EVENTS; + valid_mask |= FAN_USER_PERM_EVENTS; if (mask & ~valid_mask) return -EINVAL; @@ -857,7 +859,7 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, * allowed to set permissions events. */ ret = -EINVAL; - if (mask & FAN_ALL_PERM_EVENTS && + if (mask & FAN_USER_PERM_EVENTS && group->priority == FS_PRIO_0) goto fput_and_out; diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h index 096c96f4f16a..a67430811006 100644 --- a/include/linux/fanotify.h +++ b/include/linux/fanotify.h @@ -4,6 +4,56 @@ #include <uapi/linux/fanotify.h> -/* not valid from userspace, only kernel internal */ -#define FAN_MARK_ONDIR 0x00000100 +/* + * Flags not valid from userspace, only kernel internal. + * Use high bits so we won't collide with userspace flags. + */ +#define FAN_MARK_ONDIR 0x80000000 + +#define FAN_KERN_MARK_FLAGS (FAN_MARK_ONDIR) + +/* + * Flags allowed to be passed from/to userspace. + * + * We intentionally do not add new bits to the old FAN_ALL_* constants, because + * they are uapi exposed constants. If there are programs out there using + * these constant, the programs may break if re-compiled with new uapi headers + * and then run on an old kernel. + */ +#define FAN_USER_CLASS_BITS (FAN_CLASS_NOTIF | FAN_CLASS_CONTENT | \ + FAN_CLASS_PRE_CONTENT) + +#define FAN_USER_INIT_FLAGS (FAN_CLOEXEC | FAN_NONBLOCK | \ + FAN_USER_CLASS_BITS | \ + FAN_UNLIMITED_QUEUE | FAN_UNLIMITED_MARKS) + +#define FAN_USER_MARK_FLAGS (FAN_MARK_ADD | \ + FAN_MARK_REMOVE | \ + FAN_MARK_DONT_FOLLOW | \ + FAN_MARK_ONLYDIR | \ + FAN_MARK_IGNORED_MASK | \ + FAN_MARK_IGNORED_SURV_MODIFY | \ + FAN_MARK_FLUSH| \ + FAN_MARK_MOUNT) + +/* All events that user can request */ +#define FAN_USER_EVENTS (FAN_ACCESS | FAN_MODIFY | \ + FAN_CLOSE | FAN_OPEN) + +/* All events which require a permission response from userspace */ +#define FAN_USER_PERM_EVENTS (FAN_OPEN_PERM | FAN_ACCESS_PERM) + +/* All events that can be reported to user */ +#define FAN_USER_OUTGOING_EVENTS (FAN_USER_EVENTS | \ + FAN_USER_PERM_EVENTS | \ + FAN_Q_OVERFLOW) + +/* Do not use these old uapi constants internally */ +#undef FAN_ALL_CLASS_BITS +#undef FAN_ALL_INIT_FLAGS +#undef FAN_ALL_MARK_FLAGS +#undef FAN_ALL_EVENTS +#undef FAN_ALL_PERM_EVENTS +#undef FAN_ALL_OUTGOING_EVENTS + #endif /* _LINUX_FANOTIFY_H */ diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h index 74247917de04..690acdbb2bec 100644 --- a/include/uapi/linux/fanotify.h +++ b/include/uapi/linux/fanotify.h @@ -31,6 +31,8 @@ #define FAN_CLASS_NOTIF 0x00000000 #define FAN_CLASS_CONTENT 0x00000004 #define FAN_CLASS_PRE_CONTENT 0x00000008 + +/* Deprecated - do not use this in programs and do not add new flags here! */ #define FAN_ALL_CLASS_BITS (FAN_CLASS_NOTIF | FAN_CLASS_CONTENT | \ FAN_CLASS_PRE_CONTENT) @@ -38,6 +40,7 @@ #define FAN_UNLIMITED_MARKS 0x00000020 #define FAN_ENABLE_AUDIT 0x00000040 +/* Deprecated - do not use this in programs and do not add new flags here! */ #define FAN_ALL_INIT_FLAGS (FAN_CLOEXEC | FAN_NONBLOCK | \ FAN_ALL_CLASS_BITS | FAN_UNLIMITED_QUEUE |\ FAN_UNLIMITED_MARKS) @@ -52,6 +55,7 @@ #define FAN_MARK_IGNORED_SURV_MODIFY 0x00000040 #define FAN_MARK_FLUSH 0x00000080 +/* Deprecated - do not use this in programs and do not add new flags here! */ #define FAN_ALL_MARK_FLAGS (FAN_MARK_ADD |\ FAN_MARK_REMOVE |\ FAN_MARK_DONT_FOLLOW |\ @@ -61,11 +65,7 @@ FAN_MARK_IGNORED_SURV_MODIFY |\ FAN_MARK_FLUSH) -/* - * All of the events - we build the list by hand so that we can add flags in - * the future and not break backward compatibility. Apps will get only the - * events that they originally wanted. Be sure to add new events here! - */ +/* Deprecated - do not use this in programs and do not add new flags here! */ #define FAN_ALL_EVENTS (FAN_ACCESS |\ FAN_MODIFY |\ FAN_CLOSE |\ @@ -74,9 +74,11 @@ /* * All events which require a permission response from userspace */ +/* Deprecated - do not use this in programs and do not add new flags here! */ #define FAN_ALL_PERM_EVENTS (FAN_OPEN_PERM |\ FAN_ACCESS_PERM) +/* Deprecated - do not use this in programs and do not add new flags here! */ #define FAN_ALL_OUTGOING_EVENTS (FAN_ALL_EVENTS |\ FAN_ALL_PERM_EVENTS |\ FAN_Q_OVERFLOW)
We do not want to add new bits to the FAN_ALL_* uapi constants because they have been exposed to userspace. If there are programs out there using these constants, those programs could break if re-compiled with modified FAN_ALL_* constants and run on an old kernel. We deprecate the uapi constants FAN_ALL_* and define new FAN_USER_* constants for internal use to replace them. New feature bits will be added only to the new constants. Use high bits for kernel internal flag FAN_MARK_ONDIR and add BUILD_BUG_ON to avoid collision between uapi and kernel internal mark flags. Cc: <linux-api@vger.kernel.org> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Jan, I have rebased the API changes (FAN_MARK_FILESYSTEM and FAN_EVENT_INFO_TID) on top of commit 60f7ed8c7c4d ("fsnotify: send path type events to group with super block marks") from your 'fsnotify' branch starting with this change. The work is available on my branch fanotify_api-v3 [1]. The end result is that no existing uapi constant are modified and new bit group definitions (FAN_MARK_TYPE_MASK, FAN_EVENT_INFO_FLAGS) are not repeating past mistake and not exposed in uapi. If you agree with this approach and I will post the rest of the series. Thanks, Amir. [1] https://github.com/amir73il/linux/commits/fanotify_api-v3 fs/notify/fanotify/fanotify.c | 6 ++-- fs/notify/fanotify/fanotify.h | 2 +- fs/notify/fanotify/fanotify_user.c | 20 ++++++----- include/linux/fanotify.h | 54 ++++++++++++++++++++++++++++-- include/uapi/linux/fanotify.h | 12 ++++--- 5 files changed, 74 insertions(+), 20 deletions(-)