Return-Path: Received: from mx2.suse.de ([195.135.220.15]:33512 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727649AbeKNWFr (ORCPT ); Wed, 14 Nov 2018 17:05:47 -0500 Date: Wed, 14 Nov 2018 13:02:47 +0100 From: Jan Kara To: Amir Goldstein Cc: Jan Kara , Matthew Bobrowski , linux-api@vger.kernel.org, linux-fsdevel , Steve Grubb Subject: Re: [PATCH v7 0/4] fanotify: introduce new event mask FAN_OPEN_EXEC and FAN_OPEN_EXEC_PERM Message-ID: <20181114120247.GA16849@quack2.suse.cz> References: <20181113175346.GC12023@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 Wed 14-11-18 05:43:25, Amir Goldstein wrote: > On Tue, Nov 13, 2018 at 8:01 PM Amir Goldstein wrote: > > > > > > LTP tests for this feature are on my 'fanotify-exec' branch here: > > > > https://github.com/matthewbobrowski/ltp/commits/fanotify_exec. The files > > > > that contains the test cases are provided below: > > > > > > > > syscalls/fanotify03: test cases have been updated to cover > > > > FAN_OPEN_EXEC_PERM events > > > > syscalls/fanotify12: newly introduced LTP test file to cover > > > > FAN_OPEN_EXEC events > > > > > > I have been wondering for a while why the testcases passed when ignore mask > > > hasn't been properly treated in fanotify_group_event_mask() but then I > > > realized that the generic code will not even call to fanotify if ignore > > > masks result in clearing the event. > > > > So does that means we have missing test coverage? > > > > This is covered by test case #3 of Matthew's proposed LTP test. > https://github.com/matthewbobrowski/ltp/commit/9e350fe15a5423d896ed0e8e147edc15bee13b42#diff-2bb8ddff24b3a031be0f64354262e587R76 This testcase does not catch the bug we had in fanotify_group_event_mask() because the masking by mark->mask already hides the fact that we failed to apply the ignore mask. What does catch this kind of bug (tested) is a testcase (admittedly somewhat silly) like this: { "inode mark, FAN_OPEN | FAN_OPEN_EXEC events, ignore FAN_OPEN_EXEC", INIT_FANOTIFY_MARK_TYPE(INODE), FAN_OPEN | FAN_OPEN_EXEC, FAN_OPEN_EXEC, 2, {FAN_OPEN, FAN_OPEN} }, A real variant of this would be FAN_OPEN | FAN_OPEN_EXEC on mount, ignore FAN_OPEN on inode. Then we should just get one FAN_OPEN_EXEC but with the bug we'd get FAN_OPEN | FAN_OPEN_EXEC. But creating such test would be slightly more involved. But probably it is worth it. Matthew? Also I have noticed that fanotify12 test has a bug that it reports: fanotify12.c:220: FAIL: Received event: mask=1020, pid=5142 (expected 5142), fd=5 i.e., it reports expected pid instead of expected mask when mask does not match. Can you please fix it Matthew? Honza -- Jan Kara SUSE Labs, CR