* fanotify permission events on virtual filesystem
@ 2019-03-20 13:16 Jan Kara
2019-03-20 13:46 ` Amir Goldstein
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2019-03-20 13:16 UTC (permalink / raw)
To: linux-fsdevel; +Cc: Amir Goldstein, mhocko, Al Viro
Hello,
recently, one of our customers has reported a deadlock with fanotify. The
analysis has shown that a process has put (likely by mistake) FAN_OPEN_PERM
mark on /proc and / filesystem. That resulted in a deadlock like follows:
process 1: process 2: process 3:
open("/proc/process 2/maps")
- blocks waiting for response to
FAN_OPEN_PERM event
exec(2)
__do_execve_file()
- grabs current->signal->cred_guard_mutex
do_open_execat()
- blocks waiting for response to
FAN_OPEN_PERM event
reads fanotify event
generated by process 1
create_fd()
dentry_open()
proc_maps_open()
blocks on
cred_guard_mutex of process 2.
Now this is not the only case where you can setup fanotify permissions
events so that your listener deadlocks but I'd argue that this case is
especially nasty and it is unrealistic to expect from userspace that it
would be able to implement fanotify listener in such a way that is
deadlock-free for proc filesystem since the lock dependencies there are
very different. So how about we just forbid placing marks with fanotify
permission events on proc? Any other virtual filesystem we should exclude?
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: fanotify permission events on virtual filesystem
2019-03-20 13:16 fanotify permission events on virtual filesystem Jan Kara
@ 2019-03-20 13:46 ` Amir Goldstein
2019-03-20 14:30 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2019-03-20 13:46 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-fsdevel, mhocko, Al Viro
On Wed, Mar 20, 2019 at 3:16 PM Jan Kara <jack@suse.cz> wrote:
>
> Hello,
>
> recently, one of our customers has reported a deadlock with fanotify. The
> analysis has shown that a process has put (likely by mistake) FAN_OPEN_PERM
> mark on /proc and / filesystem. That resulted in a deadlock like follows:
>
> process 1: process 2: process 3:
> open("/proc/process 2/maps")
> - blocks waiting for response to
> FAN_OPEN_PERM event
>
> exec(2)
> __do_execve_file()
> - grabs current->signal->cred_guard_mutex
> do_open_execat()
> - blocks waiting for response to
> FAN_OPEN_PERM event
>
> reads fanotify event
> generated by process 1
> create_fd()
> dentry_open()
> proc_maps_open()
> blocks on
> cred_guard_mutex of process 2.
>
> Now this is not the only case where you can setup fanotify permissions
> events so that your listener deadlocks but I'd argue that this case is
> especially nasty and it is unrealistic to expect from userspace that it
> would be able to implement fanotify listener in such a way that is
> deadlock-free for proc filesystem since the lock dependencies there are
> very different. So how about we just forbid placing marks with fanotify
> permission events on proc? Any other virtual filesystem we should exclude?
>
I bet if we forbid placing marks on /proc, some apps would break.
I always though that allowing O_PATH in event_f_flags can make
sense for some apps.
What if instead of forbiding marks of /proc, we would force those
marks to use O_PATH for fd creation. Some of the functionality
will remain. Apps are less likely to break. Deadlocks will be less
likely, although maybe still possible.
Note that the new FAN_REPORT_FID listener already excludes
marking most virtual filesystems for lack of s_export_op.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: fanotify permission events on virtual filesystem
2019-03-20 13:46 ` Amir Goldstein
@ 2019-03-20 14:30 ` Jan Kara
2019-03-20 15:02 ` Amir Goldstein
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2019-03-20 14:30 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel, mhocko, Al Viro
On Wed 20-03-19 15:46:20, Amir Goldstein wrote:
> On Wed, Mar 20, 2019 at 3:16 PM Jan Kara <jack@suse.cz> wrote:
> > recently, one of our customers has reported a deadlock with fanotify. The
> > analysis has shown that a process has put (likely by mistake) FAN_OPEN_PERM
> > mark on /proc and / filesystem. That resulted in a deadlock like follows:
> >
> > process 1: process 2: process 3:
> > open("/proc/process 2/maps")
> > - blocks waiting for response to
> > FAN_OPEN_PERM event
> >
> > exec(2)
> > __do_execve_file()
> > - grabs current->signal->cred_guard_mutex
> > do_open_execat()
> > - blocks waiting for response to
> > FAN_OPEN_PERM event
> >
> > reads fanotify event
> > generated by process 1
> > create_fd()
> > dentry_open()
> > proc_maps_open()
> > blocks on
> > cred_guard_mutex of process 2.
> >
> > Now this is not the only case where you can setup fanotify permissions
> > events so that your listener deadlocks but I'd argue that this case is
> > especially nasty and it is unrealistic to expect from userspace that it
> > would be able to implement fanotify listener in such a way that is
> > deadlock-free for proc filesystem since the lock dependencies there are
> > very different. So how about we just forbid placing marks with fanotify
> > permission events on proc? Any other virtual filesystem we should exclude?
> >
>
> I bet if we forbid placing marks on /proc, some apps would break.
Well, I didn't mean all marks, just the permission ones. I'm not sure there
are apps that place permission events on /proc...
> I always though that allowing O_PATH in event_f_flags can make
> sense for some apps.
>
> What if instead of forbiding marks of /proc, we would force those
> marks to use O_PATH for fd creation. Some of the functionality
> will remain. Apps are less likely to break. Deadlocks will be less
> likely, although maybe still possible.
Yes, that's another option. But if this is automatic, it is going to be
confusing to potential users - report O_PATH fd if getting normal fd is
dangerous isn't great. And also the deadlocks are there only for permission
events so there's no strong reason to restrict normal ones.
> Note that the new FAN_REPORT_FID listener already excludes
> marking most virtual filesystems for lack of s_export_op.
Yes, I know.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: fanotify permission events on virtual filesystem
2019-03-20 14:30 ` Jan Kara
@ 2019-03-20 15:02 ` Amir Goldstein
2019-03-21 8:36 ` Marko Rauhamaa
2019-04-01 17:26 ` Jan Kara
0 siblings, 2 replies; 6+ messages in thread
From: Amir Goldstein @ 2019-03-20 15:02 UTC (permalink / raw)
To: Jan Kara; +Cc: linux-fsdevel, mhocko, Al Viro, Marko Rauhamaa
On Wed, Mar 20, 2019 at 4:30 PM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 20-03-19 15:46:20, Amir Goldstein wrote:
> > On Wed, Mar 20, 2019 at 3:16 PM Jan Kara <jack@suse.cz> wrote:
> > > recently, one of our customers has reported a deadlock with fanotify. The
> > > analysis has shown that a process has put (likely by mistake) FAN_OPEN_PERM
> > > mark on /proc and / filesystem. That resulted in a deadlock like follows:
> > >
> > > process 1: process 2: process 3:
> > > open("/proc/process 2/maps")
> > > - blocks waiting for response to
> > > FAN_OPEN_PERM event
> > >
> > > exec(2)
> > > __do_execve_file()
> > > - grabs current->signal->cred_guard_mutex
> > > do_open_execat()
> > > - blocks waiting for response to
> > > FAN_OPEN_PERM event
> > >
> > > reads fanotify event
> > > generated by process 1
> > > create_fd()
> > > dentry_open()
> > > proc_maps_open()
> > > blocks on
> > > cred_guard_mutex of process 2.
> > >
> > > Now this is not the only case where you can setup fanotify permissions
> > > events so that your listener deadlocks but I'd argue that this case is
> > > especially nasty and it is unrealistic to expect from userspace that it
> > > would be able to implement fanotify listener in such a way that is
> > > deadlock-free for proc filesystem since the lock dependencies there are
> > > very different. So how about we just forbid placing marks with fanotify
> > > permission events on proc? Any other virtual filesystem we should exclude?
> > >
> >
> > I bet if we forbid placing marks on /proc, some apps would break.
>
> Well, I didn't mean all marks, just the permission ones. I'm not sure there
> are apps that place permission events on /proc...
>
Maybe not intentionally.
I once tested a few fanotify based AntiVirus solutions.
In some of them, setting an "Exclude path" on some mount point
would cause mark to not be set on that path, but for one in particular,
the mark was still being set on the mount so path pattern filtering was
done after receiving the events.
I did not check whether /proc was blacklisted out of the box or if it
could be marked/excluded from scan.
IMO, assuming that all AntiVirus vendors blacklist all virtual filesystems
is an assumption that we need to validate.
[CC Marko from F-Secure for commenting on the above.]
> > I always though that allowing O_PATH in event_f_flags can make
> > sense for some apps.
> >
> > What if instead of forbiding marks of /proc, we would force those
> > marks to use O_PATH for fd creation. Some of the functionality
> > will remain. Apps are less likely to break. Deadlocks will be less
> > likely, although maybe still possible.
>
> Yes, that's another option. But if this is automatic, it is going to be
> confusing to potential users - report O_PATH fd if getting normal fd is
> dangerous isn't great. And also the deadlocks are there only for permission
> events so there's no strong reason to restrict normal ones.
>
Of course, we can do this hack only for reading permission events
for virtual filesystems. The question is what would be more surprising
to existing apps:
- marks for permission events no longer working on /proc
- fd from permission events no longer readable from /proc
- something completely different
Thanks,
Amir.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: fanotify permission events on virtual filesystem
2019-03-20 15:02 ` Amir Goldstein
@ 2019-03-21 8:36 ` Marko Rauhamaa
2019-04-01 17:26 ` Jan Kara
1 sibling, 0 replies; 6+ messages in thread
From: Marko Rauhamaa @ 2019-03-21 8:36 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel, mhocko, Al Viro
Amir Goldstein <amir73il@gmail.com>:
> On Wed, Mar 20, 2019 at 4:30 PM Jan Kara <jack@suse.cz> wrote:
>> Well, I didn't mean all marks, just the permission ones. I'm not sure
>> there are apps that place permission events on /proc...
>
> Maybe not intentionally.
> I once tested a few fanotify based AntiVirus solutions.
> In some of them, setting an "Exclude path" on some mount point
> would cause mark to not be set on that path, but for one in particular,
> the mark was still being set on the mount so path pattern filtering was
> done after receiving the events.
> I did not check whether /proc was blacklisted out of the box or if it
> could be marked/excluded from scan.
> IMO, assuming that all AntiVirus vendors blacklist all virtual filesystems
> is an assumption that we need to validate.
> [CC Marko from F-Secure for commenting on the above.]
Yeah, we have learned by experimentation to not mark some file systems.
(Also, inspecting some /proc files *during* OPEN_PERM processing of a
regular file can lead to deadlocks.)
Marko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: fanotify permission events on virtual filesystem
2019-03-20 15:02 ` Amir Goldstein
2019-03-21 8:36 ` Marko Rauhamaa
@ 2019-04-01 17:26 ` Jan Kara
1 sibling, 0 replies; 6+ messages in thread
From: Jan Kara @ 2019-04-01 17:26 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Jan Kara, linux-fsdevel, mhocko, Al Viro, Marko Rauhamaa
On Wed 20-03-19 17:02:54, Amir Goldstein wrote:
> On Wed, Mar 20, 2019 at 4:30 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Wed 20-03-19 15:46:20, Amir Goldstein wrote:
> > > On Wed, Mar 20, 2019 at 3:16 PM Jan Kara <jack@suse.cz> wrote:
> > > > recently, one of our customers has reported a deadlock with fanotify. The
> > > > analysis has shown that a process has put (likely by mistake) FAN_OPEN_PERM
> > > > mark on /proc and / filesystem. That resulted in a deadlock like follows:
> > > >
> > > > process 1: process 2: process 3:
> > > > open("/proc/process 2/maps")
> > > > - blocks waiting for response to
> > > > FAN_OPEN_PERM event
> > > >
> > > > exec(2)
> > > > __do_execve_file()
> > > > - grabs current->signal->cred_guard_mutex
> > > > do_open_execat()
> > > > - blocks waiting for response to
> > > > FAN_OPEN_PERM event
> > > >
> > > > reads fanotify event
> > > > generated by process 1
> > > > create_fd()
> > > > dentry_open()
> > > > proc_maps_open()
> > > > blocks on
> > > > cred_guard_mutex of process 2.
> > > >
> > > > Now this is not the only case where you can setup fanotify permissions
> > > > events so that your listener deadlocks but I'd argue that this case is
> > > > especially nasty and it is unrealistic to expect from userspace that it
> > > > would be able to implement fanotify listener in such a way that is
> > > > deadlock-free for proc filesystem since the lock dependencies there are
> > > > very different. So how about we just forbid placing marks with fanotify
> > > > permission events on proc? Any other virtual filesystem we should exclude?
> > > >
> > >
> > > I bet if we forbid placing marks on /proc, some apps would break.
> >
> > Well, I didn't mean all marks, just the permission ones. I'm not sure there
> > are apps that place permission events on /proc...
> >
>
> Maybe not intentionally.
> I once tested a few fanotify based AntiVirus solutions.
> In some of them, setting an "Exclude path" on some mount point
> would cause mark to not be set on that path, but for one in particular,
> the mark was still being set on the mount so path pattern filtering was
> done after receiving the events.
> I did not check whether /proc was blacklisted out of the box or if it
> could be marked/excluded from scan.
> IMO, assuming that all AntiVirus vendors blacklist all virtual filesystems
> is an assumption that we need to validate.
> [CC Marko from F-Secure for commenting on the above.]
>
> > > I always though that allowing O_PATH in event_f_flags can make
> > > sense for some apps.
> > >
> > > What if instead of forbiding marks of /proc, we would force those
> > > marks to use O_PATH for fd creation. Some of the functionality
> > > will remain. Apps are less likely to break. Deadlocks will be less
> > > likely, although maybe still possible.
> >
> > Yes, that's another option. But if this is automatic, it is going to be
> > confusing to potential users - report O_PATH fd if getting normal fd is
> > dangerous isn't great. And also the deadlocks are there only for permission
> > events so there's no strong reason to restrict normal ones.
> >
>
> Of course, we can do this hack only for reading permission events
> for virtual filesystems. The question is what would be more surprising
> to existing apps:
> - marks for permission events no longer working on /proc
> - fd from permission events no longer readable from /proc
> - something completely different
I think that option 1) is at least very explicit failure and given what
Marko wrote I'm willing to give that a try. If people start complaining,
we'll probably have to revert and try either reporting O_PATH fd or even
FAN_NOFD. It's a bit of a gamble I agree but current status quo isn't good
either.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-01 17:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-20 13:16 fanotify permission events on virtual filesystem Jan Kara
2019-03-20 13:46 ` Amir Goldstein
2019-03-20 14:30 ` Jan Kara
2019-03-20 15:02 ` Amir Goldstein
2019-03-21 8:36 ` Marko Rauhamaa
2019-04-01 17:26 ` Jan Kara
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.