[PATCH 1/1] inotify.7: Bug 77111 - watch descriptor reuse

[PATCH 1/1] inotify.7: Bug 77111 - watch descriptor reuse

[Heinrich Schuchardt]

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.

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

Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
---
 man7/inotify.7 | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/man7/inotify.7 b/man7/inotify.7
index 1fc83f8..b1c2f6f 100644
--- a/man7/inotify.7
+++ b/man7/inotify.7
@@ -728,6 +728,18 @@ and the accompanying
 event might be fetched only on the next
 .BR read (2).
 .SH BUGS
+As of Linux 3.14,
+the following bug exists:
+.IP * 3
+.\" FIXME https://bugzilla.kernel.org/show_bug.cgi?id=77111
+.BR inotify_add_watch (2)
+may return a watch descriptor ID released by a prior call to
+.BR inotify_rm_watch (2)
+even if events for this watch descriptor still exist on the inotify queue.
+As a workaround the inotify file descriptor can be read until the queue is
+empty before calling
+.BR inotify_add_watch (2).
+.PP
 .\" FIXME kernel commit 611da04f7a31b2208e838be55a42c7a1310ae321
 .\" implies that unmount events were buggy 2.6.11 to 2.6.36
 .\"
@@ -745,7 +757,7 @@ However, as an unintended effect of other changes,
 since Linux 2.6.36, an
 .B IN_IGNORED
 event is generated in this case.

Re: [PATCH 1/1] inotify.7: Bug 77111 - watch descriptor reuse

[Michael Kerrisk (man-pages)]

[Adding Jan and Eric to the CC, in case they want to comment.]

Background is bugs https://bugzilla.kernel.org/show_bug.cgi?id=76851
and https://bugzilla.kernel.org/show_bug.cgi?id=77111 . The point is:

1. When an inotify watch descriptor is removed, pending unread 
   events remain pending.
2. When allocating a new watch descriptor, a past WD may 
   be recycled.
3. In theory, it could happen that events left over at 1 could
   be interpreted as though they belonged to the filesystem
   object watch in step 2.

On 05/29/2014 07:39 PM, Heinrich Schuchardt wrote:
> 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.

It's unfortunate, but I'm not sure that it's very serious. I mean, in 
order to trigger this bug you need to

0. Remove your watch descriptor (wd1),
1. Leave some unread events for wd1 on the queue. and in the meantime,
2. Cycle through INT_MAX watch descriptors until you reuse wd1.

Unless I'm missing something, the chances of that are vanishingly small.
However, that's not to say that the issue shouldn't be documented. Rather,
if the issue is as unlikely to be hit as it appears to me in the above
formulation, then I thing the tome of the write-up should indicate
that the problem is more theoretical than practical. Perhaps Jan or Eric
has a comment about this.

> Actually in rare cases watch descriptor IDs are returned by inotify_add_watch
> for which events are still on the inotify queue.
> 
> cf. https://bugzilla.kernel.org/show_bug.cgi?id=77111
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
> ---
>  man7/inotify.7 | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/man7/inotify.7 b/man7/inotify.7
> index 1fc83f8..b1c2f6f 100644
> --- a/man7/inotify.7
> +++ b/man7/inotify.7
> @@ -728,6 +728,18 @@ and the accompanying
>  event might be fetched only on the next
>  .BR read (2).
>  .SH BUGS
> +As of Linux 3.14,
> +the following bug exists:
> +.IP * 3
> +.\" FIXME https://bugzilla.kernel.org/show_bug.cgi?id=77111
> +.BR inotify_add_watch (2)
> +may return a watch descriptor ID released by a prior call to
> +.BR inotify_rm_watch (2)
> +even if events for this watch descriptor still exist on the inotify queue.
> +As a workaround the inotify file descriptor can be read until the queue is
> +empty before calling
> +.BR inotify_add_watch (2).
> +.PP

Perhaps a wording something like this is more appropriate:

[[
As at Linux 3.15, the a possible bug exists in the following scenario:

1. When a watch descriptor is removed, any pending unread events for 
   that watch descriptor remain available to read. 
2. As watch descriptors are subsequently allocated with 
   inotify_ad_watch(), the kernel cycles through the range of possible
   watch descriptors incrementally. When allocating a free watch
   descriptor, no check is made to see whether that watch descriptor
   number has any pending unread events in the inotify queue.
3. Thus, it can happen that a watch descriptor is reallocated even
   when pending unread events exist for a previous incarnation of
   that watch descriptor number, with the result that the application
   might then read those events and interpret them as belonging to 
   the file associated with the newly recycle watch descriptor.

However, this bug is perhaps more theoretical, than practical.
The range for watch descriptors is from 0 to INT_MAX, so that in
order to trigger the bug, an application must leave unread events
on the inotify queue while recycling INT_MAX watch descriptors.
]]

>  .\" FIXME kernel commit 611da04f7a31b2208e838be55a42c7a1310ae321
>  .\" implies that unmount events were buggy 2.6.11 to 2.6.36
>  .\"
> @@ -745,7 +757,7 @@ However, as an unintended effect of other changes,
>  since Linux 2.6.36, an
>  .B IN_IGNORED
>  event is generated in this case.
> -
> +.PP

(Not sure why you added this piece?)

>  Before kernel 2.6.25,
>  .\" commit 1c17d18e3775485bf1e0ce79575eb637a94494a2
>  the kernel code that was intended to coalesce successive identical events

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
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

Re: [PATCH 1/1] inotify.7: Bug 77111 - watch descriptor reuse

[Heinrich Schuchardt]

On 30.05.2014 15:12, Michael Kerrisk (man-pages) wrote:
> [Adding Jan and Eric to the CC, in case they want to comment.]
>
> Background is bugs https://bugzilla.kernel.org/show_bug.cgi?id=76851
> and https://bugzilla.kernel.org/show_bug.cgi?id=77111 . The point is:
>
> 1. When an inotify watch descriptor is removed, pending unread
>     events remain pending.
> 2. When allocating a new watch descriptor, a past WD may
>     be recycled.
> 3. In theory, it could happen that events left over at 1 could
>     be interpreted as though they belonged to the filesystem
>     object watch in step 2.
>
> On 05/29/2014 07:39 PM, Heinrich Schuchardt wrote:
>> 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.
>
> It's unfortunate, but I'm not sure that it's very serious. I mean, in
> order to trigger this bug you need to
>
> 0. Remove your watch descriptor (wd1),
> 1. Leave some unread events for wd1 on the queue. and in the meantime,
> 2. Cycle through INT_MAX watch descriptors until you reuse wd1.
>
> Unless I'm missing something, the chances of that are vanishingly small.
> However, that's not to say that the issue shouldn't be documented. Rather,
> if the issue is as unlikely to be hit as it appears to me in the above
> formulation, then I thing the tome of the write-up should indicate
> that the problem is more theoretical than practical. Perhaps Jan or Eric
> has a comment about this.

68 events per second add up to INT_MAX events a year.

A server application restarted only once a year and handling a few 
hundred request a second may be at risk. My idling workstation rebooted 
once a day will never be hit.

I would feel more comfortable if the problem were not only documented 
but fixed.

Maybe Jan or Eric can comment on the following solution idea:

Couldn't the idr ID be released in inotify_read when hitting IN_IGNORED 
instead of being released inside inotify_rm_watch?

Best regards

Heinrich

>
>> Actually in rare cases watch descriptor IDs are returned by inotify_add_watch
>> for which events are still on the inotify queue.
>>
>> cf. https://bugzilla.kernel.org/show_bug.cgi?id=77111
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
>> ---
>>   man7/inotify.7 | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/man7/inotify.7 b/man7/inotify.7
>> index 1fc83f8..b1c2f6f 100644
>> --- a/man7/inotify.7
>> +++ b/man7/inotify.7
>> @@ -728,6 +728,18 @@ and the accompanying
>>   event might be fetched only on the next
>>   .BR read (2).
>>   .SH BUGS
>> +As of Linux 3.14,
>> +the following bug exists:
>> +.IP * 3
>> +.\" FIXME https://bugzilla.kernel.org/show_bug.cgi?id=77111
>> +.BR inotify_add_watch (2)
>> +may return a watch descriptor ID released by a prior call to
>> +.BR inotify_rm_watch (2)
>> +even if events for this watch descriptor still exist on the inotify queue.
>> +As a workaround the inotify file descriptor can be read until the queue is
>> +empty before calling
>> +.BR inotify_add_watch (2).
>> +.PP
>
> Perhaps a wording something like this is more appropriate:
>
> [[
> As at Linux 3.15, the a possible bug exists in the following scenario:
>
> 1. When a watch descriptor is removed, any pending unread events for
>     that watch descriptor remain available to read.
> 2. As watch descriptors are subsequently allocated with
>     inotify_ad_watch(), the kernel cycles through the range of possible
>     watch descriptors incrementally. When allocating a free watch
>     descriptor, no check is made to see whether that watch descriptor
>     number has any pending unread events in the inotify queue.
> 3. Thus, it can happen that a watch descriptor is reallocated even
>     when pending unread events exist for a previous incarnation of
>     that watch descriptor number, with the result that the application
>     might then read those events and interpret them as belonging to
>     the file associated with the newly recycle watch descriptor.
>
> However, this bug is perhaps more theoretical, than practical.
> The range for watch descriptors is from 0 to INT_MAX, so that in
> order to trigger the bug, an application must leave unread events
> on the inotify queue while recycling INT_MAX watch descriptors.
> ]]
>
>>   .\" FIXME kernel commit 611da04f7a31b2208e838be55a42c7a1310ae321
>>   .\" implies that unmount events were buggy 2.6.11 to 2.6.36
>>   .\"
>> @@ -745,7 +757,7 @@ However, as an unintended effect of other changes,
>>   since Linux 2.6.36, an
>>   .B IN_IGNORED
>>   event is generated in this case.
>> -
>> +.PP
>
> (Not sure why you added this piece?)
>
>>   Before kernel 2.6.25,
>>   .\" commit 1c17d18e3775485bf1e0ce79575eb637a94494a2
>>   the kernel code that was intended to coalesce successive identical events
>
> Cheers,
>
> Michael
>
>

--
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

Re: [PATCH 1/1] inotify.7: Bug 77111 - watch descriptor reuse

[Michael Kerrisk (man-pages)]

On 05/31/2014 11:25 PM, Heinrich Schuchardt wrote:
> On 30.05.2014 15:12, Michael Kerrisk (man-pages) wrote:
>> [Adding Jan and Eric to the CC, in case they want to comment.]
>>
>> Background is bugs https://bugzilla.kernel.org/show_bug.cgi?id=76851
>> and https://bugzilla.kernel.org/show_bug.cgi?id=77111 . The point is:
>>
>> 1. When an inotify watch descriptor is removed, pending unread
>>     events remain pending.
>> 2. When allocating a new watch descriptor, a past WD may
>>     be recycled.
>> 3. In theory, it could happen that events left over at 1 could
>>     be interpreted as though they belonged to the filesystem
>>     object watch in step 2.
>>
>> On 05/29/2014 07:39 PM, Heinrich Schuchardt wrote:
>>> 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.
>>
>> It's unfortunate, but I'm not sure that it's very serious. I mean, in
>> order to trigger this bug you need to
>>
>> 0. Remove your watch descriptor (wd1),
>> 1. Leave some unread events for wd1 on the queue. and in the meantime,
>> 2. Cycle through INT_MAX watch descriptors until you reuse wd1.
>>
>> Unless I'm missing something, the chances of that are vanishingly small.
>> However, that's not to say that the issue shouldn't be documented. Rather,
>> if the issue is as unlikely to be hit as it appears to me in the above
>> formulation, then I thing the tome of the write-up should indicate
>> that the problem is more theoretical than practical. Perhaps Jan or Eric
>> has a comment about this.
> 
> 68 events per second add up to INT_MAX events a year.
> 
> A server application restarted only once a year and handling a few 
> hundred request a second may be at risk. My idling workstation rebooted 
> once a day will never be hit.

Yes, but you skip the other piece of the puzzle. This bug relies on us
not reading events while at the same time cycling through INT_MAX watches.
That's pretty unlikey in a sane application.

> I would feel more comfortable if the problem were not only documented 
> but fixed.

Agreed, it's better if the bug were not there. It's a question of 
how much effort is required, and if there would be any significant 
performance hit associated with the fix. Weighed up against the risk
that the bug could be hit.

> Maybe Jan or Eric can comment on the following solution idea:
> 
> Couldn't the idr ID be released in inotify_read when hitting IN_IGNORED 
> instead of being released inside inotify_rm_watch?

See comments above. What would be the performance impact here? This
would be some kind of check on *every* read(), no?
Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
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

Re: [PATCH 1/1] inotify.7: Bug 77111 - watch descriptor reuse

[Heinrich Schuchardt]

On 01.06.2014 10:39, Michael Kerrisk (man-pages) wrote:
> On 05/31/2014 11:25 PM, Heinrich Schuchardt wrote:
>> On 30.05.2014 15:12, Michael Kerrisk (man-pages) wrote:
>>> [Adding Jan and Eric to the CC, in case they want to comment.]
>>>
>>> Background is bugs https://bugzilla.kernel.org/show_bug.cgi?id=76851
>>> and https://bugzilla.kernel.org/show_bug.cgi?id=77111 . The point is:
>>>
>>> 1. When an inotify watch descriptor is removed, pending unread
>>>      events remain pending.
>>> 2. When allocating a new watch descriptor, a past WD may
>>>      be recycled.
>>> 3. In theory, it could happen that events left over at 1 could
>>>      be interpreted as though they belonged to the filesystem
>>>      object watch in step 2.
>>>
>>> On 05/29/2014 07:39 PM, Heinrich Schuchardt wrote:
>>>> 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.
>>>
>>> It's unfortunate, but I'm not sure that it's very serious. I mean, in
>>> order to trigger this bug you need to
>>>
>>> 0. Remove your watch descriptor (wd1),
>>> 1. Leave some unread events for wd1 on the queue. and in the meantime,
>>> 2. Cycle through INT_MAX watch descriptors until you reuse wd1.
>>>
>>> Unless I'm missing something, the chances of that are vanishingly small.
>>> However, that's not to say that the issue shouldn't be documented. Rather,
>>> if the issue is as unlikely to be hit as it appears to me in the above
>>> formulation, then I thing the tome of the write-up should indicate
>>> that the problem is more theoretical than practical. Perhaps Jan or Eric
>>> has a comment about this.
>>
>> 68 events per second add up to INT_MAX events a year.
>>
>> A server application restarted only once a year and handling a few
>> hundred request a second may be at risk. My idling workstation rebooted
>> once a day will never be hit.
>
> Yes, but you skip the other piece of the puzzle. This bug relies on us
> not reading events while at the same time cycling through INT_MAX watches.
> That's pretty unlikey in a sane application.

In my example code there were just three events waiting on the queue and 
they started waiting only **after** cycling through INT_MAX watches.

Best regards

Heinrich

>
>> I would feel more comfortable if the problem were not only documented
>> but fixed.
>
> Agreed, it's better if the bug were not there. It's a question of
> how much effort is required, and if there would be any significant
> performance hit associated with the fix. Weighed up against the risk
> that the bug could be hit.
>
>> Maybe Jan or Eric can comment on the following solution idea:
>>
>> Couldn't the idr ID be released in inotify_read when hitting IN_IGNORED
>> instead of being released inside inotify_rm_watch?
>
> See comments above. What would be the performance impact here? This
> would be some kind of check on *every* read(), no?
> Cheers,
>
> Michael
>
>

--
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

[PATCH 1/1 v2] inotify.7: Bug 77111 - watch descriptor reuse

[Heinrich Schuchardt]

The revised patch below uses the wording suggested by Michael Kerrisk.

***

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.

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

Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
---
 man7/inotify.7 | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/man7/inotify.7 b/man7/inotify.7
index 1fc83f8..d64523f 100644
--- a/man7/inotify.7
+++ b/man7/inotify.7
@@ -754,6 +754,24 @@ if the older had not yet been read)
 instead checked if the most recent event could be coalesced with the
 .I oldest
 unread event.
+
+As of Linux 3.15.1, the following bug exists:
+.\" FIXME https://bugzilla.kernel.org/show_bug.cgi?id=77111
+When a watch descriptor is removed by calling
+.BR inotify_rm_watch (2),
+any pending unread events for that watch descriptor remain available to read.
+As watch descriptors are subsequently allocated with
+.BR inotify_add_watch (2),
+the kernel cycles through the range of possible watch descriptors (0 to
+.BR INT_MAX )
+incrementally.
+When allocating a free watch descriptor, no check is made to see whether that
+watch descriptor number has any pending unread events in the inotify queue.
+Thus, it can happen that a watch descriptor is reallocated even
+when pending unread events exist for a previous incarnation of
+that watch descriptor number, with the result that the application
+might then read those events and interpret them as belonging to 
+the file associated with the newly recycled watch descriptor.
 .SH EXAMPLE
 The following program demonstrates the usage of the inotify API.
 It marks the directories passed as a command-line arguments
-- 
2.0.0

--
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

Re: [PATCH 1/1] inotify.7: Bug 77111 - watch descriptor reuse

[Michael Kerrisk (man-pages)]

On 06/01/2014 01:29 PM, Heinrich Schuchardt wrote:
> On 01.06.2014 10:39, Michael Kerrisk (man-pages) wrote:
>> On 05/31/2014 11:25 PM, Heinrich Schuchardt wrote:
>>> On 30.05.2014 15:12, Michael Kerrisk (man-pages) wrote:
>>>> [Adding Jan and Eric to the CC, in case they want to comment.]
>>>>
>>>> Background is bugs https://bugzilla.kernel.org/show_bug.cgi?id=76851
>>>> and https://bugzilla.kernel.org/show_bug.cgi?id=77111 . The point is:
>>>>
>>>> 1. When an inotify watch descriptor is removed, pending unread
>>>>      events remain pending.
>>>> 2. When allocating a new watch descriptor, a past WD may
>>>>      be recycled.
>>>> 3. In theory, it could happen that events left over at 1 could
>>>>      be interpreted as though they belonged to the filesystem
>>>>      object watch in step 2.
>>>>
>>>> On 05/29/2014 07:39 PM, Heinrich Schuchardt wrote:
>>>>> 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.
>>>>
>>>> It's unfortunate, but I'm not sure that it's very serious. I mean, in
>>>> order to trigger this bug you need to
>>>>
>>>> 0. Remove your watch descriptor (wd1),
>>>> 1. Leave some unread events for wd1 on the queue. and in the meantime,
>>>> 2. Cycle through INT_MAX watch descriptors until you reuse wd1.
>>>>
>>>> Unless I'm missing something, the chances of that are vanishingly small.
>>>> However, that's not to say that the issue shouldn't be documented. Rather,
>>>> if the issue is as unlikely to be hit as it appears to me in the above
>>>> formulation, then I thing the tome of the write-up should indicate
>>>> that the problem is more theoretical than practical. Perhaps Jan or Eric
>>>> has a comment about this.
>>>
>>> 68 events per second add up to INT_MAX events a year.
>>>
>>> A server application restarted only once a year and handling a few
>>> hundred request a second may be at risk. My idling workstation rebooted
>>> once a day will never be hit.
>>
>> Yes, but you skip the other piece of the puzzle. This bug relies on us
>> not reading events while at the same time cycling through INT_MAX watches.
>> That's pretty unlikey in a sane application.
> 
> In my example code there were just three events waiting on the queue and 
> they started waiting only **after** cycling through INT_MAX watches.

[Sorry for not following up earlier.]

Ah yes, I see what you mean. I'd slightly misapprehended the required 
scenario. I think the chances of hitting the bug are still pretty low,
though higher than I'd originally thought. You'd need to have an
application that cycles through INT_MAX descriptors, and happens to
get unlucky by deallocating a WD that still has some unread events 
on the queue, and then reallocating that WD without first draining 
the queue, right? Definitely worth documenting, so that if someone does
hit this (Eric Paris seems also to think it pretty unlikely), then at 
least someone has a clue about the problem.

Cheers,

Michael


>>> I would feel more comfortable if the problem were not only documented
>>> but fixed.
>>
>> Agreed, it's better if the bug were not there. It's a question of
>> how much effort is required, and if there would be any significant
>> performance hit associated with the fix. Weighed up against the risk
>> that the bug could be hit.
>>
>>> Maybe Jan or Eric can comment on the following solution idea:
>>>
>>> Couldn't the idr ID be released in inotify_read when hitting IN_IGNORED
>>> instead of being released inside inotify_rm_watch?
>>
>> See comments above. What would be the performance impact here? This
>> would be some kind of check on *every* read(), no?
>> Cheers,
>>
>> Michael
>>
>>
> 
> 


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
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

Re: [PATCH 1/1 v2] inotify.7: Bug 77111 - watch descriptor reuse

[Michael Kerrisk (man-pages)]

On 06/22/2014 04:45 PM, Heinrich Schuchardt wrote:
> The revised patch below uses the wording suggested by Michael Kerrisk.
> 
> ***
> 
> 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.
> 
> cf. https://bugzilla.kernel.org/show_bug.cgi?id=77111
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
> ---
>  man7/inotify.7 | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/man7/inotify.7 b/man7/inotify.7
> index 1fc83f8..d64523f 100644
> --- a/man7/inotify.7
> +++ b/man7/inotify.7
> @@ -754,6 +754,24 @@ if the older had not yet been read)
>  instead checked if the most recent event could be coalesced with the
>  .I oldest
>  unread event.
> +
> +As of Linux 3.15.1, the following bug exists:
> +.\" FIXME https://bugzilla.kernel.org/show_bug.cgi?id=77111
> +When a watch descriptor is removed by calling
> +.BR inotify_rm_watch (2),
> +any pending unread events for that watch descriptor remain available to read.
> +As watch descriptors are subsequently allocated with
> +.BR inotify_add_watch (2),
> +the kernel cycles through the range of possible watch descriptors (0 to
> +.BR INT_MAX )
> +incrementally.
> +When allocating a free watch descriptor, no check is made to see whether that
> +watch descriptor number has any pending unread events in the inotify queue.
> +Thus, it can happen that a watch descriptor is reallocated even
> +when pending unread events exist for a previous incarnation of
> +that watch descriptor number, with the result that the application
> +might then read those events and interpret them as belonging to 
> +the file associated with the newly recycled watch descriptor.
>  .SH EXAMPLE
>  The following program demonstrates the usage of the inotify API.
>  It marks the directories passed as a command-line arguments

Thanks, Heinrich. Applied with some minor tweaks, and pushed to 
kernel.org Git. 

I also added this piece:

[[
In practice, the likelihood of hitting this bug may be extremely low,
since it requires that an application cycle through
.B INT_MAX
watch descriptors,
release a watch descriptor while leaving unread events for that
watch descriptor in the queue in the queue,
and then recycle that watch descriptor.
For this reason, and because there have been no reports
of the bug occurring in real-world applications,
as of Linux 3.15,
.\" FIXME https://bugzilla.kernel.org/show_bug.cgi?id=77111
no kernel changes have yet been made to eliminate this possible bug.
]]

Okay?

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
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

Re: [PATCH 1/1 v2] inotify.7: Bug 77111 - watch descriptor reuse

[Heinrich Schuchardt]

Hello Michael,

the further descripton is fine for me. Please, integrate it into the inotify(7) page.

Best regards

Heinrich Schuchardt

Am 23.06.14 um 10:21 schrieb Michael Kerrisk (man-pages)

> On 06/22/2014 04:45 PM, Heinrich Schuchardt wrote:
> 
> > The revised patch below uses the wording suggested by Michael Kerrisk.
> 
> > 
> 
> > ***
> 
> > 
> 
> > 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.
> 
> > 
> 
> > cf. https://bugzilla.kernel.org/show_bug.cgi?id=77111
> 
> > 
> 
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
> 
> > ---
> 
> >  man7/inotify.7 | 18 ++++++++++++++++++
> 
> >  1 file changed, 18 insertions(+)
> 
> > 
> 
> > diff --git a/man7/inotify.7 b/man7/inotify.7
> 
> > index 1fc83f8..d64523f 100644
> 
> > --- a/man7/inotify.7
> 
> > +++ b/man7/inotify.7
> 
> > @@ -754,6 +754,24 @@ if the older had not yet been read)
> 
> >  instead checked if the most recent event could be coalesced with the
> 
> >  .I oldest
> 
> >  unread event.
> 
> > +
> 
> > +As of Linux 3.15.1, the following bug exists:
> 
> > +.\" FIXME https://bugzilla.kernel.org/show_bug.cgi?id=77111
> 
> > +When a watch descriptor is removed by calling
> 
> > +.BR inotify_rm_watch (2),
> 
> > +any pending unread events for that watch descriptor remain available to read.
> 
> > +As watch descriptors are subsequently allocated with
> 
> > +.BR inotify_add_watch (2),
> 
> > +the kernel cycles through the range of possible watch descriptors (0 to
> 
> > +.BR INT_MAX )
> 
> > +incrementally.
> 
> > +When allocating a free watch descriptor, no check is made to see whether that
> 
> > +watch descriptor number has any pending unread events in the inotify queue.
> 
> > +Thus, it can happen that a watch descriptor is reallocated even
> 
> > +when pending unread events exist for a previous incarnation of
> 
> > +that watch descriptor number, with the result that the application
> 
> > +might then read those events and interpret them as belonging to 
> 
> > +the file associated with the newly recycled watch descriptor.
> 
> >  .SH EXAMPLE
> 
> >  The following program demonstrates the usage of the inotify API.
> 
> >  It marks the directories passed as a command-line arguments
> 
> 
> 
> Thanks, Heinrich. Applied with some minor tweaks, and pushed to 
> 
> kernel.org Git. 
> 
> 
> 
> I also added this piece:
> 
> 
> 
> [[
> 
> In practice, the likelihood of hitting this bug may be extremely low,
> 
> since it requires that an application cycle through
> 
> .B INT_MAX
> 
> watch descriptors,
> 
> release a watch descriptor while leaving unread events for that
> 
> watch descriptor in the queue in the queue,
> 
> and then recycle that watch descriptor.
> 
> For this reason, and because there have been no reports
> 
> of the bug occurring in real-world applications,
> 
> as of Linux 3.15,
> 
> .\" FIXME https://bugzilla.kernel.org/show_bug.cgi?id=77111
> 
> no kernel changes have yet been made to eliminate this possible bug.
> 
> ]]
> 
> 
> 
> Okay?
> 
> 
> 
> Cheers,
> 
> 
> 
> Michael
> 
> 
> 
> 
> 
> -- 
> 
> Michael Kerrisk
> 
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> 
> Linux/UNIX System Programming Training: http://man7.org/training/
--
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

Re: [PATCH 1/1 v2] inotify.7: Bug 77111 - watch descriptor reuse

[Michael Kerrisk (man-pages)]

On Mon, Jun 23, 2014 at 11:45 AM, Heinrich Schuchardt
<xypron.glpk-Mmb7MZpHnFY@public.gmane.org> wrote:
> Hello Michael,
>
> the further descripton is fine for me. Please, integrate it into the inotify(7) page.

Thanks, Heinrich. Done.

Cheers,

Michael


> Heinrich Schuchardt
>
> Am 23.06.14 um 10:21 schrieb Michael Kerrisk (man-pages)
>
>> On 06/22/2014 04:45 PM, Heinrich Schuchardt wrote:
>>
>> > The revised patch below uses the wording suggested by Michael Kerrisk.
>>
>> >
>>
>> > ***
>>
>> >
>>
>> > 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.
>>
>> >
>>
>> > cf. https://bugzilla.kernel.org/show_bug.cgi?id=77111
>>
>> >
>>
>> > Signed-off-by: Heinrich Schuchardt <xypron.glpk-Mmb7MZpHnFY@public.gmane.org>
>>
>> > ---
>>
>> >  man7/inotify.7 | 18 ++++++++++++++++++
>>
>> >  1 file changed, 18 insertions(+)
>>
>> >
>>
>> > diff --git a/man7/inotify.7 b/man7/inotify.7
>>
>> > index 1fc83f8..d64523f 100644
>>
>> > --- a/man7/inotify.7
>>
>> > +++ b/man7/inotify.7
>>
>> > @@ -754,6 +754,24 @@ if the older had not yet been read)
>>
>> >  instead checked if the most recent event could be coalesced with the
>>
>> >  .I oldest
>>
>> >  unread event.
>>
>> > +
>>
>> > +As of Linux 3.15.1, the following bug exists:
>>
>> > +.\" FIXME https://bugzilla.kernel.org/show_bug.cgi?id=77111
>>
>> > +When a watch descriptor is removed by calling
>>
>> > +.BR inotify_rm_watch (2),
>>
>> > +any pending unread events for that watch descriptor remain available to read.
>>
>> > +As watch descriptors are subsequently allocated with
>>
>> > +.BR inotify_add_watch (2),
>>
>> > +the kernel cycles through the range of possible watch descriptors (0 to
>>
>> > +.BR INT_MAX )
>>
>> > +incrementally.
>>
>> > +When allocating a free watch descriptor, no check is made to see whether that
>>
>> > +watch descriptor number has any pending unread events in the inotify queue.
>>
>> > +Thus, it can happen that a watch descriptor is reallocated even
>>
>> > +when pending unread events exist for a previous incarnation of
>>
>> > +that watch descriptor number, with the result that the application
>>
>> > +might then read those events and interpret them as belonging to
>>
>> > +the file associated with the newly recycled watch descriptor.
>>
>> >  .SH EXAMPLE
>>
>> >  The following program demonstrates the usage of the inotify API.
>>
>> >  It marks the directories passed as a command-line arguments
>>
>>
>>
>> Thanks, Heinrich. Applied with some minor tweaks, and pushed to
>>
>> kernel.org Git.
>>
>>
>>
>> I also added this piece:
>>
>>
>>
>> [[
>>
>> In practice, the likelihood of hitting this bug may be extremely low,
>>
>> since it requires that an application cycle through
>>
>> .B INT_MAX
>>
>> watch descriptors,
>>
>> release a watch descriptor while leaving unread events for that
>>
>> watch descriptor in the queue in the queue,
>>
>> and then recycle that watch descriptor.
>>
>> For this reason, and because there have been no reports
>>
>> of the bug occurring in real-world applications,
>>
>> as of Linux 3.15,
>>
>> .\" FIXME https://bugzilla.kernel.org/show_bug.cgi?id=77111
>>
>> no kernel changes have yet been made to eliminate this possible bug.
>>
>> ]]
>>
>>
>>
>> Okay?
>>
>>
>>
>> Cheers,
>>
>>
>>
>> Michael
>>
>>
>>
>>
>>
>> --
>>
>> Michael Kerrisk
>>
>> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
>>
>> Linux/UNIX System Programming Training: http://man7.org/training/



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
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