Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context

On Tue, May 17, 2022 at 1:32 PM Jan Kara <jack(a)suse.cz&gt; wrote: > > On Tue 17-05-22 08:37:28, Amir Goldstein wrote: > > On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > > > > > > This patch adds 2 structure members to the response returned from user > > > space on a permission event. The first field is 32 bits for the context > > > type. The context type will describe what the meaning is of the second > > > field. The default is none. The patch defines one additional context > > > type which means that the second field is a union containing a 32-bit > > > rule number. This will allow for the creation of other context types in > > > the future if other users of the API identify different needs. The > > > second field size is defined by the context type and can be used to pass > > > along the data described by the context. > > > > > > To support this, there is a macro for user space to check that the data > > > being sent is valid. Of course, without this check, anything that > > > overflows the bit field will trigger an EINVAL based on the use of > > > FAN_INVALID_RESPONSE_MASK in process_access_response(). > > > > > > Suggested-by: Steve Grubb <sgrubb(a)redhat.com&gt; > > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2 > > > Suggested-by: Jan Kara <jack(a)suse.cz&gt; > > > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz > > > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > > ... > > > static int process_access_response(struct fsnotify_group *group, > > > - struct fanotify_response *response_struct) > > > + struct fanotify_response *response_struct, > > > + size_t count) > > > { > > > struct fanotify_perm_event *event; > > > int fd = response_struct->fd; > > > u32 response = response_struct->response; > > > > > > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group, > > > - fd, response); > > > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__, > > > + group, fd, response, response_struct->extra_info_type, count); > > > + if (fd < 0) > > > + return -EINVAL; > > > /* > > > * make sure the response is valid, if invalid we do nothing and either > > > * userspace can send a valid response or we will clean it up after the > > > * timeout > > > */ > > > - switch (response & ~FAN_AUDIT) { > > > - case FAN_ALLOW: > > > - case FAN_DENY: > > > - break; > > > - default: > > > - return -EINVAL; > > > - } > > > - > > > - if (fd < 0) > > > + if (FAN_INVALID_RESPONSE_MASK(response)) > > > > That is a logic change, because now the response value of 0 becomes valid. > > > > Since you did not document this change in the commit message I assume this was > > non intentional? > > However, this behavior change is something that I did ask for, but it should be > > done is a separate commit: > > > > /* These are NOT bitwise flags. Both bits can be used together. */ > > #define FAN_TEST 0x00 > > #define FAN_ALLOW 0x01 > > #define FAN_DENY 0x02 > > #define FANOTIFY_RESPONSE_ACCESS \ > > (FAN_TEST|FAN_ALLOW | FAN_DENY) > > > > ... > > int access = response & FANOTIFY_RESPONSE_ACCESS; > > > > 1. Do return EINVAL for access == 0 > > 2. Let all the rest of the EINVAL checks run (including extra type) > > 3. Move if (fd < 0) to last check > > 4. Add if (!access) return 0 before if (fd < 0) > > > > That will provide a mechanism for userspace to probe the > > kernel support for extra types in general and specific types > > that it may respond with. > > I have to admit I didn't quite grok your suggestion here although I > understand (and agree with) the general direction of the proposal :). Maybe > code would explain it better what you have in mind? > +/* These are NOT bitwise flags. Both bits can be used together. */

+#define FAN_TEST 0x00 #define FAN_ALLOW 0x01 #define FAN_DENY 0x02 #define FAN_AUDIT 0x10 /* Bit mask to create audit record for result */ +#define FANOTIFY_RESPONSE_ACCESS \ + (FAN_TEST|FAN_ALLOW | FAN_DENY)