Re: [RFC][PATCH] fanotify: support watching filesystems and mounts inside userns

[Jan Kara <jack@suse.cz>]

On Sun 16-04-23 09:07:22, Amir Goldstein wrote:
> An unprivileged user is allowed to create an fanotify group and add
> inode marks, but not filesystem and mount marks.
> 
> Add limited support for setting up filesystem and mount marks by an
> unprivileged user under the following conditions:
> 
> 1.   User has CAP_SYS_ADMIN in the user ns where the group was created
> 2.a. User has CAP_SYS_ADMIN in the user ns where the filesystem was
>      mounted (implies FS_USERNS_MOUNT)
>   OR (in case setting up a mark mount)
> 2.b. User has CAP_SYS_ADMIN in the user ns attached to an idmapped mount
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

The patch looks good to me. Just two comments below. 

> Christian,
> 
> You can find this patch, along with FAN_UNMOUNT patches on my github [3].
> Please confirm that this meets your needs for watching container mounts.
> 
> [3] https://github.com/amir73il/linux/commits/fan_unmount

Yeah, it would be good to get ack from Christian that the model you propose
works for him.

> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index db3b79b8e901..2c3e123aee14 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -1238,6 +1238,7 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
>  	 * A group with FAN_UNLIMITED_MARKS does not contribute to mark count
>  	 * in the limited groups account.
>  	 */
> +	BUILD_BUG_ON(!(FANOTIFY_ADMIN_INIT_FLAGS & FAN_UNLIMITED_MARKS));
>  	if (!FAN_GROUP_FLAG(group, FAN_UNLIMITED_MARKS) &&
>  	    !inc_ucount(ucounts->ns, ucounts->uid, UCOUNT_FANOTIFY_MARKS))
>  		return ERR_PTR(-ENOSPC);
...
> @@ -1557,21 +1559,13 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>  		goto out_destroy_group;
>  	}
>  
> +	BUILD_BUG_ON(!(FANOTIFY_ADMIN_INIT_FLAGS & FAN_UNLIMITED_QUEUE));
>  	if (flags & FAN_UNLIMITED_QUEUE) {
> -		fd = -EPERM;
> -		if (!capable(CAP_SYS_ADMIN))
> -			goto out_destroy_group;
>  		group->max_events = UINT_MAX;
>  	} else {
>  		group->max_events = fanotify_max_queued_events;
>  	}
>  
> -	if (flags & FAN_UNLIMITED_MARKS) {
> -		fd = -EPERM;
> -		if (!capable(CAP_SYS_ADMIN))
> -			goto out_destroy_group;
> -	}
> -

Perhaps this hunk (plus the BUILD_BUG_ON hunk above) should go into a
separate patch with a proper changelog?  I was scratching my head over it
for a while until I've realized it's unrelated cleanup of dead code.

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