[PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Christian Brauner]

This removes two redundant capable(CAP_SYS_ADMIN) checks from
fanotify_init().
fanotify_init() guards the whole syscall with capable(CAP_SYS_ADMIN) at the
beginning. So the other two capable(CAP_SYS_ADMIN) checks are not needed.

Fixes: 5dd03f55fd2 ("fanotify: allow userspace to override max queue depth")
Fixes: ac7e22dcfaf ("fanotify: allow userspace to override max marks")
Signed-off-by: Christian Brauner <christian@brauner.io>
---
 fs/notify/fanotify/fanotify_user.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index a90bb19dcfa2..ec2739a66103 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -845,8 +845,6 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 
 	if (flags & FAN_UNLIMITED_QUEUE) {
 		fd = -EPERM;
-		if (!capable(CAP_SYS_ADMIN))
-			goto out_destroy_group;
 		group->max_events = UINT_MAX;
 	} else {
 		group->max_events = FANOTIFY_DEFAULT_MAX_EVENTS;
@@ -854,8 +852,6 @@ SYSCALL_DEFINE2(fanotify_init, unsigned int, flags, unsigned int, event_f_flags)
 
 	if (flags & FAN_UNLIMITED_MARKS) {
 		fd = -EPERM;
-		if (!capable(CAP_SYS_ADMIN))
-			goto out_destroy_group;
 		group->fanotify_data.max_marks = UINT_MAX;
 	} else {
 		group->fanotify_data.max_marks = FANOTIFY_DEFAULT_MAX_MARKS;
-- 
2.21.0

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Amir Goldstein]

On Wed, May 22, 2019 at 7:32 PM Christian Brauner <christian@brauner.io> wrote:
>
> This removes two redundant capable(CAP_SYS_ADMIN) checks from
> fanotify_init().
> fanotify_init() guards the whole syscall with capable(CAP_SYS_ADMIN) at the
> beginning. So the other two capable(CAP_SYS_ADMIN) checks are not needed.

It's intentional:

commit e7099d8a5a34d2876908a9fab4952dabdcfc5909
Author: Eric Paris <eparis@redhat.com>
Date:   Thu Oct 28 17:21:57 2010 -0400

    fanotify: limit the number of marks in a single fanotify group

    There is currently no limit on the number of marks a given fanotify group
    can have.  Since fanotify is gated on CAP_SYS_ADMIN this was not seen as
    a serious DoS threat.  This patch implements a default of 8192, the same as
    inotify to work towards removing the CAP_SYS_ADMIN gating and eliminating
    the default DoS'able status.

    Signed-off-by: Eric Paris <eparis@redhat.com>

There idea is to eventually remove the gated CAP_SYS_ADMIN.
There is no reason that fanotify could not be used by unprivileged users
to setup inotify style watch on an inode or directories children, see:
https://patchwork.kernel.org/patch/10668299/

>
> Fixes: 5dd03f55fd2 ("fanotify: allow userspace to override max queue depth")
> Fixes: ac7e22dcfaf ("fanotify: allow userspace to override max marks")

Fixes is used to tag bug fixes for stable.
There is no bug.

Thanks,
Amir.

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Christian Brauner]

On May 22, 2019 8:29:37 PM GMT+02:00, Amir Goldstein <amir73il@gmail.com> wrote:
>On Wed, May 22, 2019 at 7:32 PM Christian Brauner
><christian@brauner.io> wrote:
>>
>> This removes two redundant capable(CAP_SYS_ADMIN) checks from
>> fanotify_init().
>> fanotify_init() guards the whole syscall with capable(CAP_SYS_ADMIN)
>at the
>> beginning. So the other two capable(CAP_SYS_ADMIN) checks are not
>needed.
>
>It's intentional:
>
>commit e7099d8a5a34d2876908a9fab4952dabdcfc5909
>Author: Eric Paris <eparis@redhat.com>
>Date:   Thu Oct 28 17:21:57 2010 -0400
>
>    fanotify: limit the number of marks in a single fanotify group
>
>There is currently no limit on the number of marks a given fanotify
>group
>can have.  Since fanotify is gated on CAP_SYS_ADMIN this was not seen
>as
>a serious DoS threat.  This patch implements a default of 8192, the
>same as
>inotify to work towards removing the CAP_SYS_ADMIN gating and
>eliminating
>    the default DoS'able status.
>
>    Signed-off-by: Eric Paris <eparis@redhat.com>
>
>There idea is to eventually remove the gated CAP_SYS_ADMIN.
>There is no reason that fanotify could not be used by unprivileged
>users
>to setup inotify style watch on an inode or directories children, see:
>https://patchwork.kernel.org/patch/10668299/
>
>>
>> Fixes: 5dd03f55fd2 ("fanotify: allow userspace to override max queue
>depth")
>> Fixes: ac7e22dcfaf ("fanotify: allow userspace to override max
>marks")
>
>Fixes is used to tag bug fixes for stable.
>There is no bug.
>
>Thanks,
>Amir.

Interesting. When do you think the gate can be removed?
I was looking into switching from inotify to fanotify but since it's not useable from
non-initial userns it's a no-no
since we support nested workloads.

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Amir Goldstein]

On Wed, May 22, 2019 at 9:57 PM Christian Brauner <christian@brauner.io> wrote:
>
> On May 22, 2019 8:29:37 PM GMT+02:00, Amir Goldstein <amir73il@gmail.com> wrote:
> >On Wed, May 22, 2019 at 7:32 PM Christian Brauner
> ><christian@brauner.io> wrote:
> >>
> >> This removes two redundant capable(CAP_SYS_ADMIN) checks from
> >> fanotify_init().
> >> fanotify_init() guards the whole syscall with capable(CAP_SYS_ADMIN)
> >at the
> >> beginning. So the other two capable(CAP_SYS_ADMIN) checks are not
> >needed.
> >
> >It's intentional:
> >
> >commit e7099d8a5a34d2876908a9fab4952dabdcfc5909
> >Author: Eric Paris <eparis@redhat.com>
> >Date:   Thu Oct 28 17:21:57 2010 -0400
> >
> >    fanotify: limit the number of marks in a single fanotify group
> >
> >There is currently no limit on the number of marks a given fanotify
> >group
> >can have.  Since fanotify is gated on CAP_SYS_ADMIN this was not seen
> >as
> >a serious DoS threat.  This patch implements a default of 8192, the
> >same as
> >inotify to work towards removing the CAP_SYS_ADMIN gating and
> >eliminating
> >    the default DoS'able status.
> >
> >    Signed-off-by: Eric Paris <eparis@redhat.com>
> >
> >There idea is to eventually remove the gated CAP_SYS_ADMIN.
> >There is no reason that fanotify could not be used by unprivileged
> >users
> >to setup inotify style watch on an inode or directories children, see:
> >https://patchwork.kernel.org/patch/10668299/
> >
> >>
> >> Fixes: 5dd03f55fd2 ("fanotify: allow userspace to override max queue
> >depth")
> >> Fixes: ac7e22dcfaf ("fanotify: allow userspace to override max
> >marks")
> >
> >Fixes is used to tag bug fixes for stable.
> >There is no bug.
> >
> >Thanks,
> >Amir.
>
> Interesting. When do you think the gate can be removed?

Nobody is working on this AFAIK.
What I posted was a simple POC, but I have no use case for this.
In the patchwork link above, Jan has listed the prerequisites for
removing the gate.

One of the prerequisites is FAN_REPORT_FID, which is now merged.
When events gets reported with fid instead of fd, unprivileged user
(hopefully) cannot use fid for privilege escalation.

> I was looking into switching from inotify to fanotify but since it's not usable from
> non-initial userns it's a no-no
> since we support nested workloads.

One of Jan's questions was what is the benefit of using inotify-compatible
fanotify vs. using inotify.
So what was the reason you were looking into switching from inotify to fanotify?
Is it because of mount/filesystem watch? Because making those available for
unprivileged user sounds risky...

Thanks,
Amir.

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Christian Brauner]

On Wed, May 22, 2019 at 11:00:22PM +0300, Amir Goldstein wrote:
> On Wed, May 22, 2019 at 9:57 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > On May 22, 2019 8:29:37 PM GMT+02:00, Amir Goldstein <amir73il@gmail.com> wrote:
> > >On Wed, May 22, 2019 at 7:32 PM Christian Brauner
> > ><christian@brauner.io> wrote:
> > >>
> > >> This removes two redundant capable(CAP_SYS_ADMIN) checks from
> > >> fanotify_init().
> > >> fanotify_init() guards the whole syscall with capable(CAP_SYS_ADMIN)
> > >at the
> > >> beginning. So the other two capable(CAP_SYS_ADMIN) checks are not
> > >needed.
> > >
> > >It's intentional:
> > >
> > >commit e7099d8a5a34d2876908a9fab4952dabdcfc5909
> > >Author: Eric Paris <eparis@redhat.com>
> > >Date:   Thu Oct 28 17:21:57 2010 -0400
> > >
> > >    fanotify: limit the number of marks in a single fanotify group
> > >
> > >There is currently no limit on the number of marks a given fanotify
> > >group
> > >can have.  Since fanotify is gated on CAP_SYS_ADMIN this was not seen
> > >as
> > >a serious DoS threat.  This patch implements a default of 8192, the
> > >same as
> > >inotify to work towards removing the CAP_SYS_ADMIN gating and
> > >eliminating
> > >    the default DoS'able status.
> > >
> > >    Signed-off-by: Eric Paris <eparis@redhat.com>
> > >
> > >There idea is to eventually remove the gated CAP_SYS_ADMIN.
> > >There is no reason that fanotify could not be used by unprivileged
> > >users
> > >to setup inotify style watch on an inode or directories children, see:
> > >https://patchwork.kernel.org/patch/10668299/
> > >
> > >>
> > >> Fixes: 5dd03f55fd2 ("fanotify: allow userspace to override max queue
> > >depth")
> > >> Fixes: ac7e22dcfaf ("fanotify: allow userspace to override max
> > >marks")
> > >
> > >Fixes is used to tag bug fixes for stable.
> > >There is no bug.
> > >
> > >Thanks,
> > >Amir.
> >
> > Interesting. When do you think the gate can be removed?
> 
> Nobody is working on this AFAIK.
> What I posted was a simple POC, but I have no use case for this.
> In the patchwork link above, Jan has listed the prerequisites for
> removing the gate.
> 
> One of the prerequisites is FAN_REPORT_FID, which is now merged.
> When events gets reported with fid instead of fd, unprivileged user
> (hopefully) cannot use fid for privilege escalation.
> 
> > I was looking into switching from inotify to fanotify but since it's not usable from
> > non-initial userns it's a no-no
> > since we support nested workloads.
> 
> One of Jan's questions was what is the benefit of using inotify-compatible
> fanotify vs. using inotify.
> So what was the reason you were looking into switching from inotify to fanotify?
> Is it because of mount/filesystem watch? Because making those available for

Yeah. Well, I would need to look but you could probably do it safely for
filesystems mountable in user namespaces (which are few).
Can you do a bind-mount and then place a watch on the bind-mount or is
this superblock based?

Thanks!
Christian

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Amir Goldstein]

On Thu, May 23, 2019 at 12:55 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Wed, May 22, 2019 at 11:00:22PM +0300, Amir Goldstein wrote:
> > On Wed, May 22, 2019 at 9:57 PM Christian Brauner <christian@brauner.io> wrote:
> > >
> > > On May 22, 2019 8:29:37 PM GMT+02:00, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >On Wed, May 22, 2019 at 7:32 PM Christian Brauner
> > > ><christian@brauner.io> wrote:
> > > >>
> > > >> This removes two redundant capable(CAP_SYS_ADMIN) checks from
> > > >> fanotify_init().
> > > >> fanotify_init() guards the whole syscall with capable(CAP_SYS_ADMIN)
> > > >at the
> > > >> beginning. So the other two capable(CAP_SYS_ADMIN) checks are not
> > > >needed.
> > > >
> > > >It's intentional:
> > > >
> > > >commit e7099d8a5a34d2876908a9fab4952dabdcfc5909
> > > >Author: Eric Paris <eparis@redhat.com>
> > > >Date:   Thu Oct 28 17:21:57 2010 -0400
> > > >
> > > >    fanotify: limit the number of marks in a single fanotify group
> > > >
> > > >There is currently no limit on the number of marks a given fanotify
> > > >group
> > > >can have.  Since fanotify is gated on CAP_SYS_ADMIN this was not seen
> > > >as
> > > >a serious DoS threat.  This patch implements a default of 8192, the
> > > >same as
> > > >inotify to work towards removing the CAP_SYS_ADMIN gating and
> > > >eliminating
> > > >    the default DoS'able status.
> > > >
> > > >    Signed-off-by: Eric Paris <eparis@redhat.com>
> > > >
> > > >There idea is to eventually remove the gated CAP_SYS_ADMIN.
> > > >There is no reason that fanotify could not be used by unprivileged
> > > >users
> > > >to setup inotify style watch on an inode or directories children, see:
> > > >https://patchwork.kernel.org/patch/10668299/
> > > >
> > > >>
> > > >> Fixes: 5dd03f55fd2 ("fanotify: allow userspace to override max queue
> > > >depth")
> > > >> Fixes: ac7e22dcfaf ("fanotify: allow userspace to override max
> > > >marks")
> > > >
> > > >Fixes is used to tag bug fixes for stable.
> > > >There is no bug.
> > > >
> > > >Thanks,
> > > >Amir.
> > >
> > > Interesting. When do you think the gate can be removed?
> >
> > Nobody is working on this AFAIK.
> > What I posted was a simple POC, but I have no use case for this.
> > In the patchwork link above, Jan has listed the prerequisites for
> > removing the gate.
> >
> > One of the prerequisites is FAN_REPORT_FID, which is now merged.
> > When events gets reported with fid instead of fd, unprivileged user
> > (hopefully) cannot use fid for privilege escalation.
> >
> > > I was looking into switching from inotify to fanotify but since it's not usable from
> > > non-initial userns it's a no-no
> > > since we support nested workloads.
> >
> > One of Jan's questions was what is the benefit of using inotify-compatible
> > fanotify vs. using inotify.
> > So what was the reason you were looking into switching from inotify to fanotify?
> > Is it because of mount/filesystem watch? Because making those available for
>
> Yeah. Well, I would need to look but you could probably do it safely for
> filesystems mountable in user namespaces (which are few).
> Can you do a bind-mount and then place a watch on the bind-mount or is
> this superblock based?
>

Either.
FAN_MARK_MOUNT was there from day 1 of fanotify.
FAN_MARK_FILESYSTEM was merged to Linux Linux 4.20.

But directory modification events that are supported since v5.1 are
not available
with FAN_MARK_MOUNT, see:
https://github.com/amir73il/man-pages/blob/fanotify_fid/man2/fanotify_init.2#L97

Matthew,

Perhaps this fact is worth a mention in the linked entry for FAN_REPORT_FID
in fanotify_init.2 in addition to the comment on the entry for FAN_MARK_MOUNT
in fanotify_mark.2.

Thanks,
Amir.

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Christian Brauner]

On Thu, May 23, 2019 at 01:25:08PM +0300, Amir Goldstein wrote:
> On Thu, May 23, 2019 at 12:55 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > On Wed, May 22, 2019 at 11:00:22PM +0300, Amir Goldstein wrote:
> > > On Wed, May 22, 2019 at 9:57 PM Christian Brauner <christian@brauner.io> wrote:
> > > >
> > > > On May 22, 2019 8:29:37 PM GMT+02:00, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >On Wed, May 22, 2019 at 7:32 PM Christian Brauner
> > > > ><christian@brauner.io> wrote:
> > > > >>
> > > > >> This removes two redundant capable(CAP_SYS_ADMIN) checks from
> > > > >> fanotify_init().
> > > > >> fanotify_init() guards the whole syscall with capable(CAP_SYS_ADMIN)
> > > > >at the
> > > > >> beginning. So the other two capable(CAP_SYS_ADMIN) checks are not
> > > > >needed.
> > > > >
> > > > >It's intentional:
> > > > >
> > > > >commit e7099d8a5a34d2876908a9fab4952dabdcfc5909
> > > > >Author: Eric Paris <eparis@redhat.com>
> > > > >Date:   Thu Oct 28 17:21:57 2010 -0400
> > > > >
> > > > >    fanotify: limit the number of marks in a single fanotify group
> > > > >
> > > > >There is currently no limit on the number of marks a given fanotify
> > > > >group
> > > > >can have.  Since fanotify is gated on CAP_SYS_ADMIN this was not seen
> > > > >as
> > > > >a serious DoS threat.  This patch implements a default of 8192, the
> > > > >same as
> > > > >inotify to work towards removing the CAP_SYS_ADMIN gating and
> > > > >eliminating
> > > > >    the default DoS'able status.
> > > > >
> > > > >    Signed-off-by: Eric Paris <eparis@redhat.com>
> > > > >
> > > > >There idea is to eventually remove the gated CAP_SYS_ADMIN.
> > > > >There is no reason that fanotify could not be used by unprivileged
> > > > >users
> > > > >to setup inotify style watch on an inode or directories children, see:
> > > > >https://patchwork.kernel.org/patch/10668299/
> > > > >
> > > > >>
> > > > >> Fixes: 5dd03f55fd2 ("fanotify: allow userspace to override max queue
> > > > >depth")
> > > > >> Fixes: ac7e22dcfaf ("fanotify: allow userspace to override max
> > > > >marks")
> > > > >
> > > > >Fixes is used to tag bug fixes for stable.
> > > > >There is no bug.
> > > > >
> > > > >Thanks,
> > > > >Amir.
> > > >
> > > > Interesting. When do you think the gate can be removed?
> > >
> > > Nobody is working on this AFAIK.
> > > What I posted was a simple POC, but I have no use case for this.
> > > In the patchwork link above, Jan has listed the prerequisites for
> > > removing the gate.
> > >
> > > One of the prerequisites is FAN_REPORT_FID, which is now merged.
> > > When events gets reported with fid instead of fd, unprivileged user
> > > (hopefully) cannot use fid for privilege escalation.
> > >
> > > > I was looking into switching from inotify to fanotify but since it's not usable from
> > > > non-initial userns it's a no-no
> > > > since we support nested workloads.
> > >
> > > One of Jan's questions was what is the benefit of using inotify-compatible
> > > fanotify vs. using inotify.
> > > So what was the reason you were looking into switching from inotify to fanotify?
> > > Is it because of mount/filesystem watch? Because making those available for
> >
> > Yeah. Well, I would need to look but you could probably do it safely for
> > filesystems mountable in user namespaces (which are few).
> > Can you do a bind-mount and then place a watch on the bind-mount or is
> > this superblock based?
> >
> 
> Either.
> FAN_MARK_MOUNT was there from day 1 of fanotify.
> FAN_MARK_FILESYSTEM was merged to Linux Linux 4.20.
> 
> But directory modification events that are supported since v5.1 are
> not available
> with FAN_MARK_MOUNT, see:

Because you're worried about unprivileged users spying on events? Or
something else?
Because if you can do a bind-mount there's nothing preventing an
unprivileged user to do a hand-rolled recursive inotify that would
amount to the same thing anyway.
(And btw, v5.1 really is a major step forward and I would really like to
 use this api tbh.)

Christian

> https://github.com/amir73il/man-pages/blob/fanotify_fid/man2/fanotify_init.2#L97
> 
> Matthew,
> 
> Perhaps this fact is worth a mention in the linked entry for FAN_REPORT_FID
> in fanotify_init.2 in addition to the comment on the entry for FAN_MARK_MOUNT
> in fanotify_mark.2.
> 
> Thanks,
> Amir.

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Amir Goldstein]

On Thu, May 23, 2019 at 1:42 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Thu, May 23, 2019 at 01:25:08PM +0300, Amir Goldstein wrote:
> > On Thu, May 23, 2019 at 12:55 PM Christian Brauner <christian@brauner.io> wrote:
> > >
> > > On Wed, May 22, 2019 at 11:00:22PM +0300, Amir Goldstein wrote:
> > > > On Wed, May 22, 2019 at 9:57 PM Christian Brauner <christian@brauner.io> wrote:
> > > > >
> > > > > On May 22, 2019 8:29:37 PM GMT+02:00, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > >On Wed, May 22, 2019 at 7:32 PM Christian Brauner
> > > > > ><christian@brauner.io> wrote:
> > > > > >>
> > > > > >> This removes two redundant capable(CAP_SYS_ADMIN) checks from
> > > > > >> fanotify_init().
> > > > > >> fanotify_init() guards the whole syscall with capable(CAP_SYS_ADMIN)
> > > > > >at the
> > > > > >> beginning. So the other two capable(CAP_SYS_ADMIN) checks are not
> > > > > >needed.
> > > > > >
> > > > > >It's intentional:
> > > > > >
> > > > > >commit e7099d8a5a34d2876908a9fab4952dabdcfc5909
> > > > > >Author: Eric Paris <eparis@redhat.com>
> > > > > >Date:   Thu Oct 28 17:21:57 2010 -0400
> > > > > >
> > > > > >    fanotify: limit the number of marks in a single fanotify group
> > > > > >
> > > > > >There is currently no limit on the number of marks a given fanotify
> > > > > >group
> > > > > >can have.  Since fanotify is gated on CAP_SYS_ADMIN this was not seen
> > > > > >as
> > > > > >a serious DoS threat.  This patch implements a default of 8192, the
> > > > > >same as
> > > > > >inotify to work towards removing the CAP_SYS_ADMIN gating and
> > > > > >eliminating
> > > > > >    the default DoS'able status.
> > > > > >
> > > > > >    Signed-off-by: Eric Paris <eparis@redhat.com>
> > > > > >
> > > > > >There idea is to eventually remove the gated CAP_SYS_ADMIN.
> > > > > >There is no reason that fanotify could not be used by unprivileged
> > > > > >users
> > > > > >to setup inotify style watch on an inode or directories children, see:
> > > > > >https://patchwork.kernel.org/patch/10668299/
> > > > > >
> > > > > >>
> > > > > >> Fixes: 5dd03f55fd2 ("fanotify: allow userspace to override max queue
> > > > > >depth")
> > > > > >> Fixes: ac7e22dcfaf ("fanotify: allow userspace to override max
> > > > > >marks")
> > > > > >
> > > > > >Fixes is used to tag bug fixes for stable.
> > > > > >There is no bug.
> > > > > >
> > > > > >Thanks,
> > > > > >Amir.
> > > > >
> > > > > Interesting. When do you think the gate can be removed?
> > > >
> > > > Nobody is working on this AFAIK.
> > > > What I posted was a simple POC, but I have no use case for this.
> > > > In the patchwork link above, Jan has listed the prerequisites for
> > > > removing the gate.
> > > >
> > > > One of the prerequisites is FAN_REPORT_FID, which is now merged.
> > > > When events gets reported with fid instead of fd, unprivileged user
> > > > (hopefully) cannot use fid for privilege escalation.
> > > >
> > > > > I was looking into switching from inotify to fanotify but since it's not usable from
> > > > > non-initial userns it's a no-no
> > > > > since we support nested workloads.
> > > >
> > > > One of Jan's questions was what is the benefit of using inotify-compatible
> > > > fanotify vs. using inotify.
> > > > So what was the reason you were looking into switching from inotify to fanotify?
> > > > Is it because of mount/filesystem watch? Because making those available for
> > >
> > > Yeah. Well, I would need to look but you could probably do it safely for
> > > filesystems mountable in user namespaces (which are few).
> > > Can you do a bind-mount and then place a watch on the bind-mount or is
> > > this superblock based?
> > >
> >
> > Either.
> > FAN_MARK_MOUNT was there from day 1 of fanotify.
> > FAN_MARK_FILESYSTEM was merged to Linux Linux 4.20.
> >
> > But directory modification events that are supported since v5.1 are
> > not available
> > with FAN_MARK_MOUNT, see:
>
> Because you're worried about unprivileged users spying on events? Or
> something else?

Something else. The current fsnotify_move/create/delete() VFS hooks
have no path/mount information, so it is not possible to filter them by
mount only by inode/sb.
Fixing that would not be trivial, but first a strong use case would need
to be presented.

> Because if you can do a bind-mount there's nothing preventing an
> unprivileged user to do a hand-rolled recursive inotify that would
> amount to the same thing anyway.

There is. unprivileged user cannot traverse into directories it is not
allowed to read/search.

> (And btw, v5.1 really is a major step forward and I would really like to
>  use this api tbh.)
>

You haven't answered my question. What is the reason you are interested
in the new API? What does it provide that the old API does not?
I know the 2 APIs differ. I just want to know which difference interests *you*,
because without a strong use case, it will be hard for me to make progress
upstream.

Is what you want really a "bind-mount" watch or a "subtree watch"?
The distinction is important. I am thinking about solutions for the latter,
although there is no immediate solution in the horizon - only ideas.

Thanks,
Amir.

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Christian Brauner]

On Thu, May 23, 2019 at 02:40:39PM +0300, Amir Goldstein wrote:
> On Thu, May 23, 2019 at 1:42 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > On Thu, May 23, 2019 at 01:25:08PM +0300, Amir Goldstein wrote:
> > > On Thu, May 23, 2019 at 12:55 PM Christian Brauner <christian@brauner.io> wrote:
> > > >
> > > > On Wed, May 22, 2019 at 11:00:22PM +0300, Amir Goldstein wrote:
> > > > > On Wed, May 22, 2019 at 9:57 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > >
> > > > > > On May 22, 2019 8:29:37 PM GMT+02:00, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > > >On Wed, May 22, 2019 at 7:32 PM Christian Brauner
> > > > > > ><christian@brauner.io> wrote:
> > > > > > >>
> > > > > > >> This removes two redundant capable(CAP_SYS_ADMIN) checks from
> > > > > > >> fanotify_init().
> > > > > > >> fanotify_init() guards the whole syscall with capable(CAP_SYS_ADMIN)
> > > > > > >at the
> > > > > > >> beginning. So the other two capable(CAP_SYS_ADMIN) checks are not
> > > > > > >needed.
> > > > > > >
> > > > > > >It's intentional:
> > > > > > >
> > > > > > >commit e7099d8a5a34d2876908a9fab4952dabdcfc5909
> > > > > > >Author: Eric Paris <eparis@redhat.com>
> > > > > > >Date:   Thu Oct 28 17:21:57 2010 -0400
> > > > > > >
> > > > > > >    fanotify: limit the number of marks in a single fanotify group
> > > > > > >
> > > > > > >There is currently no limit on the number of marks a given fanotify
> > > > > > >group
> > > > > > >can have.  Since fanotify is gated on CAP_SYS_ADMIN this was not seen
> > > > > > >as
> > > > > > >a serious DoS threat.  This patch implements a default of 8192, the
> > > > > > >same as
> > > > > > >inotify to work towards removing the CAP_SYS_ADMIN gating and
> > > > > > >eliminating
> > > > > > >    the default DoS'able status.
> > > > > > >
> > > > > > >    Signed-off-by: Eric Paris <eparis@redhat.com>
> > > > > > >
> > > > > > >There idea is to eventually remove the gated CAP_SYS_ADMIN.
> > > > > > >There is no reason that fanotify could not be used by unprivileged
> > > > > > >users
> > > > > > >to setup inotify style watch on an inode or directories children, see:
> > > > > > >https://patchwork.kernel.org/patch/10668299/
> > > > > > >
> > > > > > >>
> > > > > > >> Fixes: 5dd03f55fd2 ("fanotify: allow userspace to override max queue
> > > > > > >depth")
> > > > > > >> Fixes: ac7e22dcfaf ("fanotify: allow userspace to override max
> > > > > > >marks")
> > > > > > >
> > > > > > >Fixes is used to tag bug fixes for stable.
> > > > > > >There is no bug.
> > > > > > >
> > > > > > >Thanks,
> > > > > > >Amir.
> > > > > >
> > > > > > Interesting. When do you think the gate can be removed?
> > > > >
> > > > > Nobody is working on this AFAIK.
> > > > > What I posted was a simple POC, but I have no use case for this.
> > > > > In the patchwork link above, Jan has listed the prerequisites for
> > > > > removing the gate.
> > > > >
> > > > > One of the prerequisites is FAN_REPORT_FID, which is now merged.
> > > > > When events gets reported with fid instead of fd, unprivileged user
> > > > > (hopefully) cannot use fid for privilege escalation.
> > > > >
> > > > > > I was looking into switching from inotify to fanotify but since it's not usable from
> > > > > > non-initial userns it's a no-no
> > > > > > since we support nested workloads.
> > > > >
> > > > > One of Jan's questions was what is the benefit of using inotify-compatible
> > > > > fanotify vs. using inotify.
> > > > > So what was the reason you were looking into switching from inotify to fanotify?
> > > > > Is it because of mount/filesystem watch? Because making those available for
> > > >
> > > > Yeah. Well, I would need to look but you could probably do it safely for
> > > > filesystems mountable in user namespaces (which are few).
> > > > Can you do a bind-mount and then place a watch on the bind-mount or is
> > > > this superblock based?
> > > >
> > >
> > > Either.
> > > FAN_MARK_MOUNT was there from day 1 of fanotify.
> > > FAN_MARK_FILESYSTEM was merged to Linux Linux 4.20.
> > >
> > > But directory modification events that are supported since v5.1 are
> > > not available
> > > with FAN_MARK_MOUNT, see:
> >
> > Because you're worried about unprivileged users spying on events? Or
> > something else?
> 
> Something else. The current fsnotify_move/create/delete() VFS hooks
> have no path/mount information, so it is not possible to filter them by
> mount only by inode/sb.
> Fixing that would not be trivial, but first a strong use case would need
> to be presented.
> 
> > Because if you can do a bind-mount there's nothing preventing an
> > unprivileged user to do a hand-rolled recursive inotify that would
> > amount to the same thing anyway.
> 
> There is. unprivileged user cannot traverse into directories it is not
> allowed to read/search.

Right, I should've mentioned: when you're userns root and you have
access to all files. The part that is interesting to me is getting rid
of capable(CAP_SYS_ADMIN).

> 
> > (And btw, v5.1 really is a major step forward and I would really like to
> >  use this api tbh.)
> >
> 
> You haven't answered my question. What is the reason you are interested
> in the new API? What does it provide that the old API does not?
> I know the 2 APIs differ. I just want to know which difference interests *you*,
> because without a strong use case, it will be hard for me to make progress
> upstream.
> 
> Is what you want really a "bind-mount" watch or a "subtree watch"?
> The distinction is important. I am thinking about solutions for the latter,
> although there is no immediate solution in the horizon - only ideas.

Both cases would be interesting. But subtree watch is what would
probably help a lot already. So let me explain.
For LXD - not sure if you know what that is - we allow user to "hotplug"
mounts or certain whitelisted devices into a user namespace container.
One of the nifty features is that we let users specify a "required"
property. When "required" is "false" the user can give us a path, e.g.
/bla/bla/bla/target and then we place a watch on the closest existing
ancestor of my-device. When the target shows up we hotplug it for the
user. Now, as you imagine maintaining that cache until "target" shows up
is a royal pain.

I think that we can get rid of at least some of the complexity if
subtree watch and bind-mount watches would work.

Thanks!
Christian

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Amir Goldstein]

On Thu, May 23, 2019 at 2:58 PM Christian Brauner <christian@brauner.io> wrote:
>
> On Thu, May 23, 2019 at 02:40:39PM +0300, Amir Goldstein wrote:
> > On Thu, May 23, 2019 at 1:42 PM Christian Brauner <christian@brauner.io> wrote:
> > >
> > > On Thu, May 23, 2019 at 01:25:08PM +0300, Amir Goldstein wrote:
> > > > On Thu, May 23, 2019 at 12:55 PM Christian Brauner <christian@brauner.io> wrote:
> > > > >
> > > > > On Wed, May 22, 2019 at 11:00:22PM +0300, Amir Goldstein wrote:
> > > > > > On Wed, May 22, 2019 at 9:57 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > >
> > > > > > > On May 22, 2019 8:29:37 PM GMT+02:00, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > > > >On Wed, May 22, 2019 at 7:32 PM Christian Brauner
> > > > > > > ><christian@brauner.io> wrote:
> > > > > > > >>
> > > > > > > >> This removes two redundant capable(CAP_SYS_ADMIN) checks from
> > > > > > > >> fanotify_init().
> > > > > > > >> fanotify_init() guards the whole syscall with capable(CAP_SYS_ADMIN)
> > > > > > > >at the
> > > > > > > >> beginning. So the other two capable(CAP_SYS_ADMIN) checks are not
> > > > > > > >needed.
> > > > > > > >
> > > > > > > >It's intentional:
> > > > > > > >
> > > > > > > >commit e7099d8a5a34d2876908a9fab4952dabdcfc5909
> > > > > > > >Author: Eric Paris <eparis@redhat.com>
> > > > > > > >Date:   Thu Oct 28 17:21:57 2010 -0400
> > > > > > > >
> > > > > > > >    fanotify: limit the number of marks in a single fanotify group
> > > > > > > >
> > > > > > > >There is currently no limit on the number of marks a given fanotify
> > > > > > > >group
> > > > > > > >can have.  Since fanotify is gated on CAP_SYS_ADMIN this was not seen
> > > > > > > >as
> > > > > > > >a serious DoS threat.  This patch implements a default of 8192, the
> > > > > > > >same as
> > > > > > > >inotify to work towards removing the CAP_SYS_ADMIN gating and
> > > > > > > >eliminating
> > > > > > > >    the default DoS'able status.
> > > > > > > >
> > > > > > > >    Signed-off-by: Eric Paris <eparis@redhat.com>
> > > > > > > >
> > > > > > > >There idea is to eventually remove the gated CAP_SYS_ADMIN.
> > > > > > > >There is no reason that fanotify could not be used by unprivileged
> > > > > > > >users
> > > > > > > >to setup inotify style watch on an inode or directories children, see:
> > > > > > > >https://patchwork.kernel.org/patch/10668299/
> > > > > > > >
> > > > > > > >>
> > > > > > > >> Fixes: 5dd03f55fd2 ("fanotify: allow userspace to override max queue
> > > > > > > >depth")
> > > > > > > >> Fixes: ac7e22dcfaf ("fanotify: allow userspace to override max
> > > > > > > >marks")
> > > > > > > >
> > > > > > > >Fixes is used to tag bug fixes for stable.
> > > > > > > >There is no bug.
> > > > > > > >
> > > > > > > >Thanks,
> > > > > > > >Amir.
> > > > > > >
> > > > > > > Interesting. When do you think the gate can be removed?
> > > > > >
> > > > > > Nobody is working on this AFAIK.
> > > > > > What I posted was a simple POC, but I have no use case for this.
> > > > > > In the patchwork link above, Jan has listed the prerequisites for
> > > > > > removing the gate.
> > > > > >
> > > > > > One of the prerequisites is FAN_REPORT_FID, which is now merged.
> > > > > > When events gets reported with fid instead of fd, unprivileged user
> > > > > > (hopefully) cannot use fid for privilege escalation.
> > > > > >
> > > > > > > I was looking into switching from inotify to fanotify but since it's not usable from
> > > > > > > non-initial userns it's a no-no
> > > > > > > since we support nested workloads.
> > > > > >
> > > > > > One of Jan's questions was what is the benefit of using inotify-compatible
> > > > > > fanotify vs. using inotify.
> > > > > > So what was the reason you were looking into switching from inotify to fanotify?
> > > > > > Is it because of mount/filesystem watch? Because making those available for
> > > > >
> > > > > Yeah. Well, I would need to look but you could probably do it safely for
> > > > > filesystems mountable in user namespaces (which are few).
> > > > > Can you do a bind-mount and then place a watch on the bind-mount or is
> > > > > this superblock based?
> > > > >
> > > >
> > > > Either.
> > > > FAN_MARK_MOUNT was there from day 1 of fanotify.
> > > > FAN_MARK_FILESYSTEM was merged to Linux Linux 4.20.
> > > >
> > > > But directory modification events that are supported since v5.1 are
> > > > not available
> > > > with FAN_MARK_MOUNT, see:
> > >
> > > Because you're worried about unprivileged users spying on events? Or
> > > something else?
> >
> > Something else. The current fsnotify_move/create/delete() VFS hooks
> > have no path/mount information, so it is not possible to filter them by
> > mount only by inode/sb.
> > Fixing that would not be trivial, but first a strong use case would need
> > to be presented.
> >
> > > Because if you can do a bind-mount there's nothing preventing an
> > > unprivileged user to do a hand-rolled recursive inotify that would
> > > amount to the same thing anyway.
> >
> > There is. unprivileged user cannot traverse into directories it is not
> > allowed to read/search.
>
> Right, I should've mentioned: when you're userns root and you have
> access to all files. The part that is interesting to me is getting rid
> of capable(CAP_SYS_ADMIN).

Indeed. so part of removing the gated capable(CAP_SYS_ADMIN)
is figuring out the permission checks needed for individual features.
I agree that for FAN_MARK_MOUNT/FILESYSTEM,
capabale(CAP_SYS_ADMIN) is too strong.
ns_capable(sb->s_user_ns, CAP_DAC_READ_SEARCH)
is probably enough.

>
> >
> > > (And btw, v5.1 really is a major step forward and I would really like to
> > >  use this api tbh.)
> > >
> >
> > You haven't answered my question. What is the reason you are interested
> > in the new API? What does it provide that the old API does not?
> > I know the 2 APIs differ. I just want to know which difference interests *you*,
> > because without a strong use case, it will be hard for me to make progress
> > upstream.
> >
> > Is what you want really a "bind-mount" watch or a "subtree watch"?
> > The distinction is important. I am thinking about solutions for the latter,
> > although there is no immediate solution in the horizon - only ideas.
>
> Both cases would be interesting. But subtree watch is what would
> probably help a lot already. So let me explain.
> For LXD - not sure if you know what that is -
I do

>  we allow user to "hotplug"
> mounts or certain whitelisted devices into a user namespace container.
> One of the nifty features is that we let users specify a "required"
> property. When "required" is "false" the user can give us a path, e.g.
> /bla/bla/bla/target and then we place a watch on the closest existing
> ancestor of my-device. When the target shows up we hotplug it for the
> user. Now, as you imagine maintaining that cache until "target" shows up
> is a royal pain.

You lost me there. Are you looking for notifications when device files appear?
When directory is created? Please give a concrete example.
What part of /bla/bla/bla/target appears, when and how.
fanotify does not give notifications when mounts are mounted.
I have seen a proposal by David Howells for mount change notifications.

Thanks,
Amir.

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Christian Brauner]

On Thu, May 23, 2019 at 04:16:24PM +0300, Amir Goldstein wrote:
> On Thu, May 23, 2019 at 2:58 PM Christian Brauner <christian@brauner.io> wrote:
> >
> > On Thu, May 23, 2019 at 02:40:39PM +0300, Amir Goldstein wrote:
> > > On Thu, May 23, 2019 at 1:42 PM Christian Brauner <christian@brauner.io> wrote:
> > > >
> > > > On Thu, May 23, 2019 at 01:25:08PM +0300, Amir Goldstein wrote:
> > > > > On Thu, May 23, 2019 at 12:55 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > >
> > > > > > On Wed, May 22, 2019 at 11:00:22PM +0300, Amir Goldstein wrote:
> > > > > > > On Wed, May 22, 2019 at 9:57 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > >
> > > > > > > > On May 22, 2019 8:29:37 PM GMT+02:00, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > > > > >On Wed, May 22, 2019 at 7:32 PM Christian Brauner
> > > > > > > > ><christian@brauner.io> wrote:
> > > > > > > > >>
> > > > > > > > >> This removes two redundant capable(CAP_SYS_ADMIN) checks from
> > > > > > > > >> fanotify_init().
> > > > > > > > >> fanotify_init() guards the whole syscall with capable(CAP_SYS_ADMIN)
> > > > > > > > >at the
> > > > > > > > >> beginning. So the other two capable(CAP_SYS_ADMIN) checks are not
> > > > > > > > >needed.
> > > > > > > > >
> > > > > > > > >It's intentional:
> > > > > > > > >
> > > > > > > > >commit e7099d8a5a34d2876908a9fab4952dabdcfc5909
> > > > > > > > >Author: Eric Paris <eparis@redhat.com>
> > > > > > > > >Date:   Thu Oct 28 17:21:57 2010 -0400
> > > > > > > > >
> > > > > > > > >    fanotify: limit the number of marks in a single fanotify group
> > > > > > > > >
> > > > > > > > >There is currently no limit on the number of marks a given fanotify
> > > > > > > > >group
> > > > > > > > >can have.  Since fanotify is gated on CAP_SYS_ADMIN this was not seen
> > > > > > > > >as
> > > > > > > > >a serious DoS threat.  This patch implements a default of 8192, the
> > > > > > > > >same as
> > > > > > > > >inotify to work towards removing the CAP_SYS_ADMIN gating and
> > > > > > > > >eliminating
> > > > > > > > >    the default DoS'able status.
> > > > > > > > >
> > > > > > > > >    Signed-off-by: Eric Paris <eparis@redhat.com>
> > > > > > > > >
> > > > > > > > >There idea is to eventually remove the gated CAP_SYS_ADMIN.
> > > > > > > > >There is no reason that fanotify could not be used by unprivileged
> > > > > > > > >users
> > > > > > > > >to setup inotify style watch on an inode or directories children, see:
> > > > > > > > >https://patchwork.kernel.org/patch/10668299/
> > > > > > > > >
> > > > > > > > >>
> > > > > > > > >> Fixes: 5dd03f55fd2 ("fanotify: allow userspace to override max queue
> > > > > > > > >depth")
> > > > > > > > >> Fixes: ac7e22dcfaf ("fanotify: allow userspace to override max
> > > > > > > > >marks")
> > > > > > > > >
> > > > > > > > >Fixes is used to tag bug fixes for stable.
> > > > > > > > >There is no bug.
> > > > > > > > >
> > > > > > > > >Thanks,
> > > > > > > > >Amir.
> > > > > > > >
> > > > > > > > Interesting. When do you think the gate can be removed?
> > > > > > >
> > > > > > > Nobody is working on this AFAIK.
> > > > > > > What I posted was a simple POC, but I have no use case for this.
> > > > > > > In the patchwork link above, Jan has listed the prerequisites for
> > > > > > > removing the gate.
> > > > > > >
> > > > > > > One of the prerequisites is FAN_REPORT_FID, which is now merged.
> > > > > > > When events gets reported with fid instead of fd, unprivileged user
> > > > > > > (hopefully) cannot use fid for privilege escalation.
> > > > > > >
> > > > > > > > I was looking into switching from inotify to fanotify but since it's not usable from
> > > > > > > > non-initial userns it's a no-no
> > > > > > > > since we support nested workloads.
> > > > > > >
> > > > > > > One of Jan's questions was what is the benefit of using inotify-compatible
> > > > > > > fanotify vs. using inotify.
> > > > > > > So what was the reason you were looking into switching from inotify to fanotify?
> > > > > > > Is it because of mount/filesystem watch? Because making those available for
> > > > > >
> > > > > > Yeah. Well, I would need to look but you could probably do it safely for
> > > > > > filesystems mountable in user namespaces (which are few).
> > > > > > Can you do a bind-mount and then place a watch on the bind-mount or is
> > > > > > this superblock based?
> > > > > >
> > > > >
> > > > > Either.
> > > > > FAN_MARK_MOUNT was there from day 1 of fanotify.
> > > > > FAN_MARK_FILESYSTEM was merged to Linux Linux 4.20.
> > > > >
> > > > > But directory modification events that are supported since v5.1 are
> > > > > not available
> > > > > with FAN_MARK_MOUNT, see:
> > > >
> > > > Because you're worried about unprivileged users spying on events? Or
> > > > something else?
> > >
> > > Something else. The current fsnotify_move/create/delete() VFS hooks
> > > have no path/mount information, so it is not possible to filter them by
> > > mount only by inode/sb.
> > > Fixing that would not be trivial, but first a strong use case would need
> > > to be presented.
> > >
> > > > Because if you can do a bind-mount there's nothing preventing an
> > > > unprivileged user to do a hand-rolled recursive inotify that would
> > > > amount to the same thing anyway.
> > >
> > > There is. unprivileged user cannot traverse into directories it is not
> > > allowed to read/search.
> >
> > Right, I should've mentioned: when you're userns root and you have
> > access to all files. The part that is interesting to me is getting rid
> > of capable(CAP_SYS_ADMIN).
> 
> Indeed. so part of removing the gated capable(CAP_SYS_ADMIN)
> is figuring out the permission checks needed for individual features.
> I agree that for FAN_MARK_MOUNT/FILESYSTEM,
> capabale(CAP_SYS_ADMIN) is too strong.
> ns_capable(sb->s_user_ns, CAP_DAC_READ_SEARCH)
> is probably enough.
> 
> >
> > >
> > > > (And btw, v5.1 really is a major step forward and I would really like to
> > > >  use this api tbh.)
> > > >
> > >
> > > You haven't answered my question. What is the reason you are interested
> > > in the new API? What does it provide that the old API does not?
> > > I know the 2 APIs differ. I just want to know which difference interests *you*,
> > > because without a strong use case, it will be hard for me to make progress
> > > upstream.
> > >
> > > Is what you want really a "bind-mount" watch or a "subtree watch"?
> > > The distinction is important. I am thinking about solutions for the latter,
> > > although there is no immediate solution in the horizon - only ideas.
> >
> > Both cases would be interesting. But subtree watch is what would
> > probably help a lot already. So let me explain.
> > For LXD - not sure if you know what that is -
> I do
> 
> >  we allow user to "hotplug"
> > mounts or certain whitelisted devices into a user namespace container.
> > One of the nifty features is that we let users specify a "required"
> > property. When "required" is "false" the user can give us a path, e.g.
> > /bla/bla/bla/target and then we place a watch on the closest existing
> > ancestor of my-device. When the target shows up we hotplug it for the
> > user. Now, as you imagine maintaining that cache until "target" shows up
> > is a royal pain.
> 
> You lost me there. Are you looking for notifications when device files appear?

Just when that file is created. fanotify doesn't need to do anything
special for device files. As long as a file (device or
otherwise) and directory creation/deletion triggers an event we're good.
I can then just stat the file and check that it's the device I expect.

> When directory is created? Please give a concrete example.
> What part of /bla/bla/bla/target appears, when and how.

So let's say the user tells me:
- When the "/A/B/C/target" file appears on the host filesystem,
  please give me access to "target" in the container at a path I tell
  you.
What I do right now is listen for the creation of the "target" file.
But at the time the user gives me instructions to listen for
"/A/B/C/target" only /A might exist and so I currently add a watch on A/
and then wait for the creation of B/, then wait for the creation of C/
and finally for the creation of "target" (Of course, I also need to
handle B/ and C/ being removed again an recreated and so on.). It would
be helpful, if I could specify, give me notifications, recursively for
e.g. A/ without me having to place extra watches on B/ and C/ when they
appear. Maybe that's out of scope...

> fanotify does not give notifications when mounts are mounted.
> I have seen a proposal by David Howells for mount change notifications.

David's going to send this out soon, I think.

Thanks!
Christian

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Jan Kara]

On Thu 23-05-19 15:35:18, Christian Brauner wrote:
> So let's say the user tells me:
> - When the "/A/B/C/target" file appears on the host filesystem,
>   please give me access to "target" in the container at a path I tell
>   you.
> What I do right now is listen for the creation of the "target" file.
> But at the time the user gives me instructions to listen for
> "/A/B/C/target" only /A might exist and so I currently add a watch on A/
> and then wait for the creation of B/, then wait for the creation of C/
> and finally for the creation of "target" (Of course, I also need to
> handle B/ and C/ being removed again an recreated and so on.). It would
> be helpful, if I could specify, give me notifications, recursively for
> e.g. A/ without me having to place extra watches on B/ and C/ when they
> appear. Maybe that's out of scope...

I see. But this is going to be painful whatever you do. Consider for
example situation like:

mkdir -p BAR/B/C/
touch BAR/B/C/target
mv BAR A

Or even situation where several renames race so that the end result creates
the name (or does not create it depending on how renames race). And by the
time you decide A/B/C/target exists, it doesn't need to exist anymore.
Honestly I don't see how you want to implement *any* solution in a sane
way. About the most reliable+simple would seem to be stat "A/B/C/target"
once per second as dumb as it is.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Christian Brauner]

On Thu, May 23, 2019 at 04:40:50PM +0200, Jan Kara wrote:
> On Thu 23-05-19 15:35:18, Christian Brauner wrote:
> > So let's say the user tells me:
> > - When the "/A/B/C/target" file appears on the host filesystem,
> >   please give me access to "target" in the container at a path I tell
> >   you.
> > What I do right now is listen for the creation of the "target" file.
> > But at the time the user gives me instructions to listen for
> > "/A/B/C/target" only /A might exist and so I currently add a watch on A/
> > and then wait for the creation of B/, then wait for the creation of C/
> > and finally for the creation of "target" (Of course, I also need to
> > handle B/ and C/ being removed again an recreated and so on.). It would
> > be helpful, if I could specify, give me notifications, recursively for
> > e.g. A/ without me having to place extra watches on B/ and C/ when they
> > appear. Maybe that's out of scope...
> 
> I see. But this is going to be painful whatever you do. Consider for
> example situation like:
> 
> mkdir -p BAR/B/C/
> touch BAR/B/C/target
> mv BAR A
> 
> Or even situation where several renames race so that the end result creates
> the name (or does not create it depending on how renames race). And by the
> time you decide A/B/C/target exists, it doesn't need to exist anymore.
> Honestly I don't see how you want to implement *any* solution in a sane
> way. About the most reliable+simple would seem to be stat "A/B/C/target"
> once per second as dumb as it is.

What we have kinda works rn good enough. And yes, it's inherently racy.
Basically, iirc we only watch that it exists once, then create the thing
for the container and then consider our job done. If that thing is
removed under us we don't really care.

Christian

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Amir Goldstein]

On Thu, May 23, 2019 at 5:40 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 23-05-19 15:35:18, Christian Brauner wrote:
> > So let's say the user tells me:
> > - When the "/A/B/C/target" file appears on the host filesystem,
> >   please give me access to "target" in the container at a path I tell
> >   you.
> > What I do right now is listen for the creation of the "target" file.
> > But at the time the user gives me instructions to listen for
> > "/A/B/C/target" only /A might exist and so I currently add a watch on A/
> > and then wait for the creation of B/, then wait for the creation of C/
> > and finally for the creation of "target" (Of course, I also need to
> > handle B/ and C/ being removed again an recreated and so on.). It would
> > be helpful, if I could specify, give me notifications, recursively for
> > e.g. A/ without me having to place extra watches on B/ and C/ when they
> > appear. Maybe that's out of scope...
>
> I see. But this is going to be painful whatever you do. Consider for
> example situation like:
>
> mkdir -p BAR/B/C/
> touch BAR/B/C/target
> mv BAR A
>
> Or even situation where several renames race so that the end result creates
> the name (or does not create it depending on how renames race). And by the
> time you decide A/B/C/target exists, it doesn't need to exist anymore.
> Honestly I don't see how you want to implement *any* solution in a sane
> way. About the most reliable+simple would seem to be stat "A/B/C/target"
> once per second as dumb as it is.
>

Just wanted to point out that while looking at possible solutions for
"path based rules" for fanotify (i.e. subtree filter) I realized that the audit
subsystem already has a quite sophisticated mechanism to maintain
and enforce path based filesystem rules.

I do not love that code at all, I can hardly follow it, but if someone would
have wanted a way to be notified when an object of a given path name
appears or disappears from the namespace, it seems like something in
the kernel is already going to a great deal of effort to do that already.
Or maybe I am misunderstanding what this code does.

Thanks,
Amir.

Re: [PATCH] fanotify: remove redundant capable(CAP_SYS_ADMIN)s

[Matthew Bobrowski]

On Thu, May 23, 2019 at 01:25:08PM +0300, Amir Goldstein wrote:

...

> > > > Interesting. When do you think the gate can be removed?
> > >
> > > Nobody is working on this AFAIK.
> > > What I posted was a simple POC, but I have no use case for this.
> > > In the patchwork link above, Jan has listed the prerequisites for
> > > removing the gate.
> > >
> > > One of the prerequisites is FAN_REPORT_FID, which is now merged.
> > > When events gets reported with fid instead of fd, unprivileged user
> > > (hopefully) cannot use fid for privilege escalation.
> > >
> > > > I was looking into switching from inotify to fanotify but since it's not usable from
> > > > non-initial userns it's a no-no
> > > > since we support nested workloads.
> > >
> > > One of Jan's questions was what is the benefit of using inotify-compatible
> > > fanotify vs. using inotify.
> > > So what was the reason you were looking into switching from inotify to fanotify?
> > > Is it because of mount/filesystem watch? Because making those available for
> >
> > Yeah. Well, I would need to look but you could probably do it safely for
> > filesystems mountable in user namespaces (which are few).
> > Can you do a bind-mount and then place a watch on the bind-mount or is
> > this superblock based?
> >
> 
> Either.
> FAN_MARK_MOUNT was there from day 1 of fanotify.
> FAN_MARK_FILESYSTEM was merged to Linux Linux 4.20.
> 
> But directory modification events that are supported since v5.1 are
> not available
> with FAN_MARK_MOUNT, see:
> https://github.com/amir73il/man-pages/blob/fanotify_fid/man2/fanotify_init.2#L97
> 
> Matthew,
> 
> Perhaps this fact is worth a mention in the linked entry for FAN_REPORT_FID
> in fanotify_init.2 in addition to the comment on the entry for FAN_MARK_MOUNT
> in fanotify_mark.2.

Sorry, a little late to the party...

The fact being that directory modification events that are supported since v5.1
are not available when used in conjunction with FAN_MARK_MOUNT?

-- 
Matthew Bobrowski