linux-kernel - Re: [PATCH v3 0/3] fsnotify: fix softlockups iterating over d

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 1 Nov 2022 18:51:44 +0100
From:   Jan Kara <jack@...e.cz>
To:     Stephen Brennan <stephen.s.brennan@...cle.com>
Cc:     Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, Amir Goldstein <amir73il@...il.com>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v3 0/3] fsnotify: fix softlockups iterating over d_subdirs

Hi Stephen!

On Thu 27-10-22 17:10:13, Stephen Brennan wrote:
> Here is v3 of the patch series. I've taken all of the feedback,
> thanks Amir, Christian, Hilf, et al. Differences are noted in each
> patch.
> 
> I caught an obvious and silly dentry reference leak: d_find_any_alias()
> returns a reference, which I never called dput() on. With that change, I
> no longer see the rpc_pipefs issue, but I do think I need more testing
> and thinking through the third patch. Al, I'd love your feedback on that
> one especially.
> 
> Thanks,
> Stephen
> 
> Stephen Brennan (3):
>   fsnotify: Use d_find_any_alias to get dentry associated with inode
>   fsnotify: Protect i_fsnotify_mask and child flags with inode rwsem
>   fsnotify: allow sleepable child flag update

Thanks for the patches Stephen and I'm sorry for replying somewhat late.
The first patch is a nobrainer. The other two patches ... complicate things
somewhat more complicated than I'd like. I guess I can live with them if we
don't find a better solution but I'd like to discuss a bit more about
alternatives.

So what would happen if we just clear DCACHE_FSNOTIFY_PARENT_WATCHED in
__fsnotify_parent() for the dentry which triggered the event and does not
have watched parent anymore and never bother with full children walk? I
suppose your contention problems will be gone, we'll just pay the price of
dget_parent() + fsnotify_inode_watches_children() for each child that
falsely triggers instead of for only one. Maybe that's not too bad? After
all any event upto this moment triggered this overhead as well...

Am I missing something? AFAIU this would allow us to avoid the games with
the new connector flag etc... We would probably still need to avoid
softlockups when setting the flag DCACHE_FSNOTIFY_PARENT_WATCHED but that
should be much simpler (we could use i_rwsem trick like you do).

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR