fanotify: pre-approve listener's OPEN_PERM access requests
Subsystems (3)
PATCH 1/1 fanotify: pre-approve listener's OPEN_PERM access requests
From: Steve Grubb <[email protected]>
Hello,
If a daemon using FANOTIFY needs to open a file on a watched filesystem and its wanting OPEN_PERM events, we get deadlock. (This could happen because of a library the daemon is using suddenly decides it needs to look in a new file.) Even though the man page says that the daemon should approve its own access decision, it really can't. If its in the middle of working on a request and that in turn generates another request, the second request is going to sit in the queue inside the kernel and not be read because the daemon is waiting on a library call that will never finish. We also have no idea how many requests are stacked up before we get to it. So, it really can't approve its own access requests.
The solution is to assume that the daemon is going to approve its own file access requests. So, any requested access that matches the pid of the program receiving fanotify events should be pre-approved in the kernel and not sent to user space for approval. This should prevent deadlock.
This behavior only exists if FAN_SELF_APPROVE is in the flags at fanotify_init() time.
[Eric Sandeen: Make behavior contingent on fanotify_init flag]
---
Resending this; first submission to lkml generated no responses, but offline Eric Paris indicated that the original patch was "policy in the kernel," so I'll see what people think of making it contingent on an fanotify_init flag at syscall time.
fs/notify/fanotify/fanotify.c +13 -0
@@ -105,6 +105,8 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark, {__u32 marks_mask, marks_ignored_mask;struct path *path = data; +pid_t grp_pid; +struct pid *cur_pid; pr_debug("%s: inode_mark=%p vfsmnt_mark=%p mask=%x data=%p" " data_type=%d\n", __func__, inode_mark, vfsmnt_mark,
@@ -139,6 +141,17 @@ static bool fanotify_should_send_event(struct fsnotify_mark *inode_mark,BUG (); } +/* Auto-approve the listening process's own requests if asked to */ + grp_pid =pid_nr (vfsmnt_mark->group->fanotify_data.pid); +if (grp_pid) { + cur_pid =get_pid (task_tgid (current)); +if (grp_pid ==pid_nr (cur_pid)) { +put_pid (cur_pid); +return false; + } +put_pid (cur_pid); + } +if (d_is_dir (path->dentry) && !(marks_mask &FS_ISDIR & ~marks_ignored_mask))return false;
fs/notify/fanotify/fanotify_user.c +7 -0
@@ -387,6 +387,8 @@ static int fanotify_release(struct inode *ignored, struct file *file)*/ wake_up (&group->fanotify_data.access_waitq);#endif +/* Get rid of reference held since fanotify_init */ +put_pid (group->fanotify_data.pid);/* matches the fanotify_init->fsnotify_alloc_group */ fsnotify_destroy_group (group);
@@ -741,6 +743,11 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags) group->fanotify_data.user = user;atomic_inc (&user->fanotify_listeners); +if (flags &FAN_SELF_APPROVE ) + group->fanotify_data.pid =get_pid (task_tgid (current)); +else + group->fanotify_data.pid =0 ; + oevent =fanotify_alloc_event (NULL ,FS_Q_OVERFLOW ,NULL );if (unlikely (!oevent)) { fd = -ENOMEM ;
include/linux/fsnotify_backend.h +2 -0
@@ -16,6 +16,7 @@#include <linux/spinlock.h> #include <linux/types.h> #include <linux/atomic.h> +#include <linux/pid.h> /* * IN_* from inotfy.h lines up EXACTLY with FS_*, this is so we can easily
@@ -184,6 +185,7 @@ struct fsnotify_group {int f_flags;unsigned int max_marks;struct user_struct *user; + struct pid *pid; } fanotify_data;#endif /* CONFIG_FANOTIFY */ };
include/uapi/linux/fanotify.h +1 -0
@@ -35,6 +35,7 @@#define FAN_UNLIMITED_QUEUE 0x00000010#define FAN_UNLIMITED_MARKS 0x00000020 +#define FAN_SELF_APPROVE 0x00000040/* listener pid auto-approved */ #define FAN_ALL_INIT_FLAGS (FAN_CLOEXEC | FAN_NONBLOCK | \ FAN_ALL_CLASS_BITS | FAN_UNLIMITED_QUEUE |\
Jan Kara
maintainerHi,
Quoted 23 lines · Eric Sandeen
From: Steve Grubb <[email protected]>
If a daemon using FANOTIFY needs to open a file on a watched filesystem and its wanting
OPEN_PERMevents, we get deadlock. (This could happen because of a library the daemon is using suddenly decides it needs to look in a new file.) Even though the man page says that the daemon should approve its own access decision, it really can't. If its in the middle of working on a request and that in turn generates another request, the second request is going to sit in the queue inside the kernel and not be read because the daemon is waiting on a library call that will never finish. We also have no idea how many requests are stacked up before we get to it. So, it really can't approve its own access requests.The solution is to assume that the daemon is going to approve its own file access requests. So, any requested access that matches the pid of the program receiving fanotify events should be pre-approved in the kernel and not sent to user space for approval. This should prevent deadlock.
This behavior only exists if
FAN_SELF_APPROVEis in the flags atfanotify_init()time.[Eric Sandeen: Make behavior contingent on
fanotify_initflag]
Quoted 8 lines
Signed-off-by Steve Grubb <[email protected]>Signed-off-by Eric Sandeen <[email protected]>---
Resending this; first submission to lkml generated no responses, but offline Eric Paris indicated that the original patch was "policy in the kernel," so I'll see what people think of making it contingent on an
fanotify_initflag at syscall time.
AKPM is merging fanotify patches these days so it is good to CC him. I'm keeping eye on the notification infrastructure so you can CC me when you need an opinion.
Now to the patch: This solution really looks half baked to me. What if there are two processes mediating the access? You'll get the deadlock
again: We have processes A and B mediating access. A accesses some file f, A selfapproves the event for itself but the access is still blocked on the approval from B. B proceeds to process the access request by A. It accesses some file which generates the permission event - now B is blocked waiting for A to approve its event. Dang.
This really resembles a situation when e.g. multipathd must be damn carful not to generate any IO while it is running lest it deadlocks while reconfiguring paths. So here you have to be damn careful not to generate any events when processing fanotify permission events... And I know that is inconvenient but that's how things were designed and duck taping some special cases IMHO is not acceptable.
One solution which would look reasonably clean to me would be that a process could have a capability which when set would mean that accesses by that process would not generate fanotify permission events. This would really avoid the deadlocks. But then botnet / virus writers would quickly learn to set this capability for their processes so I'm not sure if this doesn't somewhat defeat the purpose of permission events. OTOH setting this capability would be gated by CAP_SYS_ADMIN anyway and once you have this you have other ways to circumvent any protection so it is not catastrophical. But still it makes life easier for the bad guys.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 39 lines · Jan Kara
Hi,
Quoted 27 lines · Eric Sandeen
From: Steve Grubb <[email protected]>
If a daemon using FANOTIFY needs to open a file on a watched filesystem and
its wantingOPEN_PERMevents, we get deadlock. (This could happen because of a library the daemon is using suddenly decides it needs to look in a new
file.) Even though the man page says that the daemon should approve its own
access decision, it really can't. If its in the middle of working on a request and that in turn generates another request, the second request is going to sit in the queue inside the kernel and not be read because the daemon is waiting on a library call that will never finish. We also have no
idea how many requests are stacked up before we get to it. So, it really can't approve its own access requests.The solution is to assume that the daemon is going to approve its own file access requests. So, any requested access that matches the pid of the program receiving fanotify events should be pre-approved in the kernel and not sent to user space for approval. This should prevent deadlock.
This behavior only exists if
FAN_SELF_APPROVEis in the flags atfanotify_init()time.[Eric Sandeen: Make behavior contingent on
fanotify_initflag]Quoted 8 lines
Signed-off-by Steve Grubb <[email protected]>Signed-off-by Eric Sandeen <[email protected]>---
Resending this; first submission to lkml generated no responses, but offline Eric Paris indicated that the original patch was "policy in the kernel," so I'll see what people think of making it contingent on an
fanotify_initflag at syscall time.
Quoted 6 lines
AKPM is merging fanotify patches these days so it is good to CC him. I'm keeping eye on the notification infrastructure so you can CC me when you need an opinion.
Now to the patch: This solution really looks half baked to me. What if there are two processes mediating the access?
While this is certainly possible, is this actually done in real life?
Quoted 6 lines
You'll get the deadlock again: We have processes A and B mediating access. A accesses some file f, A selfapproves the event for itself but the access is still blocked on the approval from B. B proceeds to process the access request by A. It accesses some file which generates the permission event - now B is blocked waiting for A to approve its event. Dang.
Quoted 6 lines
This really resembles a situation when e.g. multipathd must be damn carful not to generate any IO while it is running lest it deadlocks while reconfiguring paths. So here you have to be damn careful not to generate any events when processing fanotify permission events... And I know that is inconvenient but that's how things were designed and duck taping some special cases IMHO is not acceptable.
The issue here is that any relatively interesting program will have several libraries who could at any time decide it needs to open a file. Perhaps even /etc/hosts for network name resolution. You really don't know what the third party libraries might do and the consequences are pretty severe - deadlock.
You could make it multi-threaded and have one thread dequeuing approval requests and another servicing them. But...each request has a live descriptor that counts towards the rlimit for max open descriptors. Hitting that is bad also.
The simplest solution is to assume that the daemon is going to approve its own request. It would never refuse its own request, should it?
-Steve
Quoted 11 lines
One solution which would look reasonably clean to me would be that a process could have a capability which when set would mean that accesses by that process would not generate fanotify permission events. This would really avoid the deadlocks. But then botnet / virus writers would quickly learn to set this capability for their processes so I'm not sure if this doesn't somewhat defeat the purpose of permission events. OTOH setting this capability would be gated by
CAP_SYS_ADMINanyway and once you have this you have other ways to circumvent any protection so it is not catastrophical. But still it makes life easier for the bad guys.Honza
On 30.03.2016 20:47, Steve Grubb wrote:
Hi,
Quoted 4 lines
Quoted 2 lines
Now to the patch: This solution really looks half baked to me. What if there are two processes mediating the access?
While this is certainly possible, is this actually done in real life?
we actually DID do this. We did not use fanotify though, since this did not exist then, but dazukoFS a stackable filesystem which worked similar to fanotify.
If you want to scan files e.g for viruses you want to have it done as fast as possible and multiple scanner (processes) is what you need then.
Quoted 20 lines
Quoted 8 lines
You'll get the deadlock again: We have processes A and B mediating access. A accesses some file f, A selfapproves the event for itself but the access is still blocked on the approval from B. B proceeds to process the access request by A. It accesses some file which generates the permission event - now B is blocked waiting for A to approve its event. Dang.
Quoted 6 lines
This really resembles a situation when e.g. multipathd must be damn carful not to generate any IO while it is running lest it deadlocks while reconfiguring paths. So here you have to be damn careful not to generate any events when processing fanotify permission events... And I know that is inconvenient but that's how things were designed and duck taping some special cases IMHO is not acceptable.
The issue here is that any relatively interesting program will have several libraries who could at any time decide it needs to open a file. Perhaps even
/etc/hostsfor network name resolution. You really don't know what the third party libraries might do and the consequences are pretty severe - deadlock.
Quoted 8 lines
You could make it multi-threaded and have one thread dequeuing approval requests and another servicing them. But...each request has a live descriptor that counts towards the rlimit for max open descriptors. Hitting that is bad also.
The simplest solution is to assume that the daemon is going to approve its own request. It would never refuse its own request, should it?
I have thought about this problem long ago when fanotify came out, because dazukoFS actually provided the possibility to set arbitrary processes to an "ignore" list exactly for this reason and I wondered how we would handle this with fanotify.
But then I came to the result that all you need to do is make sure that you have one dedicated process which does nothing else than approving (or denying) file accesses. If that process knows the pid of the processes that handle the file accesses (in our case the file scanner processes) it could simply ack all accesses done by them (note that the pid of the file accessing process is part of an fanotify event). For all file accesses done by unknown pids it would have to wait for an ACK from the scanner processes. Sure, that would require quite some interprocess communication between scanner processes and the "approver process" but it should work.
Regards,
Lino
Jan Kara
maintainerQuoted 38 lines · Steve Grubb
Quoted 37 lines · Jan Kara
Quoted 35 lines · Eric Sandeen
From: Steve Grubb <[email protected]>
If a daemon using FANOTIFY needs to open a file on a watched filesystem and
its wantingOPEN_PERMevents, we get deadlock. (This could happen because of a library the daemon is using suddenly decides it needs to look in a new
file.) Even though the man page says that the daemon should approve its own
access decision, it really can't. If its in the middle of working on a request and that in turn generates another request, the second request is going to sit in the queue inside the kernel and not be read because the daemon is waiting on a library call that will never finish. We also have no
idea how many requests are stacked up before we get to it. So, it really can't approve its own access requests.The solution is to assume that the daemon is going to approve its own file access requests. So, any requested access that matches the pid of the program receiving fanotify events should be pre-approved in the kernel and not sent to user space for approval. This should prevent deadlock.
This behavior only exists if
FAN_SELF_APPROVEis in the flags atfanotify_init()time.[Eric Sandeen: Make behavior contingent on
fanotify_initflag]Signed-off-by Steve Grubb <[email protected]>Signed-off-by Eric Sandeen <[email protected]>---
Resending this; first submission to lkml generated no responses, but offline Eric Paris indicated that the original patch was "policy in the kernel," so I'll see what people think of making it contingent on an
fanotify_initflag at syscall time.
Quoted 8 lines
Quoted 6 lines
AKPM is merging fanotify patches these days so it is good to CC him. I'm keeping eye on the notification infrastructure so you can CC me when you need an opinion.
Now to the patch: This solution really looks half baked to me. What if there are two processes mediating the access?
While this is certainly possible, is this actually done in real life?
Honestly, I don't know. But it doesn't seem like too exotic configuration to me. Think for example about containers. It just seems as a bad design to restrict fanotify permission events to: "only one process in the system can safely use this". That has always proven to be a bad idea in the long run.
Quoted 18 lines
Quoted 6 lines
You'll get the deadlock again: We have processes A and B mediating access. A accesses some file f, A selfapproves the event for itself but the access is still blocked on the approval from B. B proceeds to process the access request by A. It accesses some file which generates the permission event - now B is blocked waiting for A to approve its event. Dang.
Quoted 6 lines
This really resembles a situation when e.g. multipathd must be damn carful not to generate any IO while it is running lest it deadlocks while reconfiguring paths. So here you have to be damn careful not to generate any events when processing fanotify permission events... And I know that is inconvenient but that's how things were designed and duck taping some special cases IMHO is not acceptable.
The issue here is that any relatively interesting program will have several libraries who could at any time decide it needs to open a file. Perhaps even
/etc/hostsfor network name resolution. You really don't know what the third party libraries might do and the consequences are pretty severe - deadlock.
Quoted 4 lines
You could make it multi-threaded and have one thread dequeuing approval requests and another servicing them. But...each request has a live descriptor that counts towards the rlimit for max open descriptors. Hitting that is bad also.
I agree this reduces the attractivity of the two thread design.
Quoted 2 lines
The simplest solution is to assume that the daemon is going to approve its own request. It would never refuse its own request, should it?
I agree this is a solution which fixes your usecase but I don't think it is good enough for general use. I won't accept anything that doesn't work for several users of fanotify permission events. As I wrote below in my original email, you can use for example capabilities to make things work for several processes without too big hassle...
Quoted 9 lines
Quoted 9 lines
One solution which would look reasonably clean to me would be that a process could have a capability which when set would mean that accesses by that process would not generate fanotify permission events. This would really avoid the deadlocks. But then botnet / virus writers would quickly learn to set this capability for their processes so I'm not sure if this doesn't somewhat defeat the purpose of permission events. OTOH setting this capability would be gated by
CAP_SYS_ADMINanyway and once you have this you have other ways to circumvent any protection so it is not catastrophical. But still it makes life easier for the bad guys.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR