Re: [PATCH][RFC] fanotify: deprecate uapi FAN_ALL_* constants

[Jan Kara <jack@suse.cz>]

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

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR