Re: [PATCH v2] fs: fsnotify: account fsnotify metadata to kmemcg

On Tue, Feb 13, 2018 at 11:10 PM, Shakeel Butt [off-list ref] wrote:

On Mon, Feb 12, 2018 at 10:30 PM, Amir Goldstein [off-list ref] wrote:

quoted

On Thu, Jan 25, 2018 at 10:36 PM, Amir Goldstein [off-list ref] wrote:

quoted

On Thu, Jan 25, 2018 at 10:20 PM, Shakeel Butt [off-list ref] wrote:

quoted

On Wed, Jan 24, 2018 at 11:51 PM, Amir Goldstein [off-list ref] wrote:

quoted

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.

quoted

Something like FAN_GROUP_QUEUE  (better name is welcome)
which is mutually exclusive (?) with FAN_UNLIMITED_QUEUE.

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.

quoted

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.

quoted

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.

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,
Amir.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>