archive mirror
 help / color / mirror / Atom feed
From: Jan Kara <>
To: Amir Goldstein <>
Cc: Jan Kara <>,
	linux-fsdevel <>,
	linux-kernel <>,
	syzkaller-bugs <>,
	syzbot <>
Subject: Re: general protection fault in fanotify_handle_event
Date: Tue, 23 Apr 2019 18:27:55 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Fri 19-04-19 12:33:02, Amir Goldstein wrote:
> On Thu, Apr 18, 2019 at 8:14 PM syzbot
> <> wrote:
> >
> > syzbot has bisected this bug to:
> >
> > commit 77115225acc67d9ac4b15f04dd138006b9cd1ef2
> > Author: Amir Goldstein <>
> > Date:   Thu Jan 10 17:04:37 2019 +0000
> >
> >      fanotify: cache fsid in fsnotify_mark_connector
> >
> > bisection log:
> > start commit:   3f018f4a Add linux-next specific files for 20190418
> > git tree:       linux-next
> > final crash:
> > console output:
> > kernel config:
> > dashboard link:
> > syz repro:
> >
> > Reported-by:
> > Fixes: 77115225acc6 ("fanotify: cache fsid in fsnotify_mark_connector")
> >

Hi Amir!

> It looks like lockless access to mark->connector is not safe as there is
> nothing preventing a reader from seeing a mark on object list without
> seeing the mark->connector assignment.

Yes, that seems to be possible.

> It made me wonder if (!mark->connector) check in fsnotify_put_mark() is safe.
> I couldn't find any call site where that would be a problem, but perhaps
> we should be more careful?

Why? That check is there really only to catch destruction of a mark that
was never attached (i.e., allocated but later never used due to some
error). Otherwise we should always have mark->connector set.

> Anyway, it seems that fsnotify_put_mark() uses the non NULL mark->connector
> as the indication that mark is on object list, so just assigning mark->connector
> before adding to object list won't do.
> Since a reference of mark is our guaranty that mark->connector is not
> going away, I guess we could do opportunistic test for non NULL
> mark->connector from lockless path, if that fails, we check again
> with mark->lock held and if that fails something went wrong.
> Another option is to teach fsnotify_first_mark() and fsnotify_next_mark()
> to skip over marks with NULL mark->connector.
> What do you think? Did I over complicate this?

I'd prefer if we could make only fully initialized marks visible on
connector's list. Just to make things simple in the fast path of handling
events. So I'd just set mark->connector before adding mark to connector's
list and having smp_wmb() there to force ordering in
fsnotify_add_mark_list(). And we should use READ_ONCE() as a counter-part
to this in fanotify_get_fsid(). It is somewhat unfortunate that there are
three different ways how fsnotify_add_mark_list() can add mark to a list so
we may need to either repeat this in three different places or just use
some macro magic like:

#define fanotify_attach_obj_list(where, mark, conn, head) \
	do { \
		mark->connector = conn; \
		smp_wmb(); \
		hlist_add_##where##_rcu(&mark->obj_list, head); \
	} while (0)

And I would not really worry about fsnotify_put_mark() - if it ever sees
mark->connector set, mark really is on the connector's list and
fsnotify_put_mark() does the right thing (i.e. locks connector->lock if

Jan Kara <>

  reply	other threads:[~2019-04-23 16:28 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-18 17:06 general protection fault in fanotify_handle_event syzbot
2019-04-18 17:14 ` syzbot
2019-04-19  9:33   ` Amir Goldstein
2019-04-23 16:27     ` Jan Kara [this message]
2019-04-23 18:05       ` Amir Goldstein
2019-04-24 11:46         ` Jan Kara
2019-04-26 10:55   ` Jan Kara
2019-04-26 11:13     ` syzbot
2019-04-29  2:10       ` Jan Kara
2019-04-29  5:36         ` syzbot

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:

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

  git send-email \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).