From: Shakeel Butt <shakeelb@google.com>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>, Yang Shi <yang.s@alibaba-inc.com>,
Michal Hocko <mhocko@kernel.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Linux MM <linux-mm@kvack.org>,
LKML <linux-kernel@vger.kernel.org>,
linux-api@vger.kernel.org
Subject: Re: [PATCH v2] fs: fsnotify: account fsnotify metadata to kmemcg
Date: Tue, 13 Feb 2018 14:20:00 -0800 [thread overview]
Message-ID: <CALvZod7FTNzoGfGnaorqjk4KEsxJFdz1pApHi04P1cF10ejxpQ@mail.gmail.com> (raw)
In-Reply-To: <CAOQ4uxghwNg9Ni23EQA-971-qAaTNceSZS2MSvK06uEjoXG_yg@mail.gmail.com>
On Tue, Feb 13, 2018 at 1:54 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, Feb 13, 2018 at 11:10 PM, Shakeel Butt <shakeelb@google.com> wrote:
>> On Mon, Feb 12, 2018 at 10:30 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> On Thu, Jan 25, 2018 at 10:36 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>> On Thu, Jan 25, 2018 at 10:20 PM, Shakeel Butt <shakeelb@google.com> wrote:
>>>>> On Wed, Jan 24, 2018 at 11:51 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>>>>>
>>>>>> There is a nicer alternative, instead of failing the file access,
>>>>>> an overflow event can be queued. I sent a patch for that and Jan
>>>>>> agreed to the concept, but thought we should let user opt-in for this
>>>>>> change:
>>>>>> https://marc.info/?l=linux-fsdevel&m=150944704716447&w=2
>>>>>>
>>>>>> So IMO, if user opts-in for OVERFLOW instead of ENOMEM,
>>>>>> charging the listener memcg would be non controversial.
>>>>>> Otherwise, I cannot say that starting to charge the listener memgc
>>>>>> for events won't break any application.
>>>>>>
>>>>>
>>>
>>> Shakeel, Jan,
>>>
>>> Reviving this thread and adding linux-api, because I think it is important to
>>> agree on the API before patches.
>>>
>>> The last message on the thread you referenced suggest an API change
>>> for opting in for Q_OVERFLOW on ENOMEM:
>>> https://marc.info/?l=linux-api&m=150946878623441&w=2
>>>
>>> However, the suggested API change in in fanotify_mark() syscall and
>>> this is not the time when fsnotify_group is initialized.
>>> I believe for opting-in to accounting events for listener, you
>>> will need to add an opt-in flag for the fanotify_init() syscall.
>>>
>>
>> I thought the reason to opt-in "charge memory to listener" was the
>> risk of oom-killing the listener but it is now clear that there will
>> be no oom-kills on memcg hitting its limit (no oom-killing listener
>> risk). In my (not so strong) opinion we should only opt-in for
>> receiving the {FAN|IN}_Q_OVERFLOW event on ENOMEM but always charge
>> the memory for events to the listener's memcg if kmem accounting is
>> enabled.
>>
>
> I agree that charging listener's memcg is preferred, but it is still a change
> of behavior, because if attacker can allocate memory from listener's memcg,
> then attacker can force overflow and hide the traces of its own filesystem
> operations.
>
ACK.
>>> Something like FAN_GROUP_QUEUE (better name is welcome)
>>> which is mutually exclusive (?) with FAN_UNLIMITED_QUEUE.
>>>
How about FAN_CHARGE_MEMCG?
>>
>> There is no need to make them mutually exclusive. One should be able
>> to request an unlimited queue limited by available memory on system
>> (with no kmem charging) or limited by limit of the listener's memcg
>> (with kmem charging).
>
> OK.
>
>>
>>> The question is, do we need the user to also explicitly opt-in for
>>> Q_OVERFLOW on ENOMEM with FAN_Q_ERR mark mask?
>>> Should these 2 new APIs be coupled or independent?
>>>
>>
>> Are there any error which are not related to queue overflows? I see
>> the mention of ENODEV and EOVERFLOW in the discussion. If there are
>> such errors and might be interesting to the listener then we should
>> have 2 independent APIs.
>>
>
> These are indeed 2 different use cases.
> A Q_OVERFLOW event is only expected one of ENOMEM or
> EOVERFLOW in event->fd, but other events (like open of special device
> file) can have ENODEV in event->fd.
>
> But I am not convinced that those require 2 independent APIs.
> Specifying FAN_Q_ERR means that the user expects to reads errors
> from event->fd.
>
Can you please explain what you mean by 2 independent APIs? I thought
"no independent APIs" means FAN_Q_ERR can only be used with
FAN_Q_OVERFLOW and without FAN_Q_OVERFLOW, FAN_Q_ERR is ignored. Is
that right or I misunderstood?
>>> Another question is whether FAN_GROUP_QUEUE may require
>>> less than CAP_SYS_ADMIN? Of course for now, this is only a
>>> semantic change, because fanotify_init() requires CAP_SYS_ADMIN
>>> but as the documentation suggests, this may be relaxed in the future.
>>>
>>
>> I think there is no need for imposing CAP_SYS_ADMIN for requesting to
>> charge self for the event memory.
>>
>
> Certainly. The question is whether the flag combination
> FAN_GROUP_QUEUE|FAN_UNLIMITED_QUEUE could relax the
> CAP_SYS_ADMIN requirement that is imposed by FAN_UNLIMITED_QUEUE
> by itself.
>
Oh, I agree with relaxing CAP_SYS_ADMIN requirement if both flags are given.
> Note that FAN_UNLIMITED_MARKS cannot relax CAP_SYS_ADMIN
> even though marks are already accounted to listener memcg. This is because
> most of the memory consumption in this case comes from marks pinning the
> watched inodes to cache and not from the marks themselves.
>
thanks,
Shakeel
next prev parent reply other threads:[~2018-02-13 22:20 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-27 18:22 [PATCH v2] fs: fsnotify: account fsnotify metadata to kmemcg Yang Shi
2017-10-28 14:19 ` Amir Goldstein
2017-10-29 2:39 ` Matthew Wilcox
2017-10-30 12:43 ` Jan Kara
2017-10-30 16:39 ` Yang Shi
2017-10-31 10:12 ` Jan Kara
2017-10-31 16:44 ` Yang Shi
2017-11-01 15:15 ` Jan Kara
2017-11-09 13:54 ` Michal Hocko
2017-11-13 19:10 ` Yang Shi
2017-11-14 9:39 ` Michal Hocko
2017-11-14 17:32 ` Yang Shi
2017-11-15 9:31 ` Jan Kara
2018-01-19 15:02 ` Shakeel Butt
2018-01-22 20:31 ` Amir Goldstein
2018-01-24 10:34 ` Jan Kara
2018-01-24 11:12 ` Amir Goldstein
2018-01-25 1:08 ` Shakeel Butt
2018-01-25 1:54 ` Al Viro
2018-01-25 2:15 ` Shakeel Butt
2018-01-25 7:51 ` Amir Goldstein
2018-01-25 20:20 ` Shakeel Butt
2018-01-25 20:36 ` Amir Goldstein
2018-02-13 6:30 ` Amir Goldstein
2018-02-13 21:10 ` Shakeel Butt
2018-02-13 21:54 ` Amir Goldstein
2018-02-13 22:20 ` Shakeel Butt [this message]
2018-02-14 1:59 ` Shakeel Butt
2018-02-14 8:38 ` Amir Goldstein
2018-02-19 13:50 ` Jan Kara
2018-02-19 19:07 ` Amir Goldstein
2018-02-20 12:43 ` Jan Kara
2018-02-20 19:20 ` Shakeel Butt
2018-02-20 20:30 ` Amir Goldstein
2018-02-14 9:00 ` Amir Goldstein
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CALvZod7FTNzoGfGnaorqjk4KEsxJFdz1pApHi04P1cF10ejxpQ@mail.gmail.com \
--to=shakeelb@google.com \
--cc=amir73il@gmail.com \
--cc=jack@suse.cz \
--cc=linux-api@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=yang.s@alibaba-inc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).