On Sun 02-12-18 13:38:17, Amir Goldstein wrote:
> When user requests the flag FAN_REPORT_FID in fanotify_init(),
> a unique file indetifier of the event target object will be reported
^^^ identifier
> with the event.
>
> The file identifier includes the filesystem's fsid (i.e. from statfs(2))
> and an NFS file handle of the file (i.e. from name_to_handle_at(2)).
>
> The file identifier makes holding the path reference and passing a file
> descriptor to user redundant, so those are disabled in a group with
> FAN_REPORT_FID.
>
> Encode fid and store it in event for a group with FAN_REPORT_FID.
> Up to 12 bytes of file handle on 32bit arch (16 bytes on 64bit arch)
> are stored inline in fanotify_event struct. Larger file handles are
> stored in an extennal allocated buffer.
^^^ external
> On failure to encode fid, we print a warning and queue the event
> without the fid information.
>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
The patch looks good. Just some nits below:
> +static int fanotify_encode_fid(struct fanotify_event *event,
> + const struct path *path, gfp_t gfp)
> +{
> + struct fanotify_fid *fid = &event->fid;
> + int dwords, bytes = 0;
> + struct kstatfs stat;
> + int err, type;
> +
> + stat.f_fsid.val[0] = stat.f_fsid.val[1] = 0;
> + fid->ext_fh = NULL;
> + dwords = 0;
> + err = -ENOENT;
> + type = exportfs_encode_fh(path->dentry, NULL, &dwords, 0);
> + if (!dwords)
> + goto out_err;
> +
> + err = vfs_statfs(path, &stat);
> + if (err)
> + goto out_err;
> +
> + bytes = dwords << 2;
> + if (!fanotify_inline_fh_len(bytes)) {
I'd just remove this helper. Direct comparison against FANOTIFY_INLINE_FH_LEN
is clearer and I don't see a benefit of the wrapper.
> + /* Treat failure to allocate fh as failure to allocate event */
> + err = -ENOMEM;
> + fid->ext_fh = kmalloc(bytes, gfp);
> + if (!fid->ext_fh)
> + goto out_err;
> + }
> +
> + type = exportfs_encode_fh(path->dentry, fanotify_fid_fh(fid, bytes),
> + &dwords, 0);
> + err = -EINVAL;
> + if (!type || type == FILEID_INVALID || bytes != dwords << 2)
> + goto out_err;
> +
> + fid->fsid = stat.f_fsid;
> + event->fh_len = bytes;
> +
> + return type;
> +
> +out_err:
> + pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, type=%d, bytes=%d, err=%i)\n",
> + stat.f_fsid.val[0], stat.f_fsid.val[1],
> + type, bytes, err);
> + kfree(fid->ext_fh);
> + fid->ext_fh = NULL;
> + event->fh_len = 0;
> +
> + return FILEID_INVALID;
> +}
> +
> struct fanotify_event *fanotify_alloc_event(struct fsnotify_group *group,
> struct inode *inode, u32 mask,
> const struct path *path)
> @@ -181,10 +241,16 @@ init: __maybe_unused
> event->pid = get_pid(task_pid(current));
> else
> event->pid = get_pid(task_tgid(current));
> - if (path) {
> + event->fh_len = 0;
> + if (path && FAN_GROUP_FLAG(group, FAN_REPORT_FID)) {
> + /* Report the event without a file identifier on encode error */
> + event->fh_type = fanotify_encode_fid(event, path, gfp);
> + } else if (path) {
> + event->fh_type = 0;
Maybe using FILEID_ROOT here and in other places like the test in
fanotify_encode_fid(), fanotify_event_has_path(), etc. would make it more
obvious that there cannot be a random clash with some real fh type? At
least I had to look it up in the exportfs code...
> event->path = *path;
> path_get(&event->path);
> } else {
> + event->fh_type = FILEID_INVALID;
> event->path.mnt = NULL;
> event->path.dentry = NULL;
> }
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
