linux-ext4 - Re: [PATCH v7 19/28] fanotify: Limit number of marks with FAN_FS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOQ4uxiUrMrLYJC+ogHVJXzO4-v_LceuDoe4mmU1sSM5SMx6jg@mail.gmail.com>
Date:   Fri, 15 Oct 2021 20:49:54 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Gabriel Krisman Bertazi <krisman@...labora.com>
Cc:     Jan Kara <jack@...e.com>, "Darrick J. Wong" <djwong@...nel.org>,
        Theodore Tso <tytso@....edu>,
        David Howells <dhowells@...hat.com>,
        Khazhismel Kumykov <khazhy@...gle.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Ext4 <linux-ext4@...r.kernel.org>,
        Linux API <linux-api@...r.kernel.org>,
        Matthew Bobrowski <repnop@...gle.com>, kernel@...labora.com
Subject: Re: [PATCH v7 19/28] fanotify: Limit number of marks with
 FAN_FS_ERROR per group

On Fri, Oct 15, 2021 at 7:53 PM Gabriel Krisman Bertazi
<krisman@...labora.com> wrote:
>
> Amir Goldstein <amir73il@...il.com> writes:
>
> > On Fri, Oct 15, 2021 at 12:39 AM Gabriel Krisman Bertazi
> > <krisman@...labora.com> wrote:
> >>
> >> Since FAN_FS_ERROR memory must be pre-allocated, limit a single group
> >> from watching too many file systems at once.  The current scheme
> >> guarantees 1 slot per filesystem, so limit the number of marks with
> >> FAN_FS_ERROR per group.
> >>
> >> Signed-off-by: Gabriel Krisman Bertazi <krisman@...labora.com>
> >> ---
> >>  fs/notify/fanotify/fanotify_user.c | 10 ++++++++++
> >>  include/linux/fsnotify_backend.h   |  1 +
> >>  2 files changed, 11 insertions(+)
> >>
> >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> >> index f1cf863d6f9f..5324890500fc 100644
> >> --- a/fs/notify/fanotify/fanotify_user.c
> >> +++ b/fs/notify/fanotify/fanotify_user.c
> >> @@ -959,6 +959,10 @@ static int fanotify_remove_mark(struct fsnotify_group *group,
> >>
> >>         removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
> >>                                                  umask, &destroy_mark);
> >> +
> >> +       if (removed & FAN_FS_ERROR)
> >> +               group->fanotify_data.error_event_marks--;
> >> +
> >>         if (removed & fsnotify_conn_mask(fsn_mark->connector))
> >>                 fsnotify_recalc_mask(fsn_mark->connector);
> >>         if (destroy_mark)
> >> @@ -1057,6 +1061,9 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
> >>
> >>  static int fanotify_group_init_error_pool(struct fsnotify_group *group)
> >>  {
> >> +       if (group->fanotify_data.error_event_marks >= FANOTIFY_DEFAULT_FEE_POOL)
> >> +               return -ENOMEM;
> >
> > Why not try to mempool_resize()?
>
> Jan suggested we might not need to bother with it, but I can do that for
> the next version.
>
> > Also, I did not read the rest of the patches yet, but don't we need two
> > slots per mark? one for alloc-pre-enqueue and one for free-post-dequeue?
>
> I don't understand what you mean by two slots for alloc-pre-enqueue and
> free-post-dequeue.  I suspect it is no longer necessary now that
> FAN_FS_ERROR is handled like any other event on enqueue/dequeue, but can
> you confirm or clarify?
>

What I meant was, your code is counting error_event_marks.
Every mark accounts for either 1 or 0 queued events, because more
errors from the same fs would merge with the single queued event.

I thought we could have one more event per fs being copied to user
but really we could potentially have many events allocated, before
being "merged", so my calculation was wrong.

Anyway, as Jan explained, the limited size of mempool does not doom
allocations to fail, so we probably have nothing to worry about and
there is no need to mempool_resize().

Thanks,
Amir.