Re: Potential regression after fsnotify_nameremove() rework in 5.3

[Ivan Delalande <colona@arista.com>]

On Sun, Jan 16, 2022 at 12:14:01PM +0200, Amir Goldstein wrote:
> On Sun, Jan 16, 2022 at 3:20 AM Ivan Delalande <colona@arista.com> wrote:
>> On Sat, Jan 15, 2022 at 09:50:20PM +0200, Amir Goldstein wrote:
>>> On Sat, Jan 15, 2022 at 5:11 AM Ivan Delalande <colona@arista.com> wrote:
>>>> Sorry to bring this up so late but we might have found a regression
>>>> introduced by your "Sort out fsnotify_nameremove() mess" patch series
>>>> merged in 5.3 (116b9731ad76..7377f5bec133), and that can still be
>>>> reproduced on v5.16.
>>>>
>>>> Some of our processes use inotify to watch for IN_DELETE events (for
>>>> files on tmpfs mostly), and relied on the fact that once such events are
>>>> received, the files they refer to have actually been unlinked and can't
>>>> be open/read. So if and once open() succeeds then it is a new version of
>>>> the file that has been recreated with new content.
>>>>
>>>> This was true and working reliably before 5.3, but changed after
>>>> 49246466a989 ("fsnotify: move fsnotify_nameremove() hook out of
>>>> d_delete()") specifically. There is now a time window where a process
>>>> receiving one of those IN_DELETE events may still be able to open the
>>>> file and read its old content before it's really unlinked from the FS.
>>>
>>> This is a bit surprising to me.
>>> Do you have a reproducer?
>>
>> Yeah, I was using the following one to bisect this. It will print a
>> message every time it succeeds to read the file after receiving a
>> IN_DELETE event when run with something like `mkdir /tmp/foo;
>> ./indelchurn /tmp/foo`. It seems to hit pretty frequently and reliably
>> on various systems after 5.3, even for different #define-parameters.
>>
> 
> I see yes, it's a race between fsnotify_unlink() and d_delete()
> fsnotify_unlink() in explicitly required to be called before d_delete(), so
> it has the d_inode information and that leaves a windows for opening
> the file from cached dentry before d_delete().
> 
> I would rather that we try to address this not as a regression until
> there is proof of more users that expect the behavior you mentioned.
> I would like to provide you an API to opt-in for this behavior, because
> fixing it for everyone may cause other workloads to break.
> 
> Please test the attached patch on top of v5.16 and use
> IN_DELETE|IN_EXCL_UNLINK as the watch mask for testing.
> 
> I am assuming that it would be possible for you to modify the application
> and add the IN_EXCL_UNLINK flag and that your application does not
> care about getting IN_OPEN events on unlinked files?
> 
> My patch overloads the existing flag IN_EXCL_UNLINK with a new
> meaning. It's a bit of a hack and we can use some other flag if we need to
> but it actually makes some sense that an application that does not care for
> events on d_unlinked() files will be guaranteed to not get those events
> after getting an IN_DELETE event. It is another form of the race that you
> described.
> 
> Will that solution work out for you?

Yeah, sounds perfect for us, and adding IN_EXCL_UNLINK to our
applications is fine indeed. I've tested the 5.16 patch on my laptop
with the reproducer and can't reproduce the issue. I've also tried the
5.10 patch on our products and also stop seeing the issue both with
the reproducer but also with our internal applications and test cases
that made us look into this initially. So this looks like a good fix on
our side at least.

>>>> I'm not very familiar with the VFS and fsnotify internals, would you
>>>> consider this a regression, or was there never any intentional guarantee
>>>> for that behavior and it's best we work around this change in userspace?
>>>
>>> It may be a regression if applications depend on behavior that changed,
>>> but if are open for changes in your application please describe in more details
>>> what it tries to accomplish using IN_DELETE and the ekernel your application
>>> is running on and then I may be able to recommend a more reliable method.
>>
>> I can discuss it with our team and get more details on this but it may
>> be pretty complicated and costly to change. My understanding is that
>> these watched files are used as ID/version references for in-memory
>> objects shared between multiple processes to synchronize state, and
>> resynchronize when there are crashes or restarts. So they can be
>> recreated in place with the same or a different content, and so their
>> inode number or mtime etc. may not be useable as additonnal check.
>>
>> I think we can also have a very large number of these objects and files
>> on some configurations, so waiting to see if we have a following
>> IN_CREATE, or adding more checks/synchronization logic will probably
>> have a significant impact at scale.
>>
>> And we're targeting 5.10 and building it with our own config.
>>
> 
> I am not convinced that this is needed as a regression fix for stable
> kernels, but since you are building your own kernel I can help you do the
> backport - attached *untested* patch for v5.10 that also backport of
> "fsnotify: pass dentry instead of inode data".

Thanks a lot for making the backport too, actually later 5.10.y have
your 950cc0d2bef0 and fecc4559780d changes, so I think we should just
use the same fsnotify.c patch hunk as your 5.16 patch (but yeah still
need the fsnotify_data_dentry bit). That's what I tested on our product,
and as mentionned above seems to be working great.

And we do carry a few custom patches and mainline backports so it's
perfectly fine if you prefer not to submit it to stable.

Thanks again,

-- 
Ivan Delalande
Arista Networks