Re: [PATCH 1/2] fanotify: Avoid lost events due to ENOMEM for unlimited queues

From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Shakeel Butt <shakeelb@google.com>
Subject: Re: [PATCH 1/2] fanotify: Avoid lost events due to ENOMEM for unlimited queues
Date: Mon, 26 Feb 2018 14:06:05 +0100	[thread overview]
Message-ID: <20180226130605.v7o3ki53j3aqgdne@quack2.suse.cz> (raw)
In-Reply-To: <CAOQ4uxhct0D3GBhk=62W+0oqie3kaxe6ciQTwLLBUCc93qCFOw@mail.gmail.com>

On Thu 22-02-18 20:34:48, Amir Goldstein wrote:
> On Thu, Feb 22, 2018 at 6:22 PM, Jan Kara <jack@suse.cz> wrote:
> > On Thu 22-02-18 18:04:36, Amir Goldstein wrote:
> >> On Wed, Feb 21, 2018 at 5:44 PM, Jan Kara <jack@suse.cz> wrote:
> >> > Fanotify queues of unlimited length do not expect events can be lost.
> >> > Since these queues are used for system auditing and other security
> >>
> >> Change looks good to me, but the reasoning is going backwards.
> >> IMO, you should mention that we are going to change -ENOMEM
> >> behavior to result in Q_OVERFLOW and user does not expect
> >> Q_OVERFLOW from an UNLIMITED queue.
> >
> > See below.
> >
> >> > related tasks, loosing events can even have security implications. So
> >> > avoid loosing events due to failure to allocate memory by making event
> >> > allocation use __GFP_NOFAIL. Since the allocation is small (32-bytes),
> >> > currently there is no practical difference of this change but still it
> >> > is good to have the expectation explicitely documented.
> >> >
> >>
> >> But if currently allocations cannot fail, then why do we need patch
> >> 2/2 (queue overflow event). It is because small allocations can fail when
> >> accounted to non-root memcg?
> >
> > Yes. So how about changelog like:
> >
> > Fanotify queues of unlimited length do not expect events can be lost.
> > Since these queues are used for system auditing and other security
> > related tasks, loosing events can even have security implications.
> > Currently, since the allocation is small (32-bytes), it cannot fail
> > however when we start accounting events in memcgs, allocation can start
> > failing. So avoid loosing events due to failure to allocate memory by
> > making event allocation use __GFP_NOFAIL.
> >
> 
> Very good.

Can I add your reviewed-by to the patch then?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR