[Bug 76851] New: inotify_rm_watch(2) unspecified behavior

[Bug 76851] New: inotify_rm_watch(2) unspecified behavior

[bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r]

https://bugzilla.kernel.org/show_bug.cgi?id=76851

            Bug ID: 76851
           Summary: inotify_rm_watch(2) unspecified behavior
           Product: Documentation
           Version: unspecified
          Hardware: All
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P1
         Component: man-pages
          Assignee: documentation_man-pages-ztI5WcYan/vQLgFONoPN62D2FQJk+8+b@public.gmane.org
          Reporter: junk.jsmith+kernel.org-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
        Regression: No

The inotify interface leaves unspecified what happens to potentially "pending"
watch events (i.e., caught by the kernel but not yet read()) after an
inotify_rm_watch() call.

IN_IGNORE flagged events *allow* inotify_rm_watch() calls to be monitored
asynchronously, but it is not stated whether triggering actions between a most
recent read() and an inotify_rm_watch() call *require* all other watch events
to be handled so when users key map inotify_event wd fields to local objects.

Similarly, a conservative user would need to keep a queue of removed objects
formerly associated with a watch descriptor if it is assumed possible that the
kernel can both deliver events on an rm'd wd and reuse wds before the last
pertinent event has been read.

If the purpose of IN_IGNORE being generated on the watch removal is to protect
against poll/blocking-read vs. inotify_rm_watch() races, it should be plainly
stated. Likewise, if it is needed for safely reacting to "stale" events, this
should be explained. I believe that the current language of inotify_rm_watch(2)
and the read(2) section of inotify(7) leave both interpretations defensible,
which is sub-optimal.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 76851] inotify_rm_watch(2) unspecified behavior

[bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r]

https://bugzilla.kernel.org/show_bug.cgi?id=76851

--- Comment #1 from Jeff <junk.jsmith+kernel.org-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
For what it's worth, neither Documentation/filesystems/inotify.txt nor
fs/notify/inotify/inotify_user.c itself in the kernel source plainly comment on
the issue. As far as I can tell, nothing besides the implementation
standardizes the behavior.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 76851] inotify_rm_watch(2) unspecified behavior

[bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r]

https://bugzilla.kernel.org/show_bug.cgi?id=76851

Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

--- Comment #2 from Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
(In reply to Jeff from comment #0)
> The inotify interface leaves unspecified what happens to potentially
> "pending" watch events (i.e., caught by the kernel but not yet read()) after
> an inotify_rm_watch() call.
> 
> IN_IGNORE flagged events *allow* inotify_rm_watch() calls to be monitored
> asynchronously, but it is not stated whether triggering actions between a
> most recent read() and an inotify_rm_watch() call *require* all other watch
> events to be handled so when users key map inotify_event wd fields to local
> objects.
> 
> Similarly, a conservative user would need to keep a queue of removed objects
> formerly associated with a watch descriptor if it is assumed possible that
> the kernel can both deliver events on an rm'd wd and reuse wds before the
> last pertinent event has been read.
> 
> If the purpose of IN_IGNORE being generated on the watch removal is to
> protect against poll/blocking-read vs. inotify_rm_watch() races, it should
> be plainly stated. Likewise, if it is needed for safely reacting to "stale"
> events, this should be explained. I believe that the current language of
> inotify_rm_watch(2) and the read(2) section of inotify(7) leave both
> interpretations defensible, which is sub-optimal.


Jeff, thanks for the report. You're quite correct that there's no spec for the
behavior (and your points about the implications of the lack of such spec are
right on the money). However, man-pages is (sadly) not a spec, at least given
current development practices. Nevertheless, something should be documented.

One thing that would have been useful in the bug report would have been a
statement of what the existing behavior actually is. As far as I can see, as
things stand, if a watch is removed, pending unread events for that watch
descriptor remain pending. Can you confirm?

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 76851] inotify_rm_watch(2) unspecified behavior

[bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r]

https://bugzilla.kernel.org/show_bug.cgi?id=76851

--- Comment #3 from Jeff <junk.jsmith+kernel.org-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
(In reply to Michael Kerrisk from comment #2)
> Jeff, thanks for the report. You're quite correct that there's no spec for
> the behavior (and your points about the implications of the lack of such
> spec are right on the money). However, man-pages is (sadly) not a spec, at
> least given current development practices. Nevertheless, something should be
> documented.
> 
> One thing that would have been useful in the bug report would have been a
> statement of what the existing behavior actually is. As far as I can see, as
> things stand, if a watch is removed, pending unread events for that watch
> descriptor remain pending. Can you confirm?

Michael, I can confirm that this behavior, for at least 3.x kernels. I didn't
test this or dig through the kernel source for anything older though.

I've not been involved enough in the kernel scene to know best how to approach
this, so far as claiming what exactly seemed wrong, but the documentation
seemed like the most innocuous reasonable place to start. I frankly don't even
know where "standard" system call APIs are supposed to be officially specified,
if anywhere.

I tried to be dispassionate in my initial report since I don't know what
peoples' intents were, but it's my opinion that the current behavior is bad
enough to warrant changing. I've never used any other multiplexing interface
that can deliver events after a handle de-registration. I've also never seen
anyone's inotify-handling code other than my own that even tried to safely
handle late queued updates, and I suspect that some fraction of those people
are ignorant of the hazards.

As I tried to imply above, extant IN_IGNORE event generation would provide
sufficient safety from client poll/rm/read deadlock races in the event of an
implementation change, but I don't have the slightest clue about tactfully
approaching the kernel community with this, being a non-established individual.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 76851] inotify_rm_watch(2) unspecified behavior

[bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r]

https://bugzilla.kernel.org/show_bug.cgi?id=76851

--- Comment #4 from Jeff <junk.jsmith+kernel.org-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
I should also add that my assumptions about watch descriptor reuse may very
well be overly cautious. If there is a guarantee that a wd will never be
returned by a inotify_add_watch() call until nothing with that wd remains in
the read queue, clients could handle deregistration semi-synchronously by
knowing to drop locally unmapped events.

This is something that I've not investigated recently, and I am completely
uncertain of any guarantees, but the kernel folks may have already done this
and just not passed the information along to users.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 76851] inotify_rm_watch(2) unspecified behavior

[bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r]

https://bugzilla.kernel.org/show_bug.cgi?id=76851

Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |xypron.glpk-Mmb7MZpHnFY@public.gmane.org

--- Comment #5 from Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org> ---
Created attachment 137681
  --> https://bugzilla.kernel.org/attachment.cgi?id=137681&action=edit
test_wd_reuse.c - Test watch descriptor reuse

The test program demonstrates that inotify_add_watch may return a watch
descriptor ID for which events still exist on the inotify queue.

It creates one watch for file "0" to demonstrate the problem.
Afterwards it creates and removes watches again and again just to force
idr_alloc_cyclic to reach INT_MAX.
Then events are created for file "0" on the queue.
The watch for "0" is removed.
A watch for another file is created.

Example program output (after a few hours, on Linux 3.14.4 x86_64):

...
2147459044      2147467235      2147475426      2147483617      2147483647      
Preparation done
BINGO Collision detected
Watch descriptor 1 for /tmp/test/8192
Watch descriptor 1 for /tmp/test/0

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 76851] inotify_rm_watch(2) unspecified behavior

[bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r]

https://bugzilla.kernel.org/show_bug.cgi?id=76851

--- Comment #6 from Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org> ---
Watch descriptor IDs are returned by inotify_add_watch.
When calling inotify_rm_watch an IN_IGNORE is placed on the inotify queue
pointing to the ID of the removed watch.

inotify_add_watch should not return a watch descriptor ID for which events are
still on the queue but should return an unused ID.

Unfortunately the existing Kernel code does not provide such a guarantee.

Actually in rare cases watch descriptor IDs are returned by inotify_add_watch
for which events are still on the inotify queue.

I verified the problem exists with the appended test program (test_wd_reuse.c).

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 76851] inotify_rm_watch(2) unspecified behavior

[bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r]

https://bugzilla.kernel.org/show_bug.cgi?id=76851

--- Comment #7 from Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
(In reply to Jeff from comment #3)

Hi Jeff,

> I tried to be dispassionate in my initial report since I don't know what
> peoples' intents were, but it's my opinion that the current behavior is bad
> enough to warrant changing. I've never used any other multiplexing interface
> that can deliver events after a handle de-registration. I've also never seen
> anyone's inotify-handling code other than my own that even tried to safely
> handle late queued updates, 

(I have some code that handles it.)

> and I suspect that some fraction of those people
> are ignorant of the hazards.

I expect you are right,

> As I tried to imply above, extant IN_IGNORE event generation would provide
> sufficient safety from client poll/rm/read deadlock races in the event of an
> implementation change, but I don't have the slightest clue about tactfully
> approaching the kernel community with this, being a non-established
> individual.

I'm not sure I understand the sentence "As I tried to imply...", but I'm
guessing you mean that it would be sufficient to discard all pending events
except the IN_IGNORED event, right?

It depends on the use case, I guess, some folk would, I expect, want to do log
tracking -- that is, record all events that happened on a file. Such
applications wouldn't want events to be discarded.

Cheers,

Michael

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 76851] inotify_rm_watch(2) unspecified behavior

[bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r]

https://bugzilla.kernel.org/show_bug.cgi?id=76851

--- Comment #8 from Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
(In reply to Jeff from comment #4)
> I should also add that my assumptions about watch descriptor reuse may very
> well be overly cautious. If there is a guarantee that a wd will never be
> returned by a inotify_add_watch() call until nothing with that wd remains in
> the read queue [...]


And that's the nub of the issue of course. As I suspected, and Heinrich has
demonstrated in #5, the kernel does not provide such a guarantee.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Bug 76851] inotify_rm_watch(2) unspecified behavior

[bugzilla-daemon-590EEB7GvNiWaY/ihj7yzEB+6BGkLq7r]

https://bugzilla.kernel.org/show_bug.cgi?id=76851

--- Comment #9 from Michael Kerrisk <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> ---
(In reply to Heinrich Schuchardt from comment #5)
> Created attachment 137681 [details]
> test_wd_reuse.c - Test watch descriptor reuse
> 
> The test program demonstrates that inotify_add_watch may return a watch
> descriptor ID for which events still exist on the inotify queue.
> 
> It creates one watch for file "0" to demonstrate the problem.
> Afterwards it creates and removes watches again and again just to force
> idr_alloc_cyclic to reach INT_MAX.
> Then events are created for file "0" on the queue.
> The watch for "0" is removed.
> A watch for another file is created.
> 
> Example program output (after a few hours, on Linux 3.14.4 x86_64):
> 
> ...
> 2147459044      2147467235      2147475426      2147483617      2147483647  
> 
> Preparation done
> BINGO Collision detected
> Watch descriptor 1 for /tmp/test/8192
> Watch descriptor 1 for /tmp/test/0

Nice work Heinrich! I pretty much suspected this would be the case, even before
looking at the kernel code, and it's nice to see a demonstration[1] of the
problem. (I had in mind to code something similar myself, just to be sure, but
you beat me to it.)

Anyway, this recycling behavior, coupled with "events remain pending after
inotify_rm_watch()" behavior that Jeff reports mean that yes indeed we can
events on a recycled watch descriptor that do indeed belong to a previous
incarnation of that WD. On the other hand, you have to work pretty hard to do
this: you have to fail to read your queued events while at the same time
cycling through INT_MAX watch descriptors. I suppose no application is going to
run into that scenario in a hurry.

Cheers,

Michael

[1] No need to wait for hours when you test, Just rebuild a kernel with the
following edit in fs/notify/inotify/inotify_user.c::inotify_add_to_idr()

-        ret = idr_alloc_cyclic(idr, i_mark, 1, 0, GFP_NOWAIT);
-        ret = idr_alloc_cyclic(idr, i_mark, 1, 500000, GFP_NOWAIT);

That's sufficient as a proof of concept of the problem.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html