On Thu 03-07-25 15:05:37, Miklos Szeredi wrote:
> This is to make it easier to debug issues with AV software, which time and
> again deadlocks with no indication of where the issue comes from, and the
> kernel being blamed for the deadlock. Then we need to analyze dumps to
> prove that the kernel is not in fact at fault.
I share the pain. I had to do quite some of these analyses myself :).
Luckily our support guys have trained to do the analysis themselves over
the years so it rarely reaches my table anymore.
> With this patch a warning is printed when permission event is received by
> userspace but not answered for more than 20 seconds.
>
> The timeout is very coarse (20-40s) but I guess it's good enough for the
> purpose.
I'm not opposed to the idea (although I agree with Amir that it should be
tunable - we have /proc/sys/fs/fanotify/ for similar things). Just I'm not
sure it will have the desired deterring effect for fanotify users wanting
to blame the kernel. What usually convinces them is showing where their
tasks supposed to write reply to permission event (i.e., those that have
corresponding event fds in their fdtable) are blocked and hence they cannot
reply. But with some education I suppose it can work. After all the
messages you print contain the task responsible to answer which is already
helpful.
> +config FANOTIFY_PERM_WATCHDOG
> + bool "fanotify permission event watchdog"
> + depends on FANOTIFY_ACCESS_PERMISSIONS
> + default n
As Amir wrote, I don't think we need a kconfig for this, configuration
through /proc/sys/fs/fanotify/ will be much more flexible.
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index b44e70e44be6..8b60fbb9594f 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -438,10 +438,14 @@ FANOTIFY_ME(struct fanotify_event *event)
> struct fanotify_perm_event {
> struct fanotify_event fae;
> struct path path;
> - const loff_t *ppos; /* optional file range info */
> + union {
> + const loff_t *ppos; /* optional file range info */
> + pid_t pid; /* pid of task processing the event */
> + };
I think Amir complained about the generic 'pid' name already. Maybe
processing_pid? Also I'd just get rid of the union. We don't have *that*
many permission events that 4 bytes would matter and possible interactions
between pre-content events and this watchdog using the same space make me
somewhat uneasy.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
