Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <47112ee6-9686-a272-183e-ec2e6cdeff2e@openvz.org>
Date:   Sat, 13 Aug 2022 07:16:46 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [RFC PATCH] kernfs: enable per-inode limits for all xattr types
Content-Language: en-US
To:     Christian Brauner <brauner@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Tejun Heo <tj@kernel.org>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Hugh Dickins <hughd@google.com>, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org,
        =?UTF-8?Q?Michal_Koutn=c3=bd?= <mkoutny@suse.com>,
        Shakeel Butt <shakeelb@google.com>,
        Roman Gushchin <roman.gushchin@linux.dev>,
        Muchun Song <songmuchun@bytedance.com>,
        Michal Hocko <mhocko@suse.com>,
        Johannes Weiner <hannes@cmpxchg.org>, kernel@openvz.org
References: <b816f58a-ce25-c079-c6b3-a3406df246f9@openvz.org>
 <8ace5255-ce23-0a05-2d50-1455dd68acb6@openvz.org>
 <20220812102052.adiqtlsxx273t7wk@wittgenstein>
From:   Vasily Averin <vvs@openvz.org>
In-Reply-To: <20220812102052.adiqtlsxx273t7wk@wittgenstein>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 8/12/22 13:20, Christian Brauner wrote:
> So iiuc, for tmpfs this is effectively a per-inode limit of 128 xattrs
> and 128 user xattrs; nr_inodes * 256. I honestly have no idea if there
> are legimitate use-cases to want more. But there's at least a remote
> chance that this might break someone.
> 
> Apart from
> 
>> Currently it's possible to create a huge number of xattr per inode,
> 
> what exactly is this limit protecting against? In other words, the
> commit message misses the motivation for the patch.

This should prevent softlockup and hung_task_panic caused by slow search
in xattrs->list in simple_xattr_set() and _get()

Adding new xattr checks all present entries in the list,
so execution time linearly depends on the number of such entries.

To avoid this problem I decided to limit somehow the number of entries in the list.
As an alternative Tejun advises to switch this list to something like rb-tree,
now I think he is right.

> I'd also prefer to see a summary of what filesystems are affected by
> this change. Afaict, the patchset doesn't change anything for kernfs
> users such as cgroup{1,2} so it should only be tmpfs and potential
> future users of the simple_xattr_* api.

This affect all file systems used  simple_xattr_* API: sysfs and tmpfs.

Now I'll try to follow Tejun's advice and switch the xatrrs list to rb-tree.
Unfortunately, I doubt that I will have enough time to finish before the merge
window closes.

Thank you,
	Vasily Averin