Re: [PATCH v5 14/23] fanotify: Encode invalid file handler when no inode is provided

Jan Kara [off-list ref] writes:

On Wed 04-08-21 12:06:03, Gabriel Krisman Bertazi wrote:

quoted

Instead of failing, encode an invalid file handler in fanotify_encode_fh
if no inode is provided.  This bogus file handler will be reported by
FAN_FS_ERROR for non-inode errors.

Also adjust the single caller that might rely on failure after passing
an empty inode.

It is not 'file handler' but rather 'file handle' - several times in the
changelog and in subject :).

quoted

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Gabriel Krisman Bertazi <redacted>
---
 fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++---------------
 fs/notify/fanotify/fanotify.h |  6 ++++--
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 0d6ba218bc01..456c60107d88 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c

@@ -349,12 +349,6 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	void *buf = fh->buf;
 	int err;
 
-	fh->type = FILEID_ROOT;
-	fh->len = 0;
-	fh->flags = 0;
-	if (!inode)
-		return 0;
-

I'd keep the fh->flags initialization here. Otherwise it will not be
initialized on some error returns.

quoted

@@ -363,8 +357,9 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 	if (fh_len < 4 || WARN_ON_ONCE(fh_len % 4))
 		goto out_err;
 
-	/* No external buffer in a variable size allocated fh */
-	if (gfp && fh_len > FANOTIFY_INLINE_FH_LEN) {
+	fh->flags = 0;
+	/* No external buffer in a variable size allocated fh or null fh */
+	if (inode && gfp && fh_len > FANOTIFY_INLINE_FH_LEN) {
 		/* Treat failure to allocate fh as failure to encode fh */
 		err = -ENOMEM;
 		ext_buf = kmalloc(fh_len, gfp);

@@ -376,14 +371,24 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 		fh->flags |= FANOTIFY_FH_FLAG_EXT_BUF;
 	}
 
-	dwords = fh_len >> 2;
-	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
-	err = -EINVAL;
-	if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
-		goto out_err;
-
-	fh->type = type;
-	fh->len = fh_len;
+	if (inode) {
+		dwords = fh_len >> 2;
+		type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
+		err = -EINVAL;
+		if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
+			goto out_err;
+		fh->type = type;
+		fh->len = fh_len;
+	} else {
+		/*
+		 * Invalid FHs are used on FAN_FS_ERROR for errors not
+		 * linked to any inode. Caller needs to guarantee the fh
+		 * has at least FANOTIFY_NULL_FH_LEN bytes of space.
+		 */
+		fh->type = FILEID_INVALID;
+		fh->len = FANOTIFY_NULL_FH_LEN;
+		memset(buf, 0, FANOTIFY_NULL_FH_LEN);
+	}

Maybe it will become clearer later during the series but why do you set
fh->len to FANOTIFY_NULL_FH_LEN and not 0?

Jan,

That is how we encode a NULL file handle (i.e. superblock error).  Amir
suggested it would be an invalid FILEID_INVALID, with a zeroed handle of
size 8.  I will improve the comment on the next iteration.

-- 
Gabriel Krisman Bertazi

On Wed 11-08-21 17:12:05, Gabriel Krisman Bertazi wrote:

Jan Kara [off-list ref] writes:

quoted

quoted

@@ -376,14 +371,24 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
 		fh->flags |= FANOTIFY_FH_FLAG_EXT_BUF;
 	}
 
-	dwords = fh_len >> 2;
-	type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
-	err = -EINVAL;
-	if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
-		goto out_err;
-
-	fh->type = type;
-	fh->len = fh_len;
+	if (inode) {
+		dwords = fh_len >> 2;
+		type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
+		err = -EINVAL;
+		if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
+			goto out_err;
+		fh->type = type;
+		fh->len = fh_len;
+	} else {
+		/*
+		 * Invalid FHs are used on FAN_FS_ERROR for errors not
+		 * linked to any inode. Caller needs to guarantee the fh
+		 * has at least FANOTIFY_NULL_FH_LEN bytes of space.
+		 */
+		fh->type = FILEID_INVALID;
+		fh->len = FANOTIFY_NULL_FH_LEN;
+		memset(buf, 0, FANOTIFY_NULL_FH_LEN);
+	}

Maybe it will become clearer later during the series but why do you set
fh->len to FANOTIFY_NULL_FH_LEN and not 0?

Jan,

That is how we encode a NULL file handle (i.e. superblock error).  Amir
suggested it would be an invalid FILEID_INVALID, with a zeroed handle of
size 8.  I will improve the comment on the next iteration.

Thanks for info. Then I have a question for Amir I guess :) Amir, what's
the advantage of zeroed handle of size 8 instead of just 0 length file
handle?

								Honza

-- 
Jan Kara [off-list ref]
SUSE Labs, CR

On Thu, Aug 12, 2021 at 5:20 PM Jan Kara [off-list ref] wrote:

On Wed 11-08-21 17:12:05, Gabriel Krisman Bertazi wrote:

quoted

Jan Kara [off-list ref] writes:

quoted

quoted

@@ -376,14 +371,24 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
           fh->flags |= FANOTIFY_FH_FLAG_EXT_BUF;
   }

-  dwords = fh_len >> 2;
-  type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
-  err = -EINVAL;
-  if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
-          goto out_err;
-
-  fh->type = type;
-  fh->len = fh_len;
+  if (inode) {
+          dwords = fh_len >> 2;
+          type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
+          err = -EINVAL;
+          if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
+                  goto out_err;
+          fh->type = type;
+          fh->len = fh_len;
+  } else {
+          /*
+           * Invalid FHs are used on FAN_FS_ERROR for errors not
+           * linked to any inode. Caller needs to guarantee the fh
+           * has at least FANOTIFY_NULL_FH_LEN bytes of space.
+           */
+          fh->type = FILEID_INVALID;
+          fh->len = FANOTIFY_NULL_FH_LEN;
+          memset(buf, 0, FANOTIFY_NULL_FH_LEN);
+  }

Maybe it will become clearer later during the series but why do you set
fh->len to FANOTIFY_NULL_FH_LEN and not 0?

Jan,

That is how we encode a NULL file handle (i.e. superblock error).  Amir
suggested it would be an invalid FILEID_INVALID, with a zeroed handle of
size 8.  I will improve the comment on the next iteration.

Thanks for info. Then I have a question for Amir I guess :) Amir, what's
the advantage of zeroed handle of size 8 instead of just 0 length file
handle?

With current code, zero fh->len means we are not reporting an FID info
record (e.g. due to encode error), see copy_info_records_to_user().

This is because fh->len plays a dual role for indicating the length of the
file handle and the existence of FID info.

I figured that keeping a positive length for the special NULL_FH is an
easy way to workaround this ambiguity and keep the code simpler.
We don't really need to pay any cost for keeping the 8 bytes zero buffer.

Thanks,
Amir.

On Thu 12-08-21 18:17:10, Amir Goldstein wrote:

On Thu, Aug 12, 2021 at 5:20 PM Jan Kara [off-list ref] wrote:

quoted

On Wed 11-08-21 17:12:05, Gabriel Krisman Bertazi wrote:

quoted

Jan Kara [off-list ref] writes:

quoted

quoted

@@ -376,14 +371,24 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
           fh->flags |= FANOTIFY_FH_FLAG_EXT_BUF;
   }

-  dwords = fh_len >> 2;
-  type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
-  err = -EINVAL;
-  if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
-          goto out_err;
-
-  fh->type = type;
-  fh->len = fh_len;
+  if (inode) {
+          dwords = fh_len >> 2;
+          type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
+          err = -EINVAL;
+          if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
+                  goto out_err;
+          fh->type = type;
+          fh->len = fh_len;
+  } else {
+          /*
+           * Invalid FHs are used on FAN_FS_ERROR for errors not
+           * linked to any inode. Caller needs to guarantee the fh
+           * has at least FANOTIFY_NULL_FH_LEN bytes of space.
+           */
+          fh->type = FILEID_INVALID;
+          fh->len = FANOTIFY_NULL_FH_LEN;
+          memset(buf, 0, FANOTIFY_NULL_FH_LEN);
+  }

Maybe it will become clearer later during the series but why do you set
fh->len to FANOTIFY_NULL_FH_LEN and not 0?

Jan,

That is how we encode a NULL file handle (i.e. superblock error).  Amir
suggested it would be an invalid FILEID_INVALID, with a zeroed handle of
size 8.  I will improve the comment on the next iteration.

Thanks for info. Then I have a question for Amir I guess :) Amir, what's
the advantage of zeroed handle of size 8 instead of just 0 length file
handle?

With current code, zero fh->len means we are not reporting an FID info
record (e.g. due to encode error), see copy_info_records_to_user().

This is because fh->len plays a dual role for indicating the length of the
file handle and the existence of FID info.

I see, thanks for info.

I figured that keeping a positive length for the special NULL_FH is an
easy way to workaround this ambiguity and keep the code simpler.
We don't really need to pay any cost for keeping the 8 bytes zero buffer.

There are two separate questions:
1) How do we internally propagate the information that we don't have
file_handle to report but we do want fsid reported.
2) What do we report to userspace in file_handle.

For 2) I think we should report fsid + FILEID_INVALID, 0-length filehandle.
Currently the non-zero lenght FILEID_INVALID filehandle was propagating to
userspace and IMO that's confusing. For 1), whatever is the simplest to
propagate the information "we want only fsid reported" internally is fine
by me. 

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR

On Fri, Aug 13, 2021 at 3:09 PM Jan Kara [off-list ref] wrote:

On Thu 12-08-21 18:17:10, Amir Goldstein wrote:

quoted

On Thu, Aug 12, 2021 at 5:20 PM Jan Kara [off-list ref] wrote:

quoted

On Wed 11-08-21 17:12:05, Gabriel Krisman Bertazi wrote:

quoted

Jan Kara [off-list ref] writes:

quoted

quoted

@@ -376,14 +371,24 @@ static int fanotify_encode_fh(struct fanotify_fh *fh, struct inode *inode,
           fh->flags |= FANOTIFY_FH_FLAG_EXT_BUF;
   }

-  dwords = fh_len >> 2;
-  type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
-  err = -EINVAL;
-  if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
-          goto out_err;
-
-  fh->type = type;
-  fh->len = fh_len;
+  if (inode) {
+          dwords = fh_len >> 2;
+          type = exportfs_encode_inode_fh(inode, buf, &dwords, NULL);
+          err = -EINVAL;
+          if (!type || type == FILEID_INVALID || fh_len != dwords << 2)
+                  goto out_err;
+          fh->type = type;
+          fh->len = fh_len;
+  } else {
+          /*
+           * Invalid FHs are used on FAN_FS_ERROR for errors not
+           * linked to any inode. Caller needs to guarantee the fh
+           * has at least FANOTIFY_NULL_FH_LEN bytes of space.
+           */
+          fh->type = FILEID_INVALID;
+          fh->len = FANOTIFY_NULL_FH_LEN;
+          memset(buf, 0, FANOTIFY_NULL_FH_LEN);
+  }

Maybe it will become clearer later during the series but why do you set
fh->len to FANOTIFY_NULL_FH_LEN and not 0?

Jan,

That is how we encode a NULL file handle (i.e. superblock error).  Amir
suggested it would be an invalid FILEID_INVALID, with a zeroed handle of
size 8.  I will improve the comment on the next iteration.

Thanks for info. Then I have a question for Amir I guess :) Amir, what's
the advantage of zeroed handle of size 8 instead of just 0 length file
handle?

With current code, zero fh->len means we are not reporting an FID info
record (e.g. due to encode error), see copy_info_records_to_user().

This is because fh->len plays a dual role for indicating the length of the
file handle and the existence of FID info.

I see, thanks for info.

quoted

I figured that keeping a positive length for the special NULL_FH is an
easy way to workaround this ambiguity and keep the code simpler.
We don't really need to pay any cost for keeping the 8 bytes zero buffer.

There are two separate questions:
1) How do we internally propagate the information that we don't have
file_handle to report but we do want fsid reported.
2) What do we report to userspace in file_handle.

For 2) I think we should report fsid + FILEID_INVALID, 0-length filehandle.
Currently the non-zero lenght FILEID_INVALID filehandle was propagating to
userspace and IMO that's confusing.

Agree. That was implemented in v6.

For 1), whatever is the simplest to
propagate the information "we want only fsid reported" internally is fine
by me.

Ok. I think it would be fair (based on v6 patches) to call the fh->len = 4
option "simple".

But following my "simple" suggestion as is, v6 has a side effect of
adding 4 bytes of zero padding after the event fid info record.

Can you please confirm that this side effect is fine by you as well.
It wouldn't be too hard to special case FILEID_INVALID and also
truncate those 4 bytes of zero padding, but I really don't think it is
worth the effort.

Thanks,
Amir.