linux-kernel - Re: [PATCH v4] inotify: Increase default inotify.max_user

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: <CAOQ4uxj26Pb_zyErgnpmKeMThrxnRuO5PAF=igt9mvr_80BkCQ@mail.gmail.com>
Date:   Mon, 9 Nov 2020 06:58:52 +0200
From:   Amir Goldstein <amir73il@...il.com>
To:     Waiman Long <longman@...hat.com>
Cc:     Jan Kara <jack@...e.cz>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Luca BRUNO <lucab@...hat.com>
Subject: Re: [PATCH v4] inotify: Increase default inotify.max_user_watches
 limit to 1048576

On Mon, Nov 9, 2020 at 6:00 AM Waiman Long <longman@...hat.com> wrote:
>
> The default value of inotify.max_user_watches sysctl parameter was set
> to 8192 since the introduction of the inotify feature in 2005 by
> commit 0eeca28300df ("[PATCH] inotify"). Today this value is just too
> small for many modern usage. As a result, users have to explicitly set
> it to a larger value to make it work.
>
> After some searching around the web, these are the
> inotify.max_user_watches values used by some projects:
>  - vscode:  524288
>  - dropbox support: 100000
>  - users on stackexchange: 12228
>  - lsyncd user: 2000000
>  - code42 support: 1048576
>  - monodevelop: 16384
>  - tectonic: 524288
>  - openshift origin: 65536
>
> Each watch point adds an inotify_inode_mark structure to an inode to
> be watched. It also pins the watched inode.
>
> Modeled after the epoll.max_user_watches behavior to adjust the default
> value according to the amount of addressable memory available, make
> inotify.max_user_watches behave in a similar way to make it use no more
> than 1% of addressable memory within the range [8192, 1048576].
>
> For 64-bit archs, inotify_inode_mark plus 2 vfs inode have a size that
> is a bit over 1 kbytes (1284 bytes with my x86-64 config).

The sentence above seems out of context (where did the 2 vfs inodes
come from?). Perhaps the comment in the patch should go here above.

> That means
> a system with 128GB or more memory will likely have the maximum value
> of 1048576 for inotify.max_user_watches. This default should be big
> enough for most use cases.
>
> [v3: increase inotify watch cost as suggested by Amir and Honza]

That patch change log usually doesn't belong in the git commit
because it adds little value in git log perspective.

If you think that the development process is relevant to understanding
the patch (like the discussion leading to the formula of the cost)
then a Link: to the discussion on lore.kernel.org would serve the
commit better.

>
> Signed-off-by: Waiman Long <longman@...hat.com>

Apart from this minor nits,
Reviewed-by: Amir Goldstein <amir73il@...il.com>

> ---
>  fs/notify/inotify/inotify_user.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index 186722ba3894..24d17028375e 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -37,6 +37,15 @@
>
>  #include <asm/ioctls.h>
>
> +/*
> + * An inotify watch requires allocating an inotify_inode_mark structure as
> + * well as pinning the watched inode. Doubling the size of a VFS inode
> + * should be more than enough to cover the additional filesystem inode
> + * size increase.
> + */
> +#define INOTIFY_WATCH_COST     (sizeof(struct inotify_inode_mark) + \
> +                                2 * sizeof(struct inode))
> +
>  /* configurable via /proc/sys/fs/inotify/ */
>  static int inotify_max_queued_events __read_mostly;
>
> @@ -801,6 +810,18 @@ SYSCALL_DEFINE2(inotify_rm_watch, int, fd, __s32, wd)
>   */
>  static int __init inotify_user_setup(void)
>  {
> +       unsigned long watches_max;
> +       struct sysinfo si;
> +
> +       si_meminfo(&si);
> +       /*
> +        * Allow up to 1% of addressable memory to be allocated for inotify
> +        * watches (per user) limited to the range [8192, 1048576].
> +        */
> +       watches_max = (((si.totalram - si.totalhigh) / 100) << PAGE_SHIFT) /
> +                       INOTIFY_WATCH_COST;
> +       watches_max = clamp(watches_max, 8192UL, 1048576UL);
> +
>         BUILD_BUG_ON(IN_ACCESS != FS_ACCESS);
>         BUILD_BUG_ON(IN_MODIFY != FS_MODIFY);
>         BUILD_BUG_ON(IN_ATTRIB != FS_ATTRIB);
> @@ -827,7 +848,7 @@ static int __init inotify_user_setup(void)
>
>         inotify_max_queued_events = 16384;
>         init_user_ns.ucount_max[UCOUNT_INOTIFY_INSTANCES] = 128;
> -       init_user_ns.ucount_max[UCOUNT_INOTIFY_WATCHES] = 8192;
> +       init_user_ns.ucount_max[UCOUNT_INOTIFY_WATCHES] = watches_max;
>
>         return 0;
>  }
> --
> 2.18.1
>