Re: [PATCH v4 09/15] fanotify: cache fsid in fsnotify_mark_connector

[Jan Kara <jack@suse.cz>]

On Sun 02-12-18 13:38:20, Amir Goldstein wrote:
> @@ -268,6 +289,7 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>  	int ret = 0;
>  	struct fanotify_event *event;
>  	struct fsnotify_event *fsn_event;
> +	__kernel_fsid_t __fsid, *fsid = NULL;
>  
>  	BUILD_BUG_ON(FAN_ACCESS != FS_ACCESS);
>  	BUILD_BUG_ON(FAN_MODIFY != FS_MODIFY);
> @@ -300,7 +322,10 @@ static int fanotify_handle_event(struct fsnotify_group *group,
>  			return 0;
>  	}
>  
> -	event = fanotify_alloc_event(group, inode, mask, data);
> +	if (FAN_GROUP_FLAG(group, FAN_REPORT_FID))
> +		fsid = fanotify_get_fsid(iter_info, &__fsid);
> +
> +	event = fanotify_alloc_event(group, inode, mask, data, fsid);

This looks wrong? fsid is never assigned anything != NULL? How could have
this worked?

> @@ -853,9 +855,9 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
>  }
>  
>  /* Check if filesystem can encode a unique fid */
> -static int fanotify_test_fid(struct path *path)
> +static int fanotify_test_fid(struct path *path, struct kstatfs *stat)
>  {
> -	struct kstatfs stat, root_stat;
> +	struct kstatfs root_stat;

Any reason why you don't just return fsid from fanotify_test_fid() instead
of full kstatfs structure? The extra copy of 8 bytes does not really hurt
us...

> @@ -553,15 +560,33 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
>  
>  	if (WARN_ON(!fsnotify_valid_obj_type(type)))
>  		return -EINVAL;
> +
> +	/* Backend is expected to check for zero fsid (e.g. tmpfs) */
> +	if (fsid && WARN_ON_ONCE(!fsid->val[0] && !fsid->val[1]))
> +		return -ENODEV;
> +
>  restart:
>  	spin_lock(&mark->lock);
>  	conn = fsnotify_grab_connector(connp);
>  	if (!conn) {
>  		spin_unlock(&mark->lock);
> -		err = fsnotify_attach_connector_to_object(connp, type);
> +		err = fsnotify_attach_connector_to_object(connp, type, fsid);
>  		if (err)
>  			return err;
>  		goto restart;
> +	} else if (fsid &&
> +		   (fsid->val[0] != conn->fsid.val[0] ||
> +		    fsid->val[1] != conn->fsid.val[1])) {

I think you need to allow transition from 0 to something != 0 here.
Otherwise you'll get this warning if someone attaches say inotify mark to
an inode and then fanotify mark for FAN_REPORT_FID group.

> +		/*
> +		 * Backend is expected to check for non uniform fsid
> +		 * (e.g. btrfs), but maybe we missed something?
> +		 */
> +		pr_warn_ratelimited("%s: fsid mismatch on object of type %u: %x.%x != %x.%x\n",
> +				    __func__, conn->type,
> +				    fsid->val[0], fsid->val[1],
> +				    conn->fsid.val[0], conn->fsid.val[1]);
> +		err = -EXDEV;
> +		goto out_err;
>  	}
>  
>  	/* is mark the first mark? */

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