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

["Michael Kerrisk (man-pages)" <mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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