| Message ID | 20210804160612.3575505-15-krisman@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | File system wide monitoring | expand |
On Wed 04-08-21 12:06:03, Gabriel Krisman Bertazi wrote: > 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 :). > Suggested-by: Amir Goldstein <amir73il@gmail.com> > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > --- > 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. > @@ -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? Honza
Jan Kara <jack@suse.cz> writes: > On Wed 04-08-21 12:06:03, Gabriel Krisman Bertazi wrote: >> 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 :). > >> Suggested-by: Amir Goldstein <amir73il@gmail.com> >> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> >> --- >> 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. > >> @@ -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.
On Wed 11-08-21 17:12:05, Gabriel Krisman Bertazi wrote: > Jan Kara <jack@suse.cz> writes: > >> @@ -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 <jack@suse.cz> writes: > On Wed 11-08-21 17:12:05, Gabriel Krisman Bertazi wrote: >> Jan Kara <jack@suse.cz> writes: >> >> @@ -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? Jan, Looking back at the email from Amir, I realize I misunderstood his original suggestion. Amir suggested it be FILEID_INVALID with 0-len OR FILEID_INO32_GEN with zeroed fields. I mixed the two suggestions. The advantage of doing FILEID_INO32_GEN with zeroed field is to avoid special casing the test program. But I don't have a good reason to use FILEID_INVALID with a len > 0. I'm sending a v6 with everything, including this, addressed. testcase and man pages will be updated as well.
On Thu, Aug 12, 2021 at 5:20 PM Jan Kara <jack@suse.cz> wrote: > > On Wed 11-08-21 17:12:05, Gabriel Krisman Bertazi wrote: > > Jan Kara <jack@suse.cz> writes: > > >> @@ -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, Aug 12, 2021 at 6:14 PM Gabriel Krisman Bertazi <krisman@collabora.com> wrote: > > Jan Kara <jack@suse.cz> writes: > > > On Wed 11-08-21 17:12:05, Gabriel Krisman Bertazi wrote: > >> Jan Kara <jack@suse.cz> writes: > >> >> @@ -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? > > Jan, > > Looking back at the email from Amir, I realize I misunderstood his > original suggestion. Amir suggested it be FILEID_INVALID with 0-len OR > FILEID_INO32_GEN with zeroed fields. I mixed the two suggestions. > That was from a discussion about UAPI and agree it doesn't make much sense to report non-zero handle_bytes and invalid handle_type. But specifically, Jan's question above was directly referring to the internal representation of the event. Since you are going to allocate a file handle buffer of size MAX_HANDLE_SZ (right?), then there is no caveat in declaring the NULL_FH with positive size internally, which I think, simplifies the implementation. NULL_FH internal length could be 4 instead of 8 though. You will not have to special case fanotify_fid_info_len() and the info record hdr.len will include a 4 bytes zero padding after the fid info record with NULL_FH. You will only special case this line in copy_fid_info_to_user(): handle.handle_type = fh->type; if (fh->type != FILEID_INVALID) handle.handle_bytes = fh_len; Thanks, Amir.
On Thu 12-08-21 18:17:10, Amir Goldstein wrote: > On Thu, Aug 12, 2021 at 5:20 PM Jan Kara <jack@suse.cz> wrote: > > > > On Wed 11-08-21 17:12:05, Gabriel Krisman Bertazi wrote: > > > Jan Kara <jack@suse.cz> writes: > > > >> @@ -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
On Fri, Aug 13, 2021 at 3:09 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 12-08-21 18:17:10, Amir Goldstein wrote: > > On Thu, Aug 12, 2021 at 5:20 PM Jan Kara <jack@suse.cz> wrote: > > > > > > On Wed 11-08-21 17:12:05, Gabriel Krisman Bertazi wrote: > > > > Jan Kara <jack@suse.cz> writes: > > > > >> @@ -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. 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.
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; - /* * !gpf means preallocated variable size fh, but fh_len could * be zero in that case if encoding fh len failed. @@ -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); + } /* * Mix fh into event merge key. Hash might be NULL in case of @@ -511,7 +516,7 @@ static struct fanotify_event *fanotify_alloc_name_event(struct inode *id, struct fanotify_info *info; struct fanotify_fh *dfh, *ffh; unsigned int dir_fh_len = fanotify_encode_fh_len(id); - unsigned int child_fh_len = fanotify_encode_fh_len(child); + unsigned int child_fh_len = child ? fanotify_encode_fh_len(child) : 0; unsigned int size; size = sizeof(*fne) + FANOTIFY_FH_HDR_LEN + dir_fh_len; diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index 8061040f0348..aa555975c0f8 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -349,18 +349,20 @@ static inline unsigned int fanotify_event_hash_bucket( return event->hash & FANOTIFY_HTABLE_MASK; } +#define FANOTIFY_NULL_FH_LEN 8 + /* * Check size needed to encode fanotify_fh. * * Return size of encoded fh without fanotify_fh header. - * Return 0 on failure to encode. + * For a NULL inode, return the size of a NULL FH. */ static inline int fanotify_encode_fh_len(struct inode *inode) { int dwords = 0; if (!inode) - return 0; + return FANOTIFY_NULL_FH_LEN; exportfs_encode_inode_fh(inode, NULL, &dwords, NULL);
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. Suggested-by: Amir Goldstein <amir73il@gmail.com> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- fs/notify/fanotify/fanotify.c | 39 ++++++++++++++++++++--------------- fs/notify/fanotify/fanotify.h | 6 ++++-- 2 files changed, 26 insertions(+), 19 deletions(-)