On Tue, Feb 13, 2018 at 11:10 PM, Shakeel Butt <shakeelb@xxxxxxxxxx> wrote: > On Mon, Feb 12, 2018 at 10:30 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >> On Thu, Jan 25, 2018 at 10:36 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >>> On Thu, Jan 25, 2018 at 10:20 PM, Shakeel Butt <shakeelb@xxxxxxxxxx> wrote: >>>> On Wed, Jan 24, 2018 at 11:51 PM, Amir Goldstein <amir73il@xxxxxxxxx> 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. >> 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. > >> 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. >> 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.
