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@xxxxxxxxxxxxxxx>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
>
> 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
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
