[v5,07/17] fanotify: encode file identifier for FAN_REPORT_FID
diff mbox series

Message ID 20190110170444.30616-8-amir73il@gmail.com
State New
Headers show
Series
  • fanotify: add support for more event types
Related show

Commit Message

Amir Goldstein Jan. 10, 2019, 5:04 p.m. UTC
When user requests the flag FAN_REPORT_FID in fanotify_init(),
a unique file identifier of the event target object will be reported
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 external allocated buffer.

On failure to encode fid, we print a warning and queue the event
without the fid information.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify.c      | 81 +++++++++++++++++++++++++++---
 fs/notify/fanotify/fanotify.h      | 78 ++++++++++++++++++++++++++--
 fs/notify/fanotify/fanotify_user.c |  6 +--
 include/uapi/linux/fanotify.h      |  1 +
 4 files changed, 153 insertions(+), 13 deletions(-)

Comments

kernel test robot Jan. 11, 2019, 8:10 a.m. UTC | #1
Hi Amir,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.0-rc1]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Amir-Goldstein/fanotify-add-support-for-more-event-types/20190111-090241
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=mips 

All warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:14:0,
                    from include/linux/list.h:9,
                    from include/linux/preempt.h:11,
                    from include/linux/spinlock.h:51,
                    from include/linux/fdtable.h:11,
                    from fs/notify/fanotify/fanotify.c:3:
   fs/notify/fanotify/fanotify.c: In function 'fanotify_encode_fid':
   include/linux/kern_levels.h:5:18: warning: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'long int' [-Wformat=]
    #define KERN_SOH "\001"  /* ASCII Start Of Header */
                     ^
   include/linux/printk.h:424:10: note: in definition of macro 'printk_ratelimited'
      printk(fmt, ##__VA_ARGS__);    \
             ^~~
   include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
    #define KERN_WARNING KERN_SOH "4" /* warning conditions */
                         ^~~~~~~~
   include/linux/printk.h:440:21: note: in expansion of macro 'KERN_WARNING'
     printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
                        ^~~~~~~~~~~~
>> fs/notify/fanotify/fanotify.c:196:2: note: in expansion of macro 'pr_warn_ratelimited'
     pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, type=%d, bytes=%d, err=%i)\n",
     ^~~~~~~~~~~~~~~~~~~
   fs/notify/fanotify/fanotify.c:196:61: note: format string is defined here
     pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, type=%d, bytes=%d, err=%i)\n",
                                                               ~^
                                                               %lx
   In file included from include/linux/kernel.h:14:0,
                    from include/linux/list.h:9,
                    from include/linux/preempt.h:11,
                    from include/linux/spinlock.h:51,
                    from include/linux/fdtable.h:11,
                    from fs/notify/fanotify/fanotify.c:3:
   include/linux/kern_levels.h:5:18: warning: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'long int' [-Wformat=]
    #define KERN_SOH "\001"  /* ASCII Start Of Header */
                     ^
   include/linux/printk.h:424:10: note: in definition of macro 'printk_ratelimited'
      printk(fmt, ##__VA_ARGS__);    \
             ^~~
   include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
    #define KERN_WARNING KERN_SOH "4" /* warning conditions */
                         ^~~~~~~~
   include/linux/printk.h:440:21: note: in expansion of macro 'KERN_WARNING'
     printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
                        ^~~~~~~~~~~~
>> fs/notify/fanotify/fanotify.c:196:2: note: in expansion of macro 'pr_warn_ratelimited'
     pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, type=%d, bytes=%d, err=%i)\n",
     ^~~~~~~~~~~~~~~~~~~
   fs/notify/fanotify/fanotify.c:196:64: note: format string is defined here
     pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, type=%d, bytes=%d, err=%i)\n",
                                                                  ~^
                                                                  %lx

vim +/pr_warn_ratelimited +196 fs/notify/fanotify/fanotify.c

   154	
   155	static int fanotify_encode_fid(struct fanotify_event *event,
   156				       const struct path *path, gfp_t gfp)
   157	{
   158		struct fanotify_fid *fid = &event->fid;
   159		int dwords, bytes = 0;
   160		struct kstatfs stat;
   161		int err, type;
   162	
   163		stat.f_fsid.val[0] = stat.f_fsid.val[1] = 0;
   164		fid->ext_fh = NULL;
   165		dwords = 0;
   166		err = -ENOENT;
   167		type = exportfs_encode_fh(path->dentry, NULL, &dwords,  0);
   168		if (!dwords)
   169			goto out_err;
   170	
   171		err = vfs_statfs(path, &stat);
   172		if (err)
   173			goto out_err;
   174	
   175		bytes = dwords << 2;
   176		if (bytes > FANOTIFY_INLINE_FH_LEN) {
   177			/* Treat failure to allocate fh as failure to allocate event */
   178			err = -ENOMEM;
   179			fid->ext_fh = kmalloc(bytes, gfp);
   180			if (!fid->ext_fh)
   181				goto out_err;
   182		}
   183	
   184		type = exportfs_encode_fh(path->dentry, fanotify_fid_fh(fid, bytes),
   185					  &dwords,  0);
   186		err = -EINVAL;
   187		if (!type || type == FILEID_INVALID || bytes != dwords << 2)
   188			goto out_err;
   189	
   190		fid->fsid = stat.f_fsid;
   191		event->fh_len = bytes;
   192	
   193		return type;
   194	
   195	out_err:
 > 196		pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, type=%d, bytes=%d, err=%i)\n",
   197				    stat.f_fsid.val[0], stat.f_fsid.val[1],
   198				    type, bytes, err);
   199		kfree(fid->ext_fh);
   200		fid->ext_fh = NULL;
   201		event->fh_len = 0;
   202	
   203		return FILEID_INVALID;
   204	}
   205	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Amir Goldstein Jan. 11, 2019, 8:37 a.m. UTC | #2
On Fri, Jan 11, 2019 at 10:11 AM kbuild test robot <lkp@intel.com> wrote:
>
> Hi Amir,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on linus/master]
> [also build test WARNING on v5.0-rc1]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Amir-Goldstein/fanotify-add-support-for-more-event-types/20190111-090241
> config: mips-allmodconfig (attached as .config)
> compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         GCC_VERSION=7.2.0 make.cross ARCH=mips
>
> All warnings (new ones prefixed by >>):
>
>    In file included from include/linux/kernel.h:14:0,
>                     from include/linux/list.h:9,
>                     from include/linux/preempt.h:11,
>                     from include/linux/spinlock.h:51,
>                     from include/linux/fdtable.h:11,
>                     from fs/notify/fanotify/fanotify.c:3:
>    fs/notify/fanotify/fanotify.c: In function 'fanotify_encode_fid':
>    include/linux/kern_levels.h:5:18: warning: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'long int' [-Wformat=]

I'm confused.
__kernel_fsid_t val member is long[] on mips arch and int[] on other archs.
Which format specifier am I supposed to use to print it?

Thanks,
Amir.
Paul Burton Jan. 18, 2019, 6:39 p.m. UTC | #3
Hi Amir,

On Fri, Jan 11, 2019 at 10:37:39AM +0200, Amir Goldstein wrote:
> On Fri, Jan 11, 2019 at 10:11 AM kbuild test robot <lkp@intel.com> wrote:
> >
> > Hi Amir,
> >
> > I love your patch! Perhaps something to improve:
> >
> > [auto build test WARNING on linus/master]
> > [also build test WARNING on v5.0-rc1]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >
> > url:    https://github.com/0day-ci/linux/commits/Amir-Goldstein/fanotify-add-support-for-more-event-types/20190111-090241
> > config: mips-allmodconfig (attached as .config)
> > compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         # save the attached .config to linux build tree
> >         GCC_VERSION=7.2.0 make.cross ARCH=mips
> >
> > All warnings (new ones prefixed by >>):
> >
> >    In file included from include/linux/kernel.h:14:0,
> >                     from include/linux/list.h:9,
> >                     from include/linux/preempt.h:11,
> >                     from include/linux/spinlock.h:51,
> >                     from include/linux/fdtable.h:11,
> >                     from fs/notify/fanotify/fanotify.c:3:
> >    fs/notify/fanotify/fanotify.c: In function 'fanotify_encode_fid':
> >    include/linux/kern_levels.h:5:18: warning: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'long int' [-Wformat=]
> 
> I'm confused.
> __kernel_fsid_t val member is long[] on mips arch and int[] on other archs.
> Which format specifier am I supposed to use to print it?

That's a good question.

Here's another: why on Earth do we have this custom __kernel_fsid_t
definition for MIPS at all..?

We only provide the MIPS definition when _MIPS_SZLONG == 32, ie. when
long is the same size as int & the generic definition of the struct
would have an identical memory layout anyway.

So I'm tempted to just delete this weird code - the only thing that
might break is if someone is doing something that really expects a long
& cares about getting an int of the same size. For example if anyone
prints the value with %lx or the like - essentially the opposite case to
yours.

I consider it pretty unlikely that anyone will be doing this in a
MIPS32-specific codepath such that they've been seeing the custom
__kernel_fsid_t up until now, but does anyone see a problem with this?

Thanks,
    Paul

---
diff --git a/arch/mips/include/uapi/asm/posix_types.h b/arch/mips/include/uapi/asm/posix_types.h
index 6aa49c10f88f..f0ccb5b90ce9 100644
--- a/arch/mips/include/uapi/asm/posix_types.h
+++ b/arch/mips/include/uapi/asm/posix_types.h
@@ -21,13 +21,6 @@
 typedef long		__kernel_daddr_t;
 #define __kernel_daddr_t __kernel_daddr_t

-#if (_MIPS_SZLONG == 32)
-typedef struct {
-	long	val[2];
-} __kernel_fsid_t;
-#define __kernel_fsid_t __kernel_fsid_t
-#endif
-
 #include <asm-generic/posix_types.h>

 #endif /* _ASM_POSIX_TYPES_H */

Patch
diff mbox series

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index d8e3b6e50844..e431f63c9f58 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -13,6 +13,7 @@ 
 #include <linux/wait.h>
 #include <linux/audit.h>
 #include <linux/sched/mm.h>
+#include <linux/statfs.h>
 
 #include "fanotify.h"
 
@@ -25,10 +26,18 @@  static bool should_merge(struct fsnotify_event *old_fsn,
 	old = FANOTIFY_E(old_fsn);
 	new = FANOTIFY_E(new_fsn);
 
-	if (old_fsn->inode == new_fsn->inode && old->pid == new->pid &&
-	    old->path.mnt == new->path.mnt &&
-	    old->path.dentry == new->path.dentry)
-		return true;
+	if (old_fsn->inode != new_fsn->inode || old->pid != new->pid ||
+	    old->fh_type != new->fh_type || old->fh_len != new->fh_len)
+		return false;
+
+	if (fanotify_event_has_path(old)) {
+		return old->path.mnt == new->path.mnt &&
+			old->path.dentry == new->path.dentry;
+	} else if (fanotify_event_has_fid(old)) {
+		return fanotify_fid_equal(&old->fid, &new->fid, old->fh_len);
+	}
+
+	/* Do not merge events if we failed to encode fid */
 	return false;
 }
 
@@ -143,6 +152,57 @@  static u32 fanotify_group_event_mask(struct fsnotify_iter_info *iter_info,
 		~marks_ignored_mask;
 }
 
+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 (bytes > FANOTIFY_INLINE_FH_LEN) {
+		/* 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 = FILEID_ROOT;
 		event->path = *path;
 		path_get(&event->path);
 	} else {
+		event->fh_type = FILEID_INVALID;
 		event->path.mnt = NULL;
 		event->path.dentry = NULL;
 	}
@@ -281,7 +347,10 @@  static void fanotify_free_event(struct fsnotify_event *fsn_event)
 	struct fanotify_event *event;
 
 	event = FANOTIFY_E(fsn_event);
-	path_put(&event->path);
+	if (fanotify_event_has_path(event))
+		path_put(&event->path);
+	else if (fanotify_event_has_ext_fh(event))
+		kfree(event->fid.ext_fh);
 	put_pid(event->pid);
 	if (fanotify_is_perm_event(event->mask)) {
 		kmem_cache_free(fanotify_perm_event_cachep,
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 898b5b2bc1c7..271482fb9611 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -2,11 +2,49 @@ 
 #include <linux/fsnotify_backend.h>
 #include <linux/path.h>
 #include <linux/slab.h>
+#include <linux/exportfs.h>
 
 extern struct kmem_cache *fanotify_mark_cache;
 extern struct kmem_cache *fanotify_event_cachep;
 extern struct kmem_cache *fanotify_perm_event_cachep;
 
+/*
+ * 3 dwords are sufficient for most local fs (64bit ino, 32bit generation).
+ * For 32bit arch, fid increases the size of fanotify_event by 12 bytes and
+ * fh_* fields increase the size of fanotify_event by another 4 bytes.
+ * For 64bit arch, fid increases the size of fanotify_fid by 8 bytes and
+ * fh_* fields are packed in a hole after mask.
+ */
+#if BITS_PER_LONG == 32
+#define FANOTIFY_INLINE_FH_LEN	(3 << 2)
+#else
+#define FANOTIFY_INLINE_FH_LEN	(4 << 2)
+#endif
+
+struct fanotify_fid {
+	__kernel_fsid_t fsid;
+	union {
+		unsigned char fh[FANOTIFY_INLINE_FH_LEN];
+		unsigned char *ext_fh;
+	};
+};
+
+static inline void *fanotify_fid_fh(struct fanotify_fid *fid,
+				    unsigned int fh_len)
+{
+	return fh_len <= FANOTIFY_INLINE_FH_LEN ? fid->fh : fid->ext_fh;
+}
+
+static inline bool fanotify_fid_equal(struct fanotify_fid *fid1,
+				      struct fanotify_fid *fid2,
+				      unsigned int fh_len)
+{
+	return fid1->fsid.val[0] == fid2->fsid.val[0] &&
+		fid1->fsid.val[1] == fid2->fsid.val[1] &&
+		!memcmp(fanotify_fid_fh(fid1, fh_len),
+			fanotify_fid_fh(fid2, fh_len), fh_len);
+}
+
 /*
  * Structure for normal fanotify events. It gets allocated in
  * fanotify_handle_event() and freed when the information is retrieved by
@@ -16,13 +54,47 @@  struct fanotify_event {
 	struct fsnotify_event fse;
 	u32 mask;
 	/*
-	 * We hold ref to this path so it may be dereferenced at any point
-	 * during this object's lifetime
+	 * Those fields are outside fanotify_fid to pack fanotify_event nicely
+	 * on 64bit arch and to use fh_type as an indication of whether path
+	 * or fid are used in the union:
+	 * FILEID_ROOT (0) for path, > 0 for fid, FILEID_INVALID for neither.
 	 */
-	struct path path;
+	u8 fh_type;
+	u8 fh_len;
+	u16 pad;
+	union {
+		/*
+		 * We hold ref to this path so it may be dereferenced at any
+		 * point during this object's lifetime
+		 */
+		struct path path;
+		/*
+		 * With FAN_REPORT_FID, we do not hold any reference on the
+		 * victim object. Instead we store its NFS file handle and its
+		 * filesystem's fsid as a unique identifier.
+		 */
+		struct fanotify_fid fid;
+	};
 	struct pid *pid;
 };
 
+static inline bool fanotify_event_has_path(struct fanotify_event *event)
+{
+	return event->fh_type == FILEID_ROOT;
+}
+
+static inline bool fanotify_event_has_fid(struct fanotify_event *event)
+{
+	return event->fh_type != FILEID_ROOT &&
+		event->fh_type != FILEID_INVALID;
+}
+
+static inline bool fanotify_event_has_ext_fh(struct fanotify_event *event)
+{
+	return fanotify_event_has_fid(event) &&
+		event->fh_len > FANOTIFY_INLINE_FH_LEN;
+}
+
 /*
  * Structure for permission fanotify events. It gets allocated and freed in
  * fanotify_handle_event() since we wait there for user response. When the
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 2f4901ac090c..68a1cb18ddd8 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -181,7 +181,7 @@  static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	struct fanotify_event_metadata metadata;
 	struct fanotify_event *event;
 	struct file *f;
-	int fd, ret;
+	int ret, fd = FAN_NOFD;
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, fsn_event);
 
@@ -193,9 +193,7 @@  static ssize_t copy_event_to_user(struct fsnotify_group *group,
 	metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
 	metadata.pid = pid_vnr(event->pid);
 
-	if (unlikely(event->mask & FAN_Q_OVERFLOW)) {
-		fd = FAN_NOFD;
-	} else {
+	if (fanotify_event_has_path(event)) {
 		fd = create_fd(group, event, &f);
 		if (fd < 0)
 			return fd;
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 909c98fcace2..d07f3cbc2786 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -44,6 +44,7 @@ 
 
 /* Flags to determine fanotify event format */
 #define FAN_REPORT_TID		0x00000100	/* event->pid is thread id */
+#define FAN_REPORT_FID		0x00000200	/* Report unique file id */
 
 /* Deprecated - do not use this in programs and do not add new flags here! */
 #define FAN_ALL_INIT_FLAGS	(FAN_CLOEXEC | FAN_NONBLOCK | \