| Message ID | 20190515193337.11167-1-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: Disallow permission events for proc filesystem | expand |
On Wed, May 15, 2019 at 10:33 PM Jan Kara <jack@suse.cz> wrote: > > Proc filesystem has special locking rules for various files. Thus > fanotify which opens files on event delivery can easily deadlock > against another process that waits for fanotify permission event to be > handled. Since permission events on /proc have doubtful value anyway, > just disallow them. > Let's add context: Link: https://lore.kernel.org/linux-fsdevel/20190320131642.GE9485@quack2.suse.cz/ > Signed-off-by: Jan Kara <jack@suse.cz> > --- > fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index a90bb19dcfa2..73719949faa6 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -920,6 +920,20 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid) > return 0; > } > > +static int fanotify_events_supported(struct path *path, __u64 mask) > +{ > + /* > + * Proc is special and various files have special locking rules so > + * fanotify permission events have high chances of deadlocking the > + * system. Just disallow them. > + */ > + if (mask & FANOTIFY_PERM_EVENTS && > + !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) { Better use an SB_I_ flag to forbid permission events on fs? > + return -EOPNOTSUPP; I would go with EINVAL following precedent of per filesystem flags check on rename(2), but not insisting. Anyway, following Matthew's man page update for FAN_REPORT_FID, we should also add this as reason for EOPNOTSUPP/EINVAL. Thanks, Amir.
On Thu 16-05-19 08:54:37, Amir Goldstein wrote: > On Wed, May 15, 2019 at 10:33 PM Jan Kara <jack@suse.cz> wrote: > > > > Proc filesystem has special locking rules for various files. Thus > > fanotify which opens files on event delivery can easily deadlock > > against another process that waits for fanotify permission event to be > > handled. Since permission events on /proc have doubtful value anyway, > > just disallow them. > > > > Let's add context: > Link: https://lore.kernel.org/linux-fsdevel/20190320131642.GE9485@quack2.suse.cz/ OK, will add. > > Signed-off-by: Jan Kara <jack@suse.cz> > > --- > > fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > index a90bb19dcfa2..73719949faa6 100644 > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -920,6 +920,20 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid) > > return 0; > > } > > > > +static int fanotify_events_supported(struct path *path, __u64 mask) > > +{ > > + /* > > + * Proc is special and various files have special locking rules so > > + * fanotify permission events have high chances of deadlocking the > > + * system. Just disallow them. > > + */ > > + if (mask & FANOTIFY_PERM_EVENTS && > > + !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) { > > Better use an SB_I_ flag to forbid permission events on fs? So checking s_type->name indeed felt dirty. I don't think we need a superblock flag though. I'll probably just go with FS_XXX flag in file_system_type. > > > + return -EOPNOTSUPP; > > I would go with EINVAL following precedent of per filesystem flags > check on rename(2), but not insisting. I was undecided between EOPNOTSUPP and EINVAL. So let's go with EINVAL. > Anyway, following Matthew's man page update for FAN_REPORT_FID, > we should also add this as reason for EOPNOTSUPP/EINVAL. Good point. Thanks for review! Honza
On Thu, May 16, 2019 at 10:36:32AM +0200, Jan Kara wrote: > On Thu 16-05-19 08:54:37, Amir Goldstein wrote: > > On Wed, May 15, 2019 at 10:33 PM Jan Kara <jack@suse.cz> wrote: > > > > > > Proc filesystem has special locking rules for various files. Thus > > > fanotify which opens files on event delivery can easily deadlock > > > against another process that waits for fanotify permission event to be > > > handled. Since permission events on /proc have doubtful value anyway, > > > just disallow them. > > > > > > > Let's add context: > > Link: https://lore.kernel.org/linux-fsdevel/20190320131642.GE9485@quack2.suse.cz/ > > OK, will add. > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > --- > > > fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++ > > > 1 file changed, 20 insertions(+) > > > > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > > index a90bb19dcfa2..73719949faa6 100644 > > > --- a/fs/notify/fanotify/fanotify_user.c > > > +++ b/fs/notify/fanotify/fanotify_user.c > > > @@ -920,6 +920,20 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid) > > > return 0; > > > } > > > > > > +static int fanotify_events_supported(struct path *path, __u64 mask) > > > +{ > > > + /* > > > + * Proc is special and various files have special locking rules so > > > + * fanotify permission events have high chances of deadlocking the > > > + * system. Just disallow them. > > > + */ > > > + if (mask & FANOTIFY_PERM_EVENTS && > > > + !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) { > > > > Better use an SB_I_ flag to forbid permission events on fs? > > So checking s_type->name indeed felt dirty. I don't think we need a > superblock flag though. I'll probably just go with FS_XXX flag in > file_system_type. Would the same apply for some files that backed by sysfs and reside in /sys? > > > > > + return -EOPNOTSUPP; > > > > I would go with EINVAL following precedent of per filesystem flags > > check on rename(2), but not insisting. > > I was undecided between EOPNOTSUPP and EINVAL. So let's go with EINVAL. I was also thinking that EINVAL makes more sense in this particular case. > > Anyway, following Matthew's man page update for FAN_REPORT_FID, > > we should also add this as reason for EOPNOTSUPP/EINVAL. > > Good point. I've followed up Michael in regards to the FAN_REPORT_FID patch series, but no response as of yet. I'm happy to write the changes for this one if you like?
On Wed 22-05-19 07:57:18, Matthew Bobrowski wrote: > On Thu, May 16, 2019 at 10:36:32AM +0200, Jan Kara wrote: > > On Thu 16-05-19 08:54:37, Amir Goldstein wrote: > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > --- > > > > fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++ > > > > 1 file changed, 20 insertions(+) > > > > > > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > > > index a90bb19dcfa2..73719949faa6 100644 > > > > --- a/fs/notify/fanotify/fanotify_user.c > > > > +++ b/fs/notify/fanotify/fanotify_user.c > > > > @@ -920,6 +920,20 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid) > > > > return 0; > > > > } > > > > > > > > +static int fanotify_events_supported(struct path *path, __u64 mask) > > > > +{ > > > > + /* > > > > + * Proc is special and various files have special locking rules so > > > > + * fanotify permission events have high chances of deadlocking the > > > > + * system. Just disallow them. > > > > + */ > > > > + if (mask & FANOTIFY_PERM_EVENTS && > > > > + !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) { > > > > > > Better use an SB_I_ flag to forbid permission events on fs? > > > > So checking s_type->name indeed felt dirty. I don't think we need a > > superblock flag though. I'll probably just go with FS_XXX flag in > > file_system_type. > > Would the same apply for some files that backed by sysfs and reside in > /sys? So far I'm not aware of similar easy to trigger deadlocks with sysfs. So I opted for a cautious path and disabled permission events only for proc. We'll see how that fares. > > > > + return -EOPNOTSUPP; > > > > > > I would go with EINVAL following precedent of per filesystem flags > > > check on rename(2), but not insisting. > > > > I was undecided between EOPNOTSUPP and EINVAL. So let's go with EINVAL. > > I was also thinking that EINVAL makes more sense in this particular > case. Good, that's what I used in v2. > > > Anyway, following Matthew's man page update for FAN_REPORT_FID, > > > we should also add this as reason for EOPNOTSUPP/EINVAL. > > > > Good point. > > I've followed up Michael in regards to the FAN_REPORT_FID patch series, > but no response as of yet. I'm happy to write the changes for this one > if you like? If you had time for that, that would be nice. Thanks! Honza
On Wed, May 22, 2019 at 11:42:01AM +0200, Jan Kara wrote: > On Wed 22-05-19 07:57:18, Matthew Bobrowski wrote: > > On Thu, May 16, 2019 at 10:36:32AM +0200, Jan Kara wrote: > > > On Thu 16-05-19 08:54:37, Amir Goldstein wrote: > > > > > Signed-off-by: Jan Kara <jack@suse.cz> > > > > > --- > > > > > fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++ > > > > > 1 file changed, 20 insertions(+) > > > > > > > > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > > > > index a90bb19dcfa2..73719949faa6 100644 > > > > > --- a/fs/notify/fanotify/fanotify_user.c > > > > > +++ b/fs/notify/fanotify/fanotify_user.c > > > > > @@ -920,6 +920,20 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid) > > > > > return 0; > > > > > } > > > > > > > > > > +static int fanotify_events_supported(struct path *path, __u64 mask) > > > > > +{ > > > > > + /* > > > > > + * Proc is special and various files have special locking rules so > > > > > + * fanotify permission events have high chances of deadlocking the > > > > > + * system. Just disallow them. > > > > > + */ > > > > > + if (mask & FANOTIFY_PERM_EVENTS && > > > > > + !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) { > > > > > > > > Better use an SB_I_ flag to forbid permission events on fs? > > > > > > So checking s_type->name indeed felt dirty. I don't think we need a > > > superblock flag though. I'll probably just go with FS_XXX flag in > > > file_system_type. > > > > Would the same apply for some files that backed by sysfs and reside in > > /sys? > > So far I'm not aware of similar easy to trigger deadlocks with sysfs. So I > opted for a cautious path and disabled permission events only for proc. > We'll see how that fares. > > > > > > + return -EOPNOTSUPP; > > > > > > > > I would go with EINVAL following precedent of per filesystem flags > > > > check on rename(2), but not insisting. > > > > > > I was undecided between EOPNOTSUPP and EINVAL. So let's go with EINVAL. > > > > I was also thinking that EINVAL makes more sense in this particular > > case. > > Good, that's what I used in v2. > > > > > Anyway, following Matthew's man page update for FAN_REPORT_FID, > > > > we should also add this as reason for EOPNOTSUPP/EINVAL. > > > > > > Good point. > > > > I've followed up Michael in regards to the FAN_REPORT_FID patch series, > > but no response as of yet. I'm happy to write the changes for this one > > if you like? > > If you had time for that, that would be nice. Thanks! Simple. I will write the changes and send it through for review.
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index a90bb19dcfa2..73719949faa6 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -920,6 +920,20 @@ static int fanotify_test_fid(struct path *path, __kernel_fsid_t *fsid) return 0; } +static int fanotify_events_supported(struct path *path, __u64 mask) +{ + /* + * Proc is special and various files have special locking rules so + * fanotify permission events have high chances of deadlocking the + * system. Just disallow them. + */ + if (mask & FANOTIFY_PERM_EVENTS && + !strcmp(path->mnt->mnt_sb->s_type->name, "proc")) { + return -EOPNOTSUPP; + } + return 0; +} + static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, int dfd, const char __user *pathname) { @@ -1018,6 +1032,12 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask, if (ret) goto fput_and_out; + if (flags & FAN_MARK_ADD) { + ret = fanotify_events_supported(&path, mask); + if (ret) + goto path_put_and_out; + } + if (FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { ret = fanotify_test_fid(&path, &__fsid); if (ret)
Proc filesystem has special locking rules for various files. Thus fanotify which opens files on event delivery can easily deadlock against another process that waits for fanotify permission event to be handled. Since permission events on /proc have doubtful value anyway, just disallow them. Signed-off-by: Jan Kara <jack@suse.cz> --- fs/notify/fanotify/fanotify_user.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)