Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Amir Goldstein <amir73il@gmail.com>
To: Jan Kara <jack@suse.cz>
Cc: linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: Ignore mask handling in fanotify_group_event_mask()
Date: Mon, 25 May 2020 11:52:54 +0300
Message-ID: <CAOQ4uxiL8W4S7qdc+AOJCf0GND0K_EgxuxKY1uhY3Qbvi1RAVA@mail.gmail.com> (raw)
In-Reply-To: <20200525072322.GG14199@quack2.suse.cz>

On Mon, May 25, 2020 at 10:23 AM Jan Kara <jack@suse.cz> wrote:
>
> On Sat 23-05-20 20:14:58, Amir Goldstein wrote:
> > On Thu, May 21, 2020 at 9:10 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Thu, May 21, 2020 at 7:24 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > Hello Amir!
> > > >
> > > > I was looking into backporting of commit 55bf882c7f13dd "fanotify: fix
> > > > merging marks masks with FAN_ONDIR" and realized one oddity in
> > > > fanotify_group_event_mask(). The thing is: Even if the mark mask is such
> > > > that current event shouldn't trigger on the mark, we still have to take
> > > > mark's ignore mask into account.
> > > >
> > > > The most realistic example that would demonstrate the issue that comes to my
> > > > mind is:
> > > >
> > > > mount mark watching for FAN_OPEN | FAN_ONDIR.
> > > > inode mark on a directory with mask == 0 and ignore_mask == FAN_OPEN.
> > > >
> > > > I'd expect the group will not get any event for opening the dir but the
> > > > code in fanotify_group_event_mask() would not prevent event generation. Now
> > > > as I've tested the event currently actually does not get generated because
> > > > there is a rough test in send_to_group() that actually finds out that there
> > > > shouldn't be anything to report and so fanotify handler is actually never
> > > > called in such case. But I don't think it's good to have an inconsistent
> > > > test in fanotify_group_event_mask(). What do you think?
> > > >
> > >
> > > I agree this is not perfect.
> > > I think that moving the marks_ignored_mask line
> > > To the top of the foreach loop should fix the broken logic.
> > > It will not make the code any less complicated to follow though.
> > > Perhaps with a comment along the lines of:
> > >
> > >              /* Ignore mask is applied regardless of ISDIR and ON_CHILD flags */
> > >              marks_ignored_mask |= mark->ignored_mask;
> > >
> > > Now is there a real bug here?
> > > Probably not because send_to_group() always applied an ignore mask
> > > that is greater or equal to that of fanotify_group_event_mask().
> > >
> >
> > That's a wrong statement of course.
> > We do need to re-apply the ignore mask when narrowing the event mask.
> >
> > Exposing the bug requires a "compound" event.
> >
> > The only case of compound event I could think of is this:
> >
> > mount mark with mask == 0 and ignore_mask == FAN_OPEN. inode mark
> > on a directory with mask == FAN_EXEC | FAN_EVENT_ON_CHILD.
> >
> > The event: FAN_OPEN | FAN_EXEC | FAN_EVENT_ON_CHILD
> > would be reported to group with the FAN_OPEN flag despite the
> > fact that FAN_OPEN is in ignore mask of mount mark because
> > the mark iteration loop skips over non-inode marks for events
> > on child.
> >
> > I'll try to work that case into the relevant LTP test to prove it and
> > post a fix.
>
> Ha, that's clever. But FAN_EXEC does not exist in current fanotify. We only
> have FAN_OPEN_EXEC... And I don't think we have any compound events.
>

Typo. I meant FAN_OPEN_EXEC and you can see from LTP test
we do have at least this one compound event.

We could also split it if we wanted to, but no reason to do it now.

Thanks,
Amir.

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-21 16:24 Jan Kara
2020-05-21 18:10 ` Amir Goldstein
2020-05-23 17:14   ` Amir Goldstein
2020-05-25  7:23     ` Jan Kara
2020-05-25  8:52       ` Amir Goldstein [this message]
2020-05-25 12:42         ` Jan Kara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAOQ4uxiL8W4S7qdc+AOJCf0GND0K_EgxuxKY1uhY3Qbvi1RAVA@mail.gmail.com \
    --to=amir73il@gmail.com \
    --cc=jack@suse.cz \
    --cc=linux-fsdevel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git