From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:41073 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753134AbeBZNGH (ORCPT ); Mon, 26 Feb 2018 08:06:07 -0500 Date: Mon, 26 Feb 2018 14:06:05 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , linux-fsdevel , Shakeel Butt Subject: Re: [PATCH 1/2] fanotify: Avoid lost events due to ENOMEM for unlimited queues Message-ID: <20180226130605.v7o3ki53j3aqgdne@quack2.suse.cz> References: <20180221154430.19195-1-jack@suse.cz> <20180221154430.19195-2-jack@suse.cz> <20180222162249.5zqyieudl7cdxecv@quack2.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu 22-02-18 20:34:48, Amir Goldstein wrote: > On Thu, Feb 22, 2018 at 6:22 PM, Jan Kara wrote: > > On Thu 22-02-18 18:04:36, Amir Goldstein wrote: > >> On Wed, Feb 21, 2018 at 5:44 PM, Jan Kara 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 SUSE Labs, CR